Page MenuHome

GSoC 2019: Bevel Custom Profiles
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Aug 18 2019, 6:16 AM.
Tokens
"Like" token, awarded by TheRedWaxPolice."Yellow Medal" token, awarded by LahceneB."100" token, awarded by Frozen_Death_Knight."Love" token, awarded by brilliant_ape."Love" token, awarded by bnzs."Love" token, awarded by ace_dragon."Like" token, awarded by ThinkingPolygons.

Details

Summary

Here are the major changes from the bevel profiles branch:

Profile Widget

  • The profile widget is derived from the curvemapping widget, but the path is built to be a profile instead of a mapping between X and Y.
  • Profile sampling methods: Taking the high resolution path and getting sampled point positions. More curved areas get more samples, and points can be sharp as well. There's also an "Even Length Sampling" option.
  • The interaction methods are changed for profiles, and hotkeys are added.
  • The widget should be extenable for other situations where splines need to be edited in the UI.
  • RNA API for the widget included.

Bevel Custom Profile

  • Added custom profile bevel cases, removed existing assumptions and special cases.
  • New "pipe case" for parallel edges which reuses profile positions.
  • Profile orientation regularization: Method to ensure that the side of the start of the profile is consistent along groups of edges.
  • Presets: Moldings and steps, etc...

Bevel cutoff intersection method

  • Method chops off each profile at the boundary of each vertex mesh.
  • Leaves room for Boolean method in the future.

Bevel tool and modifier UI

  • Including the new features in the UI
  • With the new features, the UI is even larger, so updates to streamline and unify both interfaces make sense at this stage

Bevel operator code cleanup and documentation

  • Lots of comments on existing bevel code where I worked, some changed variable names, and some other refactoring.
  • The code should be more readable, which has already helped me, so hopefully it will help someone else in the future.

Still to do

T68330 lists the tasks and bugs for the project. Most listed there are planned for continued work after the merge, but the first section would probably be completed beforehand depending on feedback.

Diff Detail

Repository
rB Blender

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Howard Trickey (howardt) requested changes to this revision.Sep 22 2019, 11:01 PM

I made a first pass at reviewing this. Several minor things to change per my comments.
I think this code is about ready to merge, though we should wait until after the 2.81 stabilizing branch has been made (supposed to be Oct 10).

