Crop node gizmo doesn't work #94784

New Issue

Campbell Barton · 2022-01-10T11:37:33+01:00

Campbell Barton commented

2022-01-10 11:37:33 +01:00

The crop nodes gizmo displays as expected in 3.0 but can't be interacted with.

This is a regression in 482806c816 (should have been picked up in patch review).

With the attached file

Toggle the backdrop off/on so it refreshes (seems like a bug that this is needed at all...).
Select a corner of the crop node and drag it.
Notice all values are set to zero.

compo.blend

2.93 and prior to 482806c816 work as expected.

The crop nodes gizmo displays as expected in 3.0 but can't be interacted with. This is a regression in 482806c816 (should have been picked up in patch review). With the attached file - Toggle the backdrop off/on so it refreshes *(seems like a bug that this is needed at all...).* - Select a corner of the crop node and drag it. - Notice all values are set to zero. [compo.blend](https://archive.blender.org/developer/F12798604/compo.blend) 2.93 and prior to 482806c816 work as expected.

Sebastian Parborg was assigned by Campbell Barton

2022-01-10 11:37:33 +01:00

Campbell Barton commented

2022-01-10 11:37:33 +01:00

Added subscriber: @ideasman42

Philipp Oeser commented

2022-01-10 11:55:17 +01:00

Added subscriber: @lichtwerk

Philipp Oeser commented

2022-01-10 11:55:17 +01:00

Changed status from 'Needs Triage' to: 'Confirmed'

Philipp Oeser commented

2022-01-10 11:55:17 +01:00

CC @ZedDB

Sebastian Parborg commented

2022-01-12 15:56:11 +01:00

The bisection didn't catch the correct commit actually.
My commit broke it, yes. but we reverted part of it later so that the code started working again.

The commit that breaks this is actually cbca71a7cf
There is a mistake where the rctf that is passed to BLI_rctf_isect had its max and min values swapped.

The older code explicitly stated which variables it would set, however the raw order is not the same.
So the clean up changes set the xmin AND xmax to zero instead of xmin and ymin.

I think this is the correct fix:

