Page MenuHome

Curves: Add Custom Profile Bevel Support
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Jul 27 2020, 5:30 PM.
Tags
None
Tokens
"Love" token, awarded by gilberto_rodrigues."Love" token, awarded by jc4d."Love" token, awarded by julperado."Like" token, awarded by Arkhangels."Love" token, awarded by koko_ze."Love" token, awarded by kenziemac130."Love" token, awarded by RC12."Like" token, awarded by TheRedWaxPolice."Like" token, awarded by Rusculleda.

Details

Summary

This adds support for the same custom bevel profile widget used in the tool and modifier to
the geometry generation for curves.

Although curves can already use another curve to make the bevel geometry, this is a quicker way,
and it also changes just the profile of one bevel, so it isn't redundant. It also makes sense to
be able to use the same custom profile functionality wherever there is bevel functionality.



This is useful expecially for text and 2D curves with extrusion, as it works much better than a weld & bevel modifier combination.
It can also be useful for adding quick detail to pipe-like objects.

The curve's bevel subpanel is also clarified with a "mode" option.

Diff Detail

Repository
rB Blender
Branch
curve-bevel-custom-profile (branched from master)
Build Status
Buildable 9274
Build 9274: arc lint + arc unit

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Jul 27 2020, 5:30 PM
Hans Goudey (HooglyBoogly) created this revision.
Hans Goudey (HooglyBoogly) planned changes to this revision.Jul 27 2020, 5:32 PM

Still one small fix to make with first / last vertex duplication

  • Fix off by one error in bevel segments creation

Generally looks good, comments inline.

source/blender/blenkernel/intern/curve.c
1734–1735

Use const for values that don't change.

1734–1735

This commit is part refactor changing else if / return...

I'm not so keen on this as it means we can't easily add cleanup/finalization functions at the end of existing code, without adding goto's or switching back to else-if blocks (causes noisy changes).

In this case I think it makes more sense to split these into functions, then the logic to run them reads much better:

Something like this:

if (cu->bevel_mode == CU_BEV_MODE_OBJECT) {
  curve_bevel_make_with_object(ob, disp);
}
else if (!(use_extrude || use_bevel)) {
  /* pass */
}
else if (!use_bevel) {
  curve_bevel_make_with_round(ob, disp);
}
else if ((cu->bevel_mode != CU_BEV_MODE_CUSTOM) && build_full && !use_extrude) {
  curve_bevel_make_with_curve_profile(ob, disp);
}
else {
  /* The general case for nonzero extrusion or an incomplete loop. */
  curve_bevel_make_extrusion(ob, disp);
}

Since this function is growing and getting more involved, I think it's reasonable to split this into curve_bevel.c too, as curve.c is quite large and it seems this is reasonable to isolate.

It'd be better to split out from this change though.

1738–1740

Term build here is quite generic.

Maybe use_fill_(front/back/full) ?

1743–1744

These will be small enough to use stack memory (alloca).

1750–1755

I don't think this is thread safe, as this runs from the main object update function which runs from threads, and objects can share object data.

It looks like it could be removed.

source/blender/makesrna/intern/rna_curve.c
1572–1576

Regarding terminology, I'm not sure about using "Custom", this made some sense for mesh bevel since it's the only way to customize.

For curve-objects curve objects are just as custom as curve profiles. Suggest renaming CU_BEV_MODE_CUSTOM to CU_BEV_MODE_CURVE_PROFILE, as it's then clear what's being used.
If this is too cramped for the UI, we could use the term "Profile" instead of the full "Curve Profile".

Besides custom seeming a too generic, I don't have a strong opinions on this so check with others on the UI team.

Campbell Barton (campbellbarton) requested changes to this revision.Jul 29 2020, 3:41 AM
This revision now requires changes to proceed.Jul 29 2020, 3:41 AM
Hans Goudey (HooglyBoogly) marked 5 inline comments as done.
  • Merge branch 'curve-bevel-cleanup' into curve-bevel-custom-profile
  • Use stack allocations for bevel quarters
  • Rename Custom -> Profile
source/blender/blenkernel/intern/curve.c
1734–1735

I initially did the refactor because working a few levels of indentation deep is just annoying.

But you're right, it shouldn't be part of this patch. So I split it off, along with a bit more cleanup: D8425

1750–1755