source/blender/blenkernel/intern/profile_widget.c
94 ↗(On Diff #18231)

Requiress -> Requires
(this typo occurs a number of times in the file, so please fix them all)

264 ↗(On Diff #18231)

When n == 2 (as when segments == 1), we get n_steps_y == 0, so this division produces a NaN, and the profile widget display looks funky. (The actual built profile is a ramp, which I guess is fine for this case.) Probably should special case the n == 2 case at the beginning of this function.

282 ↗(On Diff #18231)

It would be nice to allow a dynamic number of points here too, like you do with steps.

305 ↗(On Diff #18231)

I won't require this, but you could consider a more data-driven way of doing this function: putting the points in arrays (with another array for flags), and then not using a switch here but rather just pick the right arrays to use.

544 ↗(On Diff #18231)

I haven't tried to understand this function in detail. But I did notice that if I use the "Support Loops" preset, this filling in of sample points seems a little strange: it seems to, at some segment numbers, add one extra point each into the flat areas on either side of the curved part of the support loops. And strangely, on one side it adds that extra point about half-way along the segment while on the other it seems to add it just a little distance away from the curved part. Not sure if that means there's a slight bug in this code.

source/blender/bmesh/intern/bmesh_opdefines.c
1760

While this appears to work for your purposes, it is probably better to emulate what is done above for offset_type: that is, making the third arg be {(int)BMO_OP_SLOT_SUBTYPE_INT_ENUM}, bmo_enum_bevel_vmesh_method_type}, and define the enum type above this section. I believe the benefit of doing this is so that the API call from Python can use strings for the values instead of ints. Of course, maybe this is pointless since you can't call this from Python anyway for custom profiles until some mechanism would get added to be able to pass the prwdgt struct pointer.

source/blender/bmesh/tools/bmesh_bevel.c
146

Thanks for these renames and others like it in code throughout this file. I'm sure they make it easier for future developers to understand the code.

1666

pro->start to pro->end

1745

Could use zero_v3(p);

3093

What if there is no second best -- that is, if there is only one other beveled edge at this bevvert so that there is only one way out? Can't we pick that anyway then? I think this only comes up if the best_dot is also 0 (at 90 degrees from incoming). I happened to run across that case, I think, when I took a cube and selected two adjacent edges on top, and then from the other end of those two, selected the edge that continued down to the bottom.

source/blender/editors/interface/interface_draw.c
2116

If you think this code is likely stable and unlikely to need much further debugging, consider removing the debugging code now.

This revision now requires changes to proceed.Sep 22 2019, 11:01 PM
Hans Goudey (HooglyBoogly) marked 11 inline comments as done.Sep 24 2019, 2:17 AM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenkernel/intern/profile_widget.c
305 ↗(On Diff #18231)

I considered doing something like that earlier on, but it just seemed a little more complicated than what was necessary. I think the biggest downside of this approach is the length of this function, so I've created a helper to make this whole section shorter.

Also, eventually it would be great to have these presets just load actual actual curve objects instead (maybe some integration with the "asset manager" along with other presets), so I think keeping this section simple in the meantime is desirable.

544 ↗(On Diff #18231)

This is a particularly complicated area of the code. The big idea is it's assigning a number of samples to each of the widget edges and then sampling from those edges afterward.

Until you get more samples than control points, each sample will lie on a control point. The samples are also drawn in the widget, but they were drawn with the same color as the control points, so it was hard to tell where they are. So I changed the sample color to a darker one. This should help at least to understand what the sampling does.

I'm not running into the issue you described. Maybe it was a problem with the preset.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.
  • Responded / fixed comments from review

(Using the right diff this time)

Adding the Profile Widget instead of expanding the CurveMapping Widget

I wanted to write down the justification for having a new profile widget, because I think that's one thing reviewers could wonder about. One thing I considered at the beginning if the project was adding this functionality to the CurveMapping widget, but I decided not to for these reasons:

  1. Better tooltips The use cases of the two widgets are quite different. Different RNA for the two widgets means the tooltips can be specific to the function.
  2. Each widget is simpler with fewer cases It will be much easier in the future to add features and fix problems with the two widgets if the code for each is simpler. It will be easier to make the profile widget mirror a curve object in the future because of this.
  3. Completely different evaluation paradigm For the profile widget, the evaluation / sampling code matters much more, because it directly controls the final mesh, whereas the evaluation of a CurveMapping widgey is a behind the scenes operation. But more importantly, evaluating the profile widget returns a pair of coordinates, not just a Y value.
  4. Overhangs Because the profile isn't a mapping from X to Y, it has to be drawn and evaluated differently. Adding this complication to the CurveMapping widget would unecessarily increase its complexity.
  5. New potential use cases for the Profile Widet The profile widget could be used to bring more power to any generated mesh (possibly curves, wireframes..) And it's accessible from Python so any addon can use it.
source/blender/bmesh/tools/bmesh_bevel.c
3093

Good point, I didn't catch this! I solved it by adding a special case at the beginning, which should speed it up too anyway.

It also looks like the automated bevel test is failing: https://builder.blender.org/admin/#/builders/16/builds/250/steps/4/logs/stdio

I'm looking into this so I see what I find. My guess is I inadvertently made a change that affects the existing behavior.

After looking through many of the bevel test results manually, I haven't been able to visually confirm many of the failed operations. The test reports that there is a "Vertex Coordinate Mismatch," but all the vertices I've checked are in the correct locations, and their indexing has been the same as well.

For others the difference doesn't quite make sense. It would be great if someone with more familiar with the automated bevel tests took a look at this; it's not clear to me what the issue is at this point.

After looking through many of the bevel test results manually, I haven't been able to visually confirm many of the failed operations. The test reports that there is a "Vertex Coordinate Mismatch," but all the vertices I've checked are in the correct locations, and their indexing has been the same as well.
For others the difference doesn't quite make sense. It would be great if someone with more familiar with the automated bevel tests took a look at this; it's not clear to me what the issue is at this point.

I'll look at this soon. If things are visually ok, then it could be something very similar but not exactly the same as the old position. With the loop slide option, the optimization phase is numeric and would require exactly the same inputs to get the same outputs (and I'm not even sure if that is sufficient -- there may be some randomization in that routine). Ideally I should change the mesh comparison function to have a parameter that is more tolerant of slight changes in vertex positions and (even better) reorderings of vertices. But in the absence of that, we can just overwrite the old results with new results, once we are sure they are all ok.

I would like to merge this into master now that we are in the early 2.82 stages there. Any objections, Campbell?

Initial code review (first pass),

Didn't review bmesh_bevel.c in detail (left this one for Howard).

source/blender/blenkernel/intern/profile_widget.c
46–49 ↗(On Diff #18988)

MEM_SAFE_FREE(...) does this, avoiding 3x repetitions in each case.

72–80 ↗(On Diff #18988)

No need for NULL checks here.

107–116 ↗(On Diff #18988)

There is no need to loop here.

index = point - path;

Bounds check, asserting if the index is out of range, then remove the index.


Although this calls into question if it's better for the API to take in an index or a pointer.

Whatever the case, the loop is redundant.

127 ↗(On Diff #18988)

Naming here seems a bit off.

Could call this BKE_profilewidget_remove_by_flag, instead of BKE_profilewidget_remove.

304 ↗(On Diff #18988)

Wouldn't use inline unless this is known to be a performance bottleneck it's normally best to let the compiler choose.

304 ↗(On Diff #18988)

Although it's fairly obvious, this is more of an initialization function then a setter.

Suggest to call point_init.

556 ↗(On Diff #18988)

If you order sizeof(...) first, you don't need the size_t cast.

619 ↗(On Diff #18988)

Typo.

787 ↗(On Diff #18988)

More conventional in Blender's code to use #_update instead of #_changed.

Since it's not obvious is changed is a test or an action you apply to the data.


Realize this follows BKE_curvemapping_changed (think both could be renamed to update).

source/blender/makesdna/DNA_profilewidget_types.h
44–49 ↗(On Diff #18988)

Since it's noted as a TODO to support manually editing bezier handles I think it would be better to split out the vector type from the flag.

Otherwise we end up with multiple handle types being mixed in a flag where only one ends up being used - it can work but makes flag usage hard to follow.

This could be a char h1, h2; matching BezTriple - even if you start by only supporting vector & auto.

source/blender/makesrna/intern/rna_modifier.c
3747–3749

This seems needlessly cryptic, why not call custom_profile ? (DNA too for that matter).

Campbell Barton (campbellbarton) requested changes to this revision.EditedOct 14 2019, 6:31 AM

General Review

Naming
  • Profile is an overloaded term which can also relate to performance, this feels like a spesific functionality, given a generic prefix.

    While it's probably best to discuss this in chat, think BKE_curveprofile_* / CurveProfile would be a better fit, since this is similar to BKE_curvemapping_* / CurveMapping.
  • Including widget in the struct name is confusing.

    Currently we differentiate the widget from the data that's being edited - which I think is reasonable to continue.

If renaming is a hassle, I don't mind helping to automate it and update the patch.

ToolSetting Profile

Including the profile in the scene's tool-settings seems like it's something we could avoid since it's only used at run-time, by the operator.

It would be strange if we added 2x other tools that use profiles and they also need entries in the tool settings, or have them all reuse the bevel profile data.

Saving an image is an example of an operator that uses it's own allocated struct (see ImageSaveOptions use in image_ops.c).

We can always remove this and use somethings stored in the operator, we're not locked into keeping & versioning this data.

While I don't consider this a blocker, I'd like to figure out how this might work - ideally.

If there is no strong reason to store this in the tool settings it's probably not much work to move it to the operator.

This revision now requires changes to proceed.Oct 14 2019, 6:31 AM
source/blender/blenkernel/BKE_profile_widget.h
20–21 ↗(On Diff #18988)

Convention is to use __UPPERCASE_NAME_H__.

Hans Goudey (HooglyBoogly) marked 11 inline comments as done.Oct 14 2019, 10:49 PM
Hans Goudey (HooglyBoogly) added inline comments.
source/blender/blenkernel/intern/profile_widget.c
556 ↗(On Diff #18988)

My editor (QT creator) warns me about it both ways, but it tends to warn about way too much anyway, so I'll change it.

Hans Goudey (HooglyBoogly) marked 2 inline comments as done.Oct 15 2019, 7:13 PM
  • Renamed ProfileWidget -> ProfileCurve. I used ProfileCurve instead of CurveProfile to give emphasis to the word "Profile" rather than "Curve." Although this doesn't mirror CurveMapping quite as well, I think it's a better name. It keeps the data separate from the UI, and it's clear that it's not a profiling tool.
  • ProfilePoint: Store handle type for left and right handle separately from the flag.
  • Other renamed functions

For the ToolSettings comment, I also agree it would preferable to store the ProfileCurve specifically for the operator. I haven't changed that yet but I'll look into it.

Updated against latest master

Commented out repface to avoid warning. (Issue should be solved, but low priority and needs input)

  • Cleanup: use conventional variable names for length members.
  • Remove DNA_curveprofile_types.h from scene header.
  • Cleanup: rename PROFILE -> CURVEPROFILE
Campbell Barton (campbellbarton) requested changes to this revision.Nov 7 2019, 8:16 AM
Campbell Barton (campbellbarton) added inline comments.
release/scripts/startup/bl_ui/properties_data_modifier.py
185–187

Initializing data in drawing code means it wont run reliably (if the UI hasn't yet displayed).

Data should be initialized by RNA update functions or versioning code.

source/blender/makesdna/DNA_scene_types.h
1522

Based on discussion, this should be documented as temporary, which should be replaced by a preset system. Suggest naming custom_bevel_profile_preset

source/blender/makesrna/intern/rna_curveprofile.c
253

use use_ prefix.

258

use use_ prefix.

287–292

This shouldn't be needed, Python can call len(profile.segments)

This revision now requires changes to proceed.Nov 7 2019, 8:16 AM
Hans Goudey (HooglyBoogly) marked 4 inline comments as done.

Addressed comments from review

Campbell Barton (campbellbarton) added inline comments.
release/scripts/startup/bl_ui/properties_data_modifier.py
165

Use md.miter_inner in {'MITER_PATCH', 'MITER_ARC'}

source/blender/blenkernel/BKE_curveprofile.h
21–22

Update for new file name.

Addressed final comments

This revision was not accepted when it landed; it landed in state Needs Review.Nov 20 2019, 10:27 PM
This revision was automatically updated to reflect the committed changes.