diff --git a/source/blender/editors/space_node/node_gizmo.cc b/source/blender/editors/space_node/node_gizmo.cc
index 41d13976209..9aa4f299774 100644
--- a/source/blender/editors/space_node/node_gizmo.cc
+++ b/source/blender/editors/space_node/node_gizmo.cc
@@ -292,7 +292,7 @@ static void gizmo_node_crop_prop_matrix_set(const wmGizmo *gz,
   const bool ny = rct.ymin > rct.ymax;
   BLI_rctf_resize(&rct, fabsf(matrix[0][0]), fabsf(matrix[1][1]));
   BLI_rctf_recenter(&rct, (matrix[3][0] / dims[0]) + 0.5f, (matrix[3][1] / dims[1]) + 0.5f);
-  const rctf rct_isect{0, 0, 1, 1};
+  const rctf rct_isect{0, 1, 0, 1};
   BLI_rctf_isect(&rct_isect, &rct, &rct);
   if (nx) {
     SWAP(float, rct.xmin, rct.xmax);

Can you guys confirm?

The bisection didn't catch the correct commit actually. My commit broke it, yes. but we reverted part of it later so that the code started working again. The commit that breaks this is actually cbca71a7cf There is a mistake where the `rctf` that is passed to `BLI_rctf_isect` had its max and min values swapped. The older code explicitly stated which variables it would set, however the raw order is not the same. So the clean up changes set the xmin AND xmax to zero instead of xmin and ymin. I think this is the correct fix: ``` diff --git a/source/blender/editors/space_node/node_gizmo.cc b/source/blender/editors/space_node/node_gizmo.cc index 41d13976209..9aa4f299774 100644 --- a/source/blender/editors/space_node/node_gizmo.cc +++ b/source/blender/editors/space_node/node_gizmo.cc @@ -292,7 +292,7 @@ static void gizmo_node_crop_prop_matrix_set(const wmGizmo *gz, const bool ny = rct.ymin > rct.ymax; BLI_rctf_resize(&rct, fabsf(matrix[0][0]), fabsf(matrix[1][1])); BLI_rctf_recenter(&rct, (matrix[3][0] / dims[0]) + 0.5f, (matrix[3][1] / dims[1]) + 0.5f); - const rctf rct_isect{0, 0, 1, 1}; + const rctf rct_isect{0, 1, 0, 1}; BLI_rctf_isect(&rct_isect, &rct, &rct); if (nx) { SWAP(float, rct.xmin, rct.xmax); ``` Can you guys confirm?

Hans Goudey commented

2022-01-12 16:18:02 +01:00

Added subscriber: @HooglyBoogly

Hans Goudey commented

2022-01-12 16:18:02 +01:00

That seems to work for me, sorry for the typo.

Campbell Barton commented

2022-01-13 07:48:28 +01:00

This works, although I'm increasingly of the opinion that we should avoid positional arguments when initializing struct members especially when moving C code to C++, it's bitten us a few times, is less readable - and can potentially backfire if struct members are ever recorded.

Sebastian Parborg commented

2022-01-13 16:52:46 +01:00

@ideasman42 how would you like it to be initialized?

Campbell Barton commented

2022-01-14 00:24:44 +01:00

Until MSVC supports C99's struct initialization - this is the closest match AFAICS.

rctf rct_isect{};
rct_isect.xmin = 0;
rct_isect.xmax = 1;
rct_isect.ymin = 0;
rct_isect.ymax = 1;

The {} are important, to zero members not assigned later (in the case of rctf it's not so important but as a convention I think we should stick to this as it's caused multiple bugs when moving from C to C++ (Unless leaving other members uninitialized is intended, in that case it can be left out).
It's unfortinate the struct cant remain const but I don't think there are better alternatives.

Until MSVC supports C99's struct initialization - this is the closest match AFAICS. ``` rctf rct_isect{}; rct_isect.xmin = 0; rct_isect.xmax = 1; rct_isect.ymin = 0; rct_isect.ymax = 1; ``` - The `{}` are important, to zero members not assigned later (in the case of `rctf` it's not so important but as a convention I think we should stick to this as it's caused multiple bugs when moving from C to C++ (Unless leaving other members uninitialized is intended, in that case it can be left out). - It's unfortinate the struct cant remain `const` but I don't think there are better alternatives.

blender-admin commented

2022-01-14 17:42:28 +01:00

This issue was referenced by 5703efab88

This issue was referenced by 5703efab886dd612e6a52f9ac81e157db754b14f

Sebastian Parborg commented

2022-01-14 17:44:45 +01:00

Changed status from 'Confirmed' to: 'Resolved'

Sebastian Parborg closed this issue

2022-01-14 17:44:45 +01:00

Philipp Oeser commented

2022-01-17 12:36:31 +01:00

@ZedDB: for 3.0.1 corrective release, 5703efab88 doesnt apply (cbca71a7cf is not in 3.0)

In #94784#1287023, @ZedDB wrote:
My commit broke it, yes. but we reverted part of it later so that the code started working again.

What was reverted partially? What should go into 3.0.1? Are we sure this is actually broken in blender-v3.0-release? (I cannot reproduce there... neither in 3.0 release for some reason, pretty sure I could repro initially though)

@ZedDB: for 3.0.1 corrective release, 5703efab88 doesnt apply (cbca71a7cf is not in 3.0) > In #94784#1287023, @ZedDB wrote: > My commit broke it, yes. but we reverted part of it later so that the code started working again. What was reverted partially? What should go into 3.0.1? Are we sure this is actually broken in blender-v3.0-release? (I cannot reproduce there... neither in 3.0 release for some reason, pretty sure I could repro initially though)

Sebastian Parborg commented

2022-01-17 15:32:38 +01:00

@lichtwerk my bad, the clean up commit from Hans is indeed not in the 3.0 branch.
So disregard this fix.

@lichtwerk my bad, the clean up commit from Hans is indeed not in the 3.0 branch. So disregard this fix.

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Crop node gizmo doesn't work #94784