Right, that makes sense. The issue I had is that the segments table (the curveprofile evauation) is cleared when the file is saved, so the segment table is empty. Maybe just changing that is the thing to do, but it's runtime data. Maybe in the depsgraph there's a way to run something once per object data?

source/blender/makesrna/intern/rna_curve.c
1572–1576

I agree! I didn't think of anything at the time, but I think exposing "Profile" for the UI is a good solution.

Trying to create a diff from the cleanup patch

Sync with master, no functional changes

Campbell Barton (campbellbarton) requested changes to this revision.Jul 30 2020, 7:32 AM
Campbell Barton (campbellbarton) added inline comments.
source/blender/blenkernel/intern/curve.c
1750–1755

Off hand I'm not sure of a good solution. Best double check this is actually a threading issue, although I assume it is.

It could run on file-read, I'm not sure of a better option, besides adding a lock around this code.

source/blender/blenkernel/intern/curve_bevel.c
108–109

#include "BLI_alloca.h" is needed (for MSVC).

115

Some of these changes don't seem related to curve profile, they could be committed separately explaining what the changes are for (what the old code was doing wrong).

This revision now requires changes to proceed.Jul 30 2020, 7:32 AM
source/blender/blenkernel/intern/curve.c
1750–1755

It could work to put this in CurveCache, so each object gets it's own run-time curve profile data.

  • Merge branch 'master' into curve-bevel-custom-profile
Hans Goudey (HooglyBoogly) marked an inline comment as done.
  • Add BLI_alloca.h include

I also didn't notice @Campbell Barton (campbellbarton) already updated this to master earlier, oops!

source/blender/blenkernel/intern/curve.c
1750–1755

I messed around with a file with a bunch of objects using the same curve data. Everything worked fine until I did undo, then I had a double free from a separate thread. So definitely a memory issue.

So about solutions:

  1. Save segments table in file: Each point is 40 bytes. The number of segments is usually low, but in the worst case this could add 4000 bytes per bevel modifier with maxed resolution (and 1280 bytes per curve data with max resolution).
  2. Running initialize on file read: I'm not sure if there is precedent for this?
  3. Storing a runtime copy in CurveCache: I can look into this more. Looking at it briefly it appeared that the areas that deal with CurveCache are still threaded?
source/blender/blenkernel/intern/curve_bevel.c
115

I think it looks like that because the general case is used for the curve profile bevel even when there is no extrusion (Which would have previously been just curve_bevel_make_full_circle).

Since the logic before was dependent on setting the angle correctly rather than choosing the right indices to pull the location from, I'm not sure it makes sense to split off changes from this patch.

  • Initialize CurveProfile on file load

This solves the thread safety problems. It would also be just as easy to save the evaluated
segments in the file, it's just a matter of where to make the tradeoff. I'm not sure the CurveCache
idea would work though.

Maybe @Brecht Van Lommel (brecht) has an opinion about this tradeoff of a small increase in file size vs a small
increase in loading time? More details are just above or here: https://developer.blender.org/D8402#inline-67548

  • Check curve bevel type properly
  • Small UI tweaks
  • Fix RNA identifier
  • Merge master
  • Small UI fix
  • Move versioning to proper block

@Campbell Barton (campbellbarton) Everything you mentioned should be handled here. If you don't mind giving
it a quick look it could go in to 2.91.

Juan (jc4d) added a subscriber: Juan (jc4d).
Campbell Barton (campbellbarton) added inline comments.
release/scripts/startup/bl_ui/properties_data_curve.py
202–230

Having the mapping bevel factor and mapping between the bevel profile & type selector seems a bit odd (as the mapping factors aren't related to the bevel mode).

Not sure of best solution, since having the profile above will mean they move a lot when changing bevel-mode to profile.

source/blender/makesrna/intern/rna_curve.c
1570–1571

missing space on wrapped line.

This revision is now accepted and ready to land.Sep 16 2020, 2:25 PM
This revision was automatically updated to reflect the committed changes.
Hans Goudey (HooglyBoogly) marked an inline comment as done.
Hans Goudey (HooglyBoogly) marked 4 inline comments as done.Sep 16 2020, 5:40 PM
Hans Goudey (HooglyBoogly) added inline comments.
release/scripts/startup/bl_ui/properties_data_curve.py
202–230

Yeah, I agree this is a bit of an issue. Thing is, this is a problem in master too. These properties apply even when the bevel depth is 0, because they control the mapping for only extrusions too. I created D8910 to address this.