Details

Reviewers

Commits

rBbaff05ad1c15: UI: Add Free Handle Types to CurveProfile Widget

Summary

Under the hood the CurveProfile widget (currently used for bevel custom profiles) has a bezier curve. In master right now though it only supports two of the bezier curve handle types, vector and auto. This patch adds support for free handles and adds all of the logic for editing them.

This is the first step to the ability to import and export curve objects in the widget.

There's some code cleanup in curveprofile.c. Movement for handles and control points is abstracted to functions there rather than happening in interface_handlers.c.

Along with that there's an "Apply Preset" button which solves a confusing issue where you apply a preset, then change the number of samples and the preset doesn't change. The button makes it clear that the preset needs to be reapplied.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) created this revision.Dec 22 2019, 8:57 PM

TheRedWaxPolice (TheRedWaxPolice) awarded a token.Dec 22 2019, 9:00 PM

Matej Junk (MJunk) awarded a token.Dec 22 2019, 9:11 PM

Erick Tukuniata (erickblender) awarded a token.Dec 22 2019, 9:37 PM

No reviewers for this beauty? 🙂

Clang format

Hans Goudey (HooglyBoogly) added reviewers: Julian Eisel (Severin), Campbell Barton (campbellbarton).Jan 14 2020, 12:25 AM

Use new handle type icons

Partial review, found some things to address, but nothing too serious.

I was a bit confused about the code in CurveProfile_buttons_layout(). It manually adds buttons for enums and such, despite them representing RNA data. In such cases our UI code should be able to get the needed info from RNA and place the buttons itself. This patch shows what I mean:

P1243 Proposed changes to D6470

