Fix #119596: Multiple points operations of Curvemap and Curveprofile #120242

XDzZyq · 2024-04-04T08:18:13+02:00

XDzZyq commented

2024-04-04 08:18:13 +02:00

As the issue mentioned in #119596, there are a few undefined behaviors when operating more than one point. This PR will try to fix this issue by defining the operations such as shift and remove for more than one selected point.

Curvemap
Curveprofile

As the issue mentioned in https://projects.blender.org/blender/blender/issues/119596, there are a few undefined behaviors when operating more than one point. This PR will try to fix this issue by defining the operations such as ```shift``` and ```remove``` for more than one selected point. - [x] Curvemap - [x] Curveprofile

XDzZyq added 1 commit 2024-04-04 08:18:29 +02:00

01541fecf4 4.4 Store all selected point with Vector

XDzZyq added 3 commits 2024-04-04 10:08:07 +02:00

06b57d1a85 4.4 Shift point positions from center

128f56fb2d 4.4 flags

8a6cabd4bf Update interface_templates.cc

XDzZyq commented

2024-04-04 10:18:01 +02:00

the general goal has been accomplished.

Shift all selected points together with slider control.
Display selected points properties (flags) properly.
Clamp the position on the boundary.

However, the center of the selected points is now stored in CurveMap and will be updated in interface_template.cc, I am not sure it is a good idea.

the general goal has been accomplished. - Shift all selected points together with slider control. - Display selected points properties (flags) properly. - Clamp the position on the boundary. <video src="/attachments/9a627dc6-b747-4ac3-aad8-85cd294823e4" title="multi-selection.mp4" controls></video> However, the center of the selected points is now stored in ```CurveMap``` and will be updated in ```interface_template.cc```, I am not sure it is a good idea.

multi-selection.mp4

1.3 MiB

XDzZyq changed title from ~~WIP: Fix #119596: Multiple points operations of curvemapping~~ to Fix #119596: Multiple points operations of curvemapping

2024-04-04 10:18:14 +02:00

XDzZyq requested review from Campbell Barton 2024-04-04 10:23:10 +02:00

XDzZyq added 1 commit 2024-04-04 10:25:48 +02:00

a92176468c unused variable removed

Iliya Katushenock reviewed 2024-04-04 16:28:01 +02:00

source/blender/editors/interface/templates/interface_templates.cc Outdated

						
				@ -4560,3 +4560,2 @@

				  /* Sliders for selected curve point. */

				  int i;

				  CurveMapPoint *cmp = nullptr;

				  Vector<CurveMapPoint *> cmps{};

Iliya Katushenock commented

2024-04-04 16:25:48 +02:00

Vector<CurveMapPoint *> cmps;

`Vector<CurveMapPoint *> cmps;`

Zyq-XDz marked this conversation as resolved

