This patch addresses the issue described in T80979. Prior to this patch, bevel on curves could create distorted geometry as the same profile was applied to each segment. This patch addresses the issue by taking into account the angle of the underlying curve at each given point prior to bevel rotation.
Details
Details
- Reviewers
Hans Goudey (HooglyBoogly) Campbell Barton (campbellbarton) Germano Cavalcante (mano-wii) - Group Reviewers
Modeling - Maniphest Tasks
- T80979: Account for curvature in curve to mesh node
Diff Detail
Diff Detail
- Repository
- rB Blender
- Branch
- T80979-Bevel (branched from master)
- Build Status
Buildable 11601 Build 11601: arc lint + arc unit
Event Timeline
There are a very large number of changes, so older changes are hidden. Show Older Changes
Comment Actions
Nice, thx. It is quite challenging for a good first issue.
If I'm not mistaking, the suggestion in the report was to have an option (in the interface) for this to work differently.
I didn't find anything in the patch showing this option.I believe that this option would be useful to not modify old files.
No problem. Any pointers on this where to put in a checkbox or something similar? Do you have any pointers to code that might help? Should this be in sync with UI team?
I do not know what exactly causes the problems in z direction.
Has this problem been fixed yet? Otherwise, you have to mark the patch as Plan Changes.
Yeah, I think so. At least it looks good to me.
I have updated the code as requested by Hans. Hopefully this is okay. @Hans Goudey (HooglyBoogly) Oone comment I did not implement as you suggest because this requires a larger refactor of the rotateBevelPiece function and the calling function. Anyway, if this is a hard requirement and if you don't agree with my comments in code, I will provide an implementation.
| source/blender/blenkernel/intern/displist.c | ||
|---|---|---|
| 1332 |
Is this a hard requirement? Since this would require another variable which might be confusing or a larger refactor of the calling function. I tried my best to comment on it. | |
Comment Actions
I would suggest exposing the option in the curve tab -> geometry panel -> bevel sub-panel. Since this only applies when generating the bevel geometry, I think it makes sense there. It could be a checkbox.
Coming up with a good name for the property might take a bit of thought. It's hard to come up with something descriptive and short. "Angle Based Radius" is the best idea I have right now, hopefully we can come up with something better though.
Comment Actions
UI checkbox for angle based radius included. Please see the screenshot
By default the new behavior is switched off. Btw., how about naming it "undistort bevelled geometry"?
| source/blender/blenkernel/intern/displist.c | ||
|---|---|---|
| 1306 | I am finding these conditions a little confusing. if (cu->flag & CU_3D) {
if (cu->flag & CU_ANGLE_BASED_RADIUS) {
if (nbevp == NULL || nbevp == bevp - 1) {
(...)
}
else {
(...)
}
}
else {
(...)
}
}
else {
(...)
} | |
| source/blender/blenkernel/intern/displist.c | ||
|---|---|---|
| 1306 | Hi Germano, | |
Comment Actions
Thanks for your work on this, it's getting there!
Most of my comments are about making the code more readable and less confusing, not really functionality. Sorry if you're tiring of that, but it's important since this code is already quite confusing and in need of a refactor.
The biggest confusing part in my opinion is that there are three cases, and they're mixed up in two different functions--
- nbevp == NULL which actually seems to be the most common case, used for every item but the first.
- nbevp == bevp - 1 seems to be the last bevel piece.
- nbevp == bevp + 1 looks like it's for the first bevel piece. This condition is currently unstated in the code, it's just handled in the else as far as I can tell.
My idea is that rewriting this piece of code (the code inside if (cu->flag & CU_ANGLE_BASED_RADIUS) { to align better with these conditions would make it more understandable.
That way precalcBevelPieceData wouldn't need two different cases, which would be preferable since it's quite nice to avoid a low level function like that doing two completely different things.
In general I'd say don't worry too much about how many lines of code you're adding, what's more important IMO is the clarity and simplicity of the different conditions.
I would also push you to use more descriptive variable names than vec, d, and t.
| source/blender/blenkernel/intern/displist.c | ||
|---|---|---|
| 1315 | No need for the double parentheses. | |
| 1315–1317 | Agh, it's not really your fault, but these set of conditions and lines are really confusing. It's a combination of the terse variable names and the large set of requirements and uses of this function. First is the fact that it looks like nbevp can be either the next bevel point or the previous bevel point, that is so weird. I'll make some suggestions in my main comment. One thing that's proven very helpful in other situations is to use asserts (BLI_assert) to make sure assumptions about the situation. If there as some assumptions that are written in comments or unsaid, they could be tested with an assert. | |
| 1324 | snake_case is the convention and preferred here. | |
| 1332 | Some comment on the different cases here would be helpful. What does the first if statement mean, what's in this else | |
| source/blender/makesdna/DNA_curve_types.h | ||
| 360 ↗ | (On Diff #32307) | This language isn't quite right. The "bevel object" is actually this thing:  It just repeats the name of the flag. This comment should be helpful! Look at it from the perspective of someone seeing this for the first time, what's the fastest way to make this clear? Maybe at more of an implementation level than Also, capital and period. |
| source/blender/makesrna/intern/rna_curve.c | ||
| 1790 ↗ | (On Diff #32307) | Similar to the comment above the flag. You could even use the same description in both places, it doesn't hurt to repeat it. How about something like: Adjust the bevel based on the angle between segments for a consistently sized spline ("Radius" isn't quite right, since it might not be a circle, and it's only adjusted on one of the axes) |
Comment Actions
I'm not sure whether it would be better on or off by default, but that's an easy decision to make later. The issue I have with your suggestion is that "bevel" and "geometry" are redundant in the name as written in the UI since it's exposed in the "Bevel" sub-panel of the "Geometry" panel.
Comment Actions
@Hans Goudey (HooglyBoogly) I tried to be more clear what the code is doing and tried to implement your suggested improvements. To me it is not at all annoying, it is a great learning experience. Thank your for your great help and reviews. I hope I was able to do better now.
Btw. Please ignore comment on line 1329. This went somehow in this post.
| source/blender/blenkernel/intern/displist.c | ||
|---|---|---|
| 1306 |
| |
| 1315–1317 | I tried to follow more precisely the structure as rotateBevelPiece is called. I know, it is quite confusing. I hope that it has become now more clear what the code is doing. | |
| 1332 | I had to introduce a single_bevp boolean variable since otherwise it is impossible to know, if there is only one bevp. And this has implication on what data I am allowed to access by pointer. | |
| source/blender/makesrna/intern/rna_curve.c | ||
| 1790 ↗ | (On Diff #32307) | How about Consistent Thickness? |
Comment Actions
Hey @Roman (roman13), it might be the easiest way when you build blender yourself? Here https://wiki.blender.org/wiki/Tools/CodeReview you can find information how to apply the patch and here https://wiki.blender.org/wiki/Building_Blender how to build blender.
Unfortunately, I have no idea if and when this patch is going to be merge into master.
Comment Actions
Sorry for the delay @Janusch Patas (patjan).
I'm still not totally happy with the implicit dependency on the last point's data, even if it's handled properly and everything. But I think you've done quite well making this clear given the context of the confusing area of code.
I think this should remain off by default, just because it can have some not-so-nice consequences in some situations. Here the next points handles are parallel to the direction of the last segment.
I will give some time for @Campbell Barton (campbellbarton) or @Germano Cavalcante (mano-wii) to look at this if they want to, but I'll commit relatively soon if not.
Comment Actions
@Hans Goudey (HooglyBoogly) : Thx. I know but this is the consequence of the approach. Future improvements might by rounded corners or similar geometric improvements.
Comment Actions
Hi,
@Janusch Patas (patjan) Thank you for this awesome patch, long time waiting for it.
This behavior on 3d beveled curves is similar to what the curve modifier dues to straight geometry when its deformed close to a control point tagged as "vector" or "Free", I'm not a programmer so I'm not sure if this same solution could be used to fix the curve modifier behavior... is it possible? or is this another complete different problem?
Here an example of what I mean:
Comment Actions
In the process of committing I made some cleanups to this patch, the inline comments and others. I updated this patch with those cleanups, I hope you don't mind.
However, during testing I also realized that this patch means that the radius set for points besides the first will be ignored.
In the context of T86243, which has been developing recently, I'm honestly not sure about moving forward with this approach. In that task I propose a different way to structure the curve geometry generation code.
Every new somewhat hacky feature makes this code harder to restructure, which is arguably better in the long run. Anyway, I would like some feedback from other developers in the modeling module about this.
| source/blender/blenkernel/intern/displist.c | ||
|---|---|---|
| 1293–1294 | These shouldn't be vectors, just a pointer would work here. | |
| 1300 | This is used as a boolean, it should not be a float. | |
| 1332 | Please leave cleanups out of patches for functional changes. | |
Comment Actions
Hmm, right now I am not sure what your expectations are. The radius seems useless to me or I am just not able to trigger it.
Comment Actions
Sorry for the delay.
I haven't tested the code yet, but it looks harmless since the new algorithm only runs when "use_consistent_thickness" is enabled.
To understand the code, I focused a lot on the comments, maybe that part can improve a little more.
In my opinion, the original code is already somewhat confusing. I think it deserve a refactor/cleanup. (That would be for another patch).
But in general the patch looks ok. Just a few observations.
| source/blender/blenkernel/intern/displist.c | ||
|---|---|---|
| 1311 | These commented comments are not helping much. /**
* Optionally #scale_to_intersect would be calculated as follows:
* \code{.c}
* float offset_plane_origin = dot_v3v3(bevp->dir, bevp->vec);
* float scale_to_intersect = -(dot_v3v3(bevp->dir, last_data_point) - offset_plane_origin) /
* offset_plane_origin;
* \endcode
*/But the result is the same so I don't see much need for this. | |
| 1317 | This part of the comment seems to be redundant here since this code is only called if nbevp == (bevp + 1). | |
| 1342 | This is the same as the "common case". Could the code be deduplicated? | |
| 1352 | Initializing and assigning at the same time? | |
Comment Actions
The radius set in the UI for each point should have an effect on top of the scale result from this patch.
| source/blender/blenkernel/intern/displist.c | ||
|---|---|---|
| 1352 | Oops, that was me. Should just cut everything before (and including) the equals signs. | |
Comment Actions
@Hans Goudey (HooglyBoogly): I still don't know what you mean by radius. There is a check box called radius but I can't see an effect when check marked even not on master.
Or do you mean the depth slider (which says "Radius of the bevel geometry" when hoovering over it and which seems to work just fine")?
| source/blender/blenkernel/intern/displist.c | ||
|---|---|---|
| 1324 | I think that here we can use the shell_v3v3_normalized_to_dist. | |
Comment Actions
@Germano Cavalcante (mano-wii): Thanks. I have substituted the function as suggested.
@Hans Goudey (HooglyBoogly): Radius factor applies now to every bevel piece.
| source/blender/makesrna/intern/rna_curve.c | ||
|---|---|---|
| 1792 ↗ | (On Diff #35323) | Note that this type of setting is called "Even" elsewhere, e.g. "Even Thickness" for solidify modifier, "Offset Even" for inset/shrink-fatten. use_even_thickness is consistent with other similar features. |
Comment Actions
Is this feature still relevant? The inital commit is almost 6 months old and little drive towards acceptance. So, if there are plan to integrate it, I like to update the patch. Otherwise it would be best to abandon it.
Comment Actions
Hi @Janusch Patas (patjan), sorry I haven't been more proactive with communication. Honestly in the meantime a fair amount has changed with respect to the curve code. Mainly the geometry nodes support for curves has been an opportunity to rethink a lot of it, and now I have plans to replace much of this code.
So, a similar feature would be welcome in the curve to mesh node (and probably a fair amount simpler to implement), but I would rather not increase the complexity of this code in displist.c at this point. That's my perspective anyway.
Comment Actions
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 anyway.
Comment Actions
I am really sad to see how this patch hasn't been ported to geo nodes yet, It solves a very annoying behavior which has been in blender since ever and somehow it has been dropped.
Thanks to all developers who worked on it and I hope it will finally be implemented