Details

Reviewers

Howard Trickey (howardt)
Campbell Barton (campbellbarton)

Commits

rBba1e9ae4733a: Bevel: Custom Profile and CurveProfile Widget

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

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

Merged lastest master

Kenn Nyström (Frozen_Death_Knight) awarded a token.Sep 19 2019, 1:45 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	Requiress -> Requires (this typo occurs a number of times in the file, so please fix them all)
264	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	It would be nice to allow a dynamic number of points here too, like you do with steps.
305	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	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

Howard Trickey (howardt) added a reviewer: Campbell Barton (campbellbarton).Sep 23 2019, 2:51 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	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	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.

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:

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.
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.
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.
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.
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.

In D5516#135587, @Hans Goudey (HooglyBoogly) wrote:

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?

Sync D5516 /w master

Harbormaster completed remote builds in B5347: Diff 18987.Oct 14 2019, 4:28 AM

Clang-format

Harbormaster completed remote builds in B5348: Diff 18988.Oct 14 2019, 4:54 AM

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
47–50	`MEM_SAFE_FREE(...)` does this, avoiding 3x repetitions in each case.
73–81	No need for NULL checks here.
108–117	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.
128	Naming here seems a bit off. Could call this `BKE_profilewidget_remove_by_flag`, instead of `BKE_profilewidget_remove`.
305	Wouldn't use `inline` unless this is known to be a performance bottleneck it's normally best to let the compiler choose.
305	Although it's fairly obvious, this is more of an initialization function then a setter. Suggest to call `point_init`.
557	If you order `sizeof(...)` first, you don't need the `size_t` cast.
620	Typo.
788	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
45–50	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
3735–3737	This seems needlessly cryptic, why not call `custom_profile` ? (DNA too for that matter).

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

Campbell Barton (campbellbarton) added inline comments.Oct 14 2019, 6:38 AM

source/blender/blenkernel/BKE_profile_widget.h
21–22	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
557	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.

Lahcene Belbachir (LahceneB) awarded a token.Oct 20 2019, 1:52 AM

Lahcene Belbachir (LahceneB) added a subscriber: Lahcene Belbachir (LahceneB).

TheRedWaxPolice (TheRedWaxPolice) awarded a token.Nov 2 2019, 3:24 PM

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.

Harbormaster completed remote builds in B5592: Diff 19456.Nov 7 2019, 7:55 AM

Remove DNA_curveprofile_types.h from scene header.

Harbormaster completed remote builds in B5593: Diff 19457.Nov 7 2019, 8:03 AM

Cleanup: rename PROFILE -> CURVEPROFILE

Harbormaster completed remote builds in B5594: Diff 19458.Nov 7 2019, 8:10 AM

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
286–291 ↗	(On Diff #19439)	This shouldn't be needed, Python can call `len(profile.segments)`
252 ↗	(On Diff #19457)	use `use_` prefix.
257 ↗	(On Diff #19457)	use `use_` prefix.

This revision now requires changes to proceed.Nov 7 2019, 8:16 AM

Addressed comments from review

Hans Goudey (HooglyBoogly) mentioned this in T71329: Asymmetrical bevel output from symmetrical input..Nov 13 2019, 10:55 PM

Campbell Barton (campbellbarton) accepted this revision.Nov 19 2019, 4:31 AM

Campbell Barton (campbellbarton) added inline comments.

release/scripts/startup/bl_ui/properties_data_modifier.py
164	Use `md.miter_inner in {'MITER_PATCH', 'MITER_ARC'}`
source/blender/blenkernel/BKE_curveprofile.h
20–21 ↗	(On Diff #19484)	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

Closed by commit rBba1e9ae4733a: Bevel: Custom Profile and CurveProfile Widget (authored by Hans Goudey (HooglyBoogly)). · Explain Why

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rBba1e9ae4733a: Bevel: Custom Profile and CurveProfile Widget.

Pablo Vazquez (pablovazquez) added a subscriber: Pablo Vazquez (pablovazquez).Nov 25 2019, 5:28 PM

GSoC 2019: Bevel Custom ProfilesClosedPublicActions

Details

Profile Widget

Bevel Custom Profile

Bevel cutoff intersection method

Bevel tool and modifier UI

Bevel operator code cleanup and documentation

Still to do

Diff Detail

Event Timeline

Adding the Profile Widget instead of expanding the CurveMapping Widget

Naming

ToolSetting Profile

Revision ContentsChangeset List

Diff 18231

release/scripts/presets/keyconfig/keymap_data/blender_default.py

release/scripts/startup/bl_ui/properties_data_modifier.py

source/blender/CMakeLists.txt

source/blender/blenkernel/BKE_profile_widget.h

source/blender/blenkernel/CMakeLists.txt

source/blender/blenkernel/intern/profile_widget.c

source/blender/blenkernel/intern/scene.c

source/blender/blenloader/intern/readfile.c

source/blender/blenloader/intern/versioning_280.c

source/blender/blenloader/intern/versioning_defaults.c

source/blender/blenloader/intern/writefile.c

source/blender/bmesh/intern/bmesh_opdefines.c

source/blender/bmesh/intern/bmesh_operator_api.h

source/blender/bmesh/intern/bmesh_operators.h

source/blender/bmesh/operators/bmo_bevel.c

source/blender/bmesh/tools/bmesh_bevel.h

source/blender/bmesh/tools/bmesh_bevel.c

source/blender/editors/include/UI_interface.h

source/blender/editors/interface/interface_draw.c

source/blender/editors/interface/interface_handlers.c

source/blender/editors/interface/interface_intern.h

source/blender/editors/interface/interface_query.c

source/blender/editors/interface/interface_templates.c

source/blender/editors/interface/interface_widgets.c

source/blender/editors/mesh/editmesh_bevel.c

source/blender/makesdna/DNA_modifier_types.h

source/blender/makesdna/DNA_profilewidget_types.h

source/blender/makesdna/DNA_scene_types.h

source/blender/makesdna/intern/makesdna.c

source/blender/makesrna/RNA_access.h

source/blender/makesrna/RNA_enum_types.h

source/blender/makesrna/intern/CMakeLists.txt

source/blender/makesrna/intern/makesrna.c

source/blender/makesrna/intern/rna_internal.h

source/blender/makesrna/intern/rna_modifier.c

source/blender/makesrna/intern/rna_profile.c

source/blender/makesrna/intern/rna_scene.c

source/blender/makesrna/intern/rna_ui_api.c

source/blender/modifiers/intern/MOD_bevel.c

GSoC 2019: Bevel Custom Profiles
ClosedPublic
Actions

Revision Contents
Changeset List