1	diff --git a/source/blender/editors/interface/interface_templates.c b/source/blender/editors/interface/interface_templates.c
2	index 6cecfca95f2..9eb6038e791 100644
3	--- a/source/blender/editors/interface/interface_templates.c
4	+++ b/source/blender/editors/interface/interface_templates.c
5	@@ -4826,46 +4826,6 @@ static void CurveProfile_buttons_delete(bContext C, void cb_v, void *profile_v
6	rna_update_cb(C, cb_v, NULL);
7	}
8
9	-static void CurveProfile_buttons_setsharp(bContext C, void cb_v, void *profile_v)
10	-{
11	- CurveProfile *profile = profile_v;
12	-
13	- BKE_curveprofile_selected_handle_set(profile, HD_VECT, HD_VECT);
14	- BKE_curveprofile_update(profile, false, false);
15	-
16	- rna_update_cb(C, cb_v, NULL);
17	-}
18	-
19	-static void CurveProfile_buttons_setcurved(bContext C, void cb_v, void *profile_v)
20	-{
21	- CurveProfile *profile = profile_v;
22	-
23	- BKE_curveprofile_selected_handle_set(profile, HD_AUTO, HD_AUTO);
24	- BKE_curveprofile_update(profile, false, false);
25	-
26	- rna_update_cb(C, cb_v, NULL);
27	-}
28	-
29	-static void CurveProfile_buttons_setfree(bContext C, void cb_v, void *profile_v)
30	-{
31	- CurveProfile *profile = profile_v;
32	-
33	- BKE_curveprofile_selected_handle_set(profile, HD_FREE, HD_FREE);
34	- BKE_curveprofile_update(profile, false, false);
35	-
36	- rna_update_cb(C, cb_v, NULL);
37	-}
38	-
39	-static void CurveProfile_buttons_setaligned(bContext C, void cb_v, void *profile_v)
40	-{
41	- CurveProfile *profile = profile_v;
42	-
43	- BKE_curveprofile_selected_handle_set(profile, HD_ALIGN, HD_ALIGN);
44	- BKE_curveprofile_update(profile, false, false);
45	-
46	- rna_update_cb(C, cb_v, NULL);
47	-}
48	-
49	static void CurveProfile_buttons_update(bContext C, void arg1_v, void *profile_v)
50	{
51	CurveProfile *profile = profile_v;
52	@@ -5068,81 +5028,19 @@ static void CurveProfile_buttons_layout(uiLayout layout, PointerRNA ptr, RNAUp
53	bounds.xmax = bounds.ymax = 1000.0;
54	}
55
56	- uiLayoutRow(layout, true);
57	+ row = uiLayoutRow(layout, true);
58
59	- /* Handle type buttons */
60	- bt = uiDefIconBut(block,
61	- UI_BTYPE_BUT,
62	- 0,
63	- ICON_HANDLE_VECTOR,
64	- 0,
65	- 0,
66	- UI_UNIT_X,
67	- UI_UNIT_X,
68	- NULL,
69	- 0.0,
70	- 0.0,
71	- 0.0,
72	- 0.0,
73	- TIP_("Set the point's handle type to sharp"));
74	- UI_but_funcN_set(bt, CurveProfile_buttons_setsharp, MEM_dupallocN(cb), profile);
75	- if (point->h1 == HD_VECT && point->h2 == HD_VECT) {
76	- UI_but_flag_enable(bt, UI_SELECT_DRAW);
77	- }
78	- bt = uiDefIconBut(block,
79	- UI_BTYPE_BUT,
80	- 0,
81	- ICON_HANDLE_AUTO,
82	- 0,
83	- 0,
84	- UI_UNIT_X,
85	- UI_UNIT_X,
86	- NULL,
87	- 0.0,
88	- 0.0,
89	- 0.0,
90	- 0.0,
91	- TIP_("Set the point's handle type to automatic"));
92	- UI_but_funcN_set(bt, CurveProfile_buttons_setcurved, MEM_dupallocN(cb), profile);
93	- if (point->h1 == HD_AUTO && point->h2 == HD_AUTO) {
94	- UI_but_flag_enable(bt, UI_SELECT_DRAW);
95	- }
96	- bt = uiDefIconBut(block,
97	- UI_BTYPE_BUT,
98	- 0,
99	- ICON_HANDLE_FREE,
100	- 0,
101	- 0,
102	- UI_UNIT_X,
103	- UI_UNIT_X,
104	- NULL,
105	- 0.0,
106	- 0.0,
107	- 0.0,
108	- 0.0,
109	- TIP_("Set the point's handle type to free"));
110	- UI_but_funcN_set(bt, CurveProfile_buttons_setfree, MEM_dupallocN(cb), profile);
111	- if (point->h1 == HD_FREE && point->h2 == HD_FREE) {
112	- UI_but_flag_enable(bt, UI_SELECT_DRAW);
113	- }
114	- bt = uiDefIconBut(block,
115	- UI_BTYPE_BUT,
116	- 0,
117	- ICON_HANDLE_ALIGNED,
118	- 0,
119	- 0,
120	- UI_UNIT_X,
121	- UI_UNIT_X,
122	- NULL,
123	- 0.0,
124	- 0.0,
125	- 0.0,
126	- 0.0,
127	- TIP_("Set the point's handle type to aligned"));
128	- UI_but_funcN_set(bt, CurveProfile_buttons_setaligned, MEM_dupallocN(cb), profile);
129	- if (point->h1 == HD_ALIGN && point->h2 == HD_ALIGN) {
130	- UI_but_flag_enable(bt, UI_SELECT_DRAW);
131	- }
132	+ PointerRNA point_ptr;
133	+ RNA_pointer_create(ptr->owner_id, &RNA_CurveProfilePoint, point, &point_ptr);
134	+ PropertyRNA *prop_handle_type = RNA_struct_find_property(&point_ptr, "handle_type_1");
135	+ uiItemFullR(row,
136	+ &point_ptr,
137	+ prop_handle_type,
138	+ RNA_NO_INDEX,
139	+ 0,
140	+ UI_ITEM_R_EXPAND \| UI_ITEM_R_ICON_ONLY,
141	+ "",
142	+ ICON_NONE);
143
144	/* Position */
145	bt = uiDefButF(block,
146	diff --git a/source/blender/makesrna/intern/rna_curveprofile.c b/source/blender/makesrna/intern/rna_curveprofile.c
147	index 489bad29c0e..a69e88f3beb 100644
148	--- a/source/blender/makesrna/intern/rna_curveprofile.c
149	+++ b/source/blender/makesrna/intern/rna_curveprofile.c
150	@@ -141,10 +141,10 @@ static void rna_CurveProfile_update(struct CurveProfile *profile)
151	#else
152
153	static const EnumPropertyItem prop_handle_type_items[] = {
154	- {HD_AUTO, "AUTO", 0, "Auto Handle", ""},
155	- {HD_VECT, "VECTOR", 0, "Vector Handle", ""},
156	- {HD_FREE, "FREE", 0, "Free Handle", ""},
157	- {HD_ALIGN, "ALIGN", 0, "Aligned Free Handles", ""},
158	+ {HD_AUTO, "AUTO", ICON_HANDLE_AUTO, "Auto Handle", ""},
159	+ {HD_VECT, "VECTOR", ICON_HANDLE_VECTOR, "Vector Handle", ""},
160	+ {HD_FREE, "FREE", ICON_HANDLE_FREE, "Free Handle", ""},
161	+ {HD_ALIGN, "ALIGN", ICON_HANDLE_ALIGNED, "Aligned Free Handles", ""},
162	{0, NULL, 0, NULL, NULL},
163	};
164

With that I get the same results (icons were updated meanwhile):

A little issue with that is that (for some reason :) ) there are two handle types, rather than one, even though both have to match from what I can tell. You can use the RNA set() callback to set the other handle, whenever one changes. Ensuring the profile is updated should also happen "implicitly" through the RNA update() callback then.
You may also want to update the description of the handle type RNA properties then, they read a bit weird :)

