Paths

Table of Contentst

Differential D9684

Fix T80979: Fixed the bevel distortion issue at bends
AbandonedPublic
Actions

Authored by Dilith Jayakody (dilithjay) on Dec 1 2020, 7:11 AM.

Details

Reviewers

None

Group Reviewers

Modeling

Summary

Problem
Prior to patch, the scale of the bevel remained constant through out the path. This resulted in the model appearing distorted / thinner at corners / bends. This is as mentioned in T80979: Account for curvature in curve to mesh node.

Solution
The proposed solution is to calculate the vertices of the bevel piece by using the vertices of the previous piece and adding to it the direction vector formed between the 2 bevel pieces. The correct coordinates are identified using the intersection point between the direction vector and the plane of the bevel piece face. I tested the speed of the bevel creation and didn't notice any difference between the previous method and this. I am yet to find any unexpected problems but more testing is ideal.

Alternative Solutions attempted
I attempted scaling based on the angle but it's behavior was not as I expected. The scaling had to be done in a different direction based on the orientation of the face which I was unable to get to work properly because the relative orientation of the face appears to vary when the global orientation was altered which made it unpredictable.

Diff Detail

Repository: rB Blender

Event Timeline

Dilith Jayakody (dilithjay) requested review of this revision.Dec 1 2020, 7:11 AM

Dilith Jayakody (dilithjay) created this revision.

Dilith Jayakody (dilithjay) added a reviewer: Good First Issue.Dec 1 2020, 7:20 AM

Dilith Jayakody (dilithjay) mentioned this in T80979: Account for curvature in curve to mesh node.Dec 1 2020, 9:01 AM

Ankit Meel (ankitm) removed a reviewer: Good First Issue.Dec 1 2020, 10:19 AM

Roman (roman13) awarded a token.Dec 1 2020, 11:25 AM

RedMser (RedMser) added a subscriber: RedMser (RedMser).Dec 1 2020, 11:30 AM

Generally the functionality does seem to work. I tried it on some various curves locally. It will be a welcome change if/when the patches for this issue make it in. While I'm not an official dev, I did take a quick look at the code and have a few comments.

source/blender/blenkernel/intern/displist.c
1333	In general, check inside `BLI_math_vector.h` for a multitude of math functions like, e.g. the dot product of 2 vectors (which you appear to be doing as part of this overly long expression). Right now this is very hard to read and the result is stored in a non-descript variable t.
1335–1337	e.g. These 3 lines can be replaced with `madd_v3_v3fl(nonscaled_data, dirvec, t)`
1685	This leaks memory - you will need to free this when done.

Jesse Yurkovich (deadpin) mentioned this in D9678: Bevel on curves does not create distorted geometry anymore.Dec 1 2020, 11:56 AM

Vitalii Vlasov (Fracture128) awarded a token.Dec 3 2020, 9:15 AM

In D9684#240756, @Jesse Yurkovich (deadpin) wrote:

Generally the functionality does seem to work. I tried it on some various curves locally. It will be a welcome change if/when the patches for this issue make it in. While I'm not an official dev, I did take a quick look at the code and have a few comments.

@Jesse Yurkovich (deadpin) Sorry about the late response and thanks a lot for the tips. I'll take a look at the alterations you suggested.

I made the following updates as per @Jesse Yurkovich (deadpin) 's suggestions.

Dealt with the memory leak due to not freeing up allocated memory for nonscaled_data
Renamed non-descriptive variable t to multiplier
Used already defined methods to make the code cleaner and more readable for finding multiplier

Thanks for the update. You can mark my prior comments as completed so the review is easier to follow. I've left another few comments here though that should be addressed.

source/blender/blenkernel/intern/displist.c
1306–1307	This looks unsafe to do, you are accessing out of bounds memory (`bevp - 1`) when this function is called for the first time from `do_makeDispListCurveTypes` The first time through `bevp` is already at the start of the array. Because `dirvec` is only used further below, inside a conditional which looks to already account for this case, maybe you can just define the variable there. It's good practice to only declare the variable inside the scope that it will be used anyhow.
1338–1340	These 2 lines have tab characters instead of spaces. Ensure clang-format is happy if you can on your side as it would catch such issues.
1342–1345	These 3 lines can just be `madd_v3_v3fl(data, vec, radius_factor);`

In D9684#242056, @Jesse Yurkovich (deadpin) wrote:

Thanks for the update. You can mark my prior comments as completed so the review is easier to follow. I've left another few comments here though that should be addressed.

Oh right. Didn't notice them. I'll make the changes. Thanks for checking.

Updated as per @Jesse Yurkovich (deadpin) 's suggestions

Moved the declaration and definition of dirvec into the scope in which it's being used.
Fixed indentation issue.
Replaced more lines of code with predefined methods.

Dilith Jayakody (dilithjay) marked 3 inline comments as done.Dec 4 2020, 4:53 AM

Jesse Yurkovich (deadpin) added a project: Modeling.Dec 16 2020, 2:48 AM

@Dilith Jayakody (dilithjay) How can i test this patch?

@Roman (roman13) If you have Blender's source code and have set up a build environment (https://wiki.blender.org/wiki/Building_Blender), you can compile your own Blender with this patch.

This seems to do the trick. I didn't think of doing it this way at all actually. My main issues with this approach are the complexity and overhead of keeping track of a separate array with the "non-scaled" data.
The first thing that's a bit odd about that is that it's first built in the same function that it's used in on the first iteration. Then it's also changed at each step, which invalidates the name a bit.

I think I prefer the approach in D9678 which is more aligned with what Campbell originally suggested. Regardless of which one we end up going with, thanks for working on this.

In D9684#247843, @Hans Goudey (HooglyBoogly) wrote:

@Roman (roman13) If you have Blender's source code and have set up a build environment (https://wiki.blender.org/wiki/Building_Blender), you can compile your own Blender with this patch.

This seems to do the trick. I didn't think of doing it this way at all actually. My main issues with this approach are the complexity and overhead of keeping track of a separate array with the "non-scaled" data.
The first thing that's a bit odd about that is that it's first built in the same function that it's used in on the first iteration. Then it's also changed at each step, which invalidates the name a bit.

I think I prefer the approach in D9678 which is more aligned with what Campbell originally suggested. Regardless of which one we end up going with, thanks for working on this.

@Hans Goudey (HooglyBoogly) I understand. The thing with the array being built in the same function in which it's being used does seem a bit inelegant and admittedly, it is completely different to how it was originally written. Thanks for taking a look either way :)

Would it be possible for someone to review this please? The bug is still there in 2.93 and I badly need that fix. Thanks

Since it's planned to replace this code with the implementation used by the curve to mesh node, I think such a feature should be implemented there instead. So I will close this patch.

Thanks for the contribution Dilith.

Revision Contents
Changeset List

Path

Size

source/

blender/

blenkernel/

intern/

displist.c

41 lines

Diff 31571

View Options

source/blender/blenkernel/intern/displist.c

Fix T80979: Fixed the bevel distortion issue at bendsAbandonedPublicActions