source/blender/editors/interface/templates/interface_templates.cc Outdated

						
				@ -4609,2 +4608,2 @@

				    {

				      bt->flag |= UI_SELECT_DRAW;

				    for (CurveMapPoint *cmp : cmps) {

Iliya Katushenock commented

2024-04-04 16:27:32 +02:00

for (const CurveMapPoint *cmp : cmps) {

`for (const CurveMapPoint *cmp : cmps) {`

Zyq-XDz marked this conversation as resolved

source/blender/editors/interface/templates/interface_templates.cc Outdated

						
				@ -4676,1 +4687,4 @@

				    active_cm->center_y = 0.0f;

				    for (CurveMapPoint *cmp : cmps)

				      active_cm->center_y += cmp->y;

Iliya Katushenock commented

2024-04-04 16:26:10 +02:00

for (CurveMapPoint *cmp : cmps) {
  ...
}

See: https://developer.blender.org/docs/handbook/guidelines/c_cpp/#braces

And the same above.

```Cpp for (CurveMapPoint *cmp : cmps) { ... } ``` See: https://developer.blender.org/docs/handbook/guidelines/c_cpp/#braces And the same above.

Zyq-XDz marked this conversation as resolved

XDzZyq added 1 commit 2024-04-05 02:37:29 +02:00

a6c21ff00e 4.5 C++ code style

Campbell Barton requested changes 2024-04-05 05:18:02 +02:00

Campbell Barton left a comment

It should be possible to implement this as a more straightforward operation that applies an offset on the selection. Notes inline.

source/blender/blenkernel/intern/colortools.cc Outdated

						
				@ -930,6 +930,34 @@ void BKE_curvemapping_premultiply(CurveMapping *cumap, bool restore)

				}

				/* ************************ more CurveMapping calls *************** */

				void BKE_curvemap_shift(CurveMapping *cumap)

Campbell Barton commented

2024-04-05 05:16:51 +02:00

Prefer BKE_curvemap_translate a float2 offset argument.

Prefer `BKE_curvemap_translate` a float2 offset argument.

Zyq-XDz marked this conversation as resolved

source/blender/makesdna/DNA_color_types.h Outdated

						
				@ -48,6 +48,8 @@ typedef struct CurveMap {

				  float ext_in[2], ext_out[2];

				  /** Actual curve. */

				  CurveMapPoint *curve;

				  /** Center of selected points */

Campbell Barton commented

2024-04-05 05:16:00 +02:00

I'd rather avoid adding run-time UI data into CurveMap, this seems like a value that could be stored in run-time data such as uiHandleButtonData.

I'd rather avoid adding run-time UI data into `CurveMap`, this seems like a value that could be stored in run-time data such as `uiHandleButtonData`.

XDzZyq commented

2024-04-05 08:22:55 +02:00

I noticed there are a few members named vec[3] and origvec[3], may I modify them for this purpose?

I noticed there are a few members named ```vec[3]``` and ```origvec[3]```, may I modify them for this purpose?

Campbell Barton commented

2024-04-05 08:28:45 +02:00

These are for vector buttons but I suppose they could be used, check on using value, origvalue, startvalue in-place, that may be all you need.

These are for vector buttons but I suppose they could be used, check on using `value, origvalue, startvalue` in-place, that may be all you need.

XDzZyq commented

2024-04-08 06:50:29 +02:00

294ccde79f/source/blender/makesdna/DNA_color_types.h (L30-L34)

Since each CurveMap requires runtime storage, I imitated how NodeCryptomatte_Runtime and Camera_Runtime are set up. I've also tried to implement it with pointers, not that much difference. If this solution is fine, I'll finish the rest of the code. :)

https://projects.blender.org/blender/blender/src/commit/294ccde79f84b267fe31c18683872406babeb4ee/source/blender/makesdna/DNA_color_types.h#L30-L34 Since each CurveMap requires runtime storage, I imitated how ```NodeCryptomatte_Runtime``` and ```Camera_Runtime``` are set up. I've also tried to implement it with pointers, not that much difference. If this solution is fine, I'll finish the rest of the code. :)

Zyq-XDz marked this conversation as resolved

XDzZyq changed title from ~~Fix #119596: Multiple points operations of curvemapping~~ to WIP: Fix #119596: Multiple points operations of curvemapping

2024-04-05 09:11:35 +02:00

Iliya Katushenock added this to the User Interface project 2024-04-05 10:18:48 +02:00

XDzZyq added 1 commit 2024-04-07 12:20:56 +02:00

5362ce8c59 4.7 runtime storage

XDzZyq changed title from ~~WIP: Fix #119596: Multiple points operations of curvemapping~~ to WIP: Fix #119596: Multiple points operations of Curvemap and Curveprofile

2024-04-07 17:05:16 +02:00

XDzZyq added 2 commits 2024-04-07 18:21:15 +02:00

d2606602df 4.8 hue correct requires 3 curvemap

dd8a6e2963 4.8 initializations and simplification

XDzZyq added 1 commit 2024-04-08 06:38:08 +02:00

294ccde79f no pointers

XDzZyq added 1 commit 2024-04-08 13:32:46 +02:00

daabfbb857 4.8 CurveProfile

XDzZyq added 1 commit 2024-04-08 13:57:05 +02:00

6ee9030866 Update interface_templates.cc

XDzZyq changed title from ~~WIP: Fix #119596: Multiple points operations of Curvemap and Curveprofile~~ to Fix #119596: Multiple points operations of Curvemap and Curveprofile

2024-04-08 13:57:53 +02:00

XDzZyq requested review from Campbell Barton 2024-04-08 13:58:12 +02:00

XDzZyq added 1 commit 2024-04-10 09:08:19 +02:00

5f4a83e26c 4.10 unnecessary code reuse

Campbell Barton commented

2024-04-10 10:13:59 +02:00

Adding a curve-runtime to every curve map and curve profile seems unnecessary for UI logic where only one value is edited at a time.

It seems this can be replaced with a single static float[2] which also isn't so nice but at least avoids adding run-time UI data into DNA.

Attached a path that does this.

I would need to dig into this in more detail to check on having a more elegant solution, as I mentioned in chat, it might be cleanest to use the same method the 3D viewport uses to transform multiple vertices at once.

CC'ing @JulianEisel for a second opinion.

Adding a curve-runtime to every curve map and curve profile seems unnecessary for UI logic where only one value is edited at a time. It seems this can be replaced with a single `static float[2]` which also isn't so nice but at least avoids adding run-time UI data into DNA. Attached a path that does this. ---- I would need to dig into this in more detail to check on having a more elegant solution, as I mentioned in chat, it might be cleanest to use the same method the 3D viewport uses to transform multiple vertices at once. CC'ing @JulianEisel for a second opinion.

120242_update.diff

10 KiB

👍 1

XDzZyq requested review from Julian Eisel 2024-04-11 16:53:44 +02:00

XDzZyq added 1 commit 2024-04-14 13:30:30 +02:00

77f7a5ed29 4.14 formating

XDzZyq added 1 commit 2024-04-24 09:30:41 +02:00

dc5f2fe6af 4.24 runtime_storage

Merge conflict checking is in progress. Try again in few moments.

View command line instructions.

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u curve-mapping-multiselect:Zyq-XDz-curve-mapping-multiselect

git checkout Zyq-XDz-curve-mapping-multiselect

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Fix #119596: Multiple points operations of Curvemap and Curveprofile #120242

Checkout