In fact the same applies to the other buttons in CurveProfile_buttons_layout(), they could use RNA to simplify things.

Also, the cleanups should be committed separately.

source/blender/blenkernel/intern/curveprofile.c
978	With multiple booleans it always becomes hard to read and add calls. You have to constantly check the definition to make sure you're using the correct order. I'd suggest adding an enum with bitflags to `BKE_curveprofile.h` instead. This also makes adding further arguments much easier.
source/blender/makesdna/DNA_curveprofile_types.h
49–50	Would suggest: float h1_loc[2]; float h2_loc[2]; The curves variable naming conventions are already confusing (h1, h2, f1, f2, f3, f5, ...), let's try to not make it worse.
source/blender/makesrna/intern/rna_curveprofile.c
281	Is the term "table" used anywhere else (except of in the two following description) or is there a written definition of it? It does seem like a technical detail, that doesn't belong into a tooltip. Could just say "Refresh internal data, remove doubles and clip points".

This revision now requires changes to proceed.Feb 6 2020, 3:49 PM

Thanks a lot for the review!

In D6470#160108, @Julian Eisel (Severin) wrote:

I was a bit confused about the code in CurveProfile_buttons_layout(). It manually adds buttons for enums and such, despite them representing RNA data. In such cases our UI code should be able to get the needed info from RNA and place the buttons itself.

Yes! This is great! Over the summer I couldn't get it working using RNA directly, the updating wasn't working correctly. But I even left a note in there-- "This should probably use RNA directly!" So thanks for the help there. I'll resolve that in the next step

A little issue with that is that (for some reason :) ) there are two handle types, rather than one, even though both have to match from what I can tell. You can use the RNA set() callback to set the other handle, whenever one changes. Ensuring the profile is updated should also happen "implicitly" through the RNA update() callback then.

I tried to support separate handle types for each handle because curve objects seem to, and I'd eventually like to support importing curve objects into the widget. It's confusing for curve objects though, so it's probably not necessary.

Also, the cleanups should be committed separately.

Sure, I'll do that.

Use bitfield flag instead of booleans for update call
Change to h1_loc and h2_loc
Update RNA UI text

Use RNA directly to build the template UI. I just need to figure out how to get it to update correctly, but the code from Julian should help.

Use RNA directly for updating the handle types. This proved difficult because the points on the path didn't have a reference to the profile itself, which needs to be updated.

Julian suggested adding a function to find the CurveProfile using the RNA ID of the points, which worked but was a bit hacky, and it wouldn't work if someone added a custom curveprofile property through python, which I don't want to make less possible.

So I added a reference to the profile from each point, which is reset on loading a CurveProfile. This should be useful for converting other template UI to just RNA, which I'll make another patch for.

Simon Thommes (simonthommes) added a subscriber: Simon Thommes (simonthommes).Mar 30 2020, 7:15 PM

Merge master

Hans Goudey (HooglyBoogly) mentioned this in T68330: Bevel Custom Profiles Project To Do List.Jun 17 2020, 3:51 PM

Wow, this still merges to master without any conflicts!

Harbormaster completed remote builds in B8676: Diff 26185.Jun 22 2020, 11:54 PM

Couldn't get it to fail or act weirdly, so this seems ready :) Only minor comments.
Once again, I didn't spend too much time digging into the logic. There will probably be bugs but you can fix them in master.

Would suggest to add an extra sanity assert somewhere, checking that CurveProfilePoint.profile indeed points to the owning profile. Might help catch some bugs, or data-races (like UI dereferencing outdated data after undo). Maybe in BKE_curveprofile_update()?

Sorry for taking so long to get back to this!

source/blender/blenkernel/intern/curveprofile.c
328–330	You could allow passing NULL for the delta. In `BKE_curveprofile_move_handle()` you can do: handle_location[0] += delta ? delta[0] : 0.0f; handle_location[1] += delta ? delta[1] : 0.0f;
349–366	Better assert that the result is >=0.
source/blender/editors/interface/interface_handlers.c
6967	Better use braces for individual checks here, even if it's just for readability.
source/blender/makesdna/DNA_curveprofile_types.h
47–54	Would mention that the handle types are linked together in RNA.

This revision is now accepted and ready to land.Jun 23 2020, 10:41 PM

Hans Goudey (HooglyBoogly) marked 4 inline comments as done.Jun 24 2020, 5:42 PM

Hans Goudey (HooglyBoogly) removed reviewers: Campbell Barton (campbellbarton), William Reynish (billreynish).Jun 24 2020, 5:47 PM

Closed by commit rBbaff05ad1c15: UI: Add Free Handle Types to CurveProfile Widget (authored by Hans Goudey (HooglyBoogly)). · Explain WhyJun 24 2020, 5:50 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rBbaff05ad1c15: UI: Add Free Handle Types to CurveProfile Widget.

Add Free Handle Types to CurveProfile Widget
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 26241

source/blender/blenkernel/BKE_curveprofile.h

source/blender/blenkernel/intern/curveprofile.c

source/blender/editors/interface/interface_draw.c

source/blender/editors/interface/interface_handlers.c

source/blender/editors/interface/interface_templates.c

source/blender/makesdna/DNA_curveprofile_types.h

source/blender/makesrna/intern/rna_curveprofile.c

Add Free Handle Types to CurveProfile WidgetClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 26241

source/blender/blenkernel/BKE_curveprofile.h

source/blender/blenkernel/intern/curveprofile.c

source/blender/editors/interface/interface_draw.c

source/blender/editors/interface/interface_handlers.c

source/blender/editors/interface/interface_templates.c

source/blender/makesdna/DNA_curveprofile_types.h

source/blender/makesrna/intern/rna_curveprofile.c

Add Free Handle Types to CurveProfile Widget
ClosedPublic
Actions

Revision Contents
Changeset List