Fix #107924: Scale limit respect negative values #107937

YimingWu · 2023-05-15T08:10:52+02:00

YimingWu commented

2023-05-15 08:10:52 +02:00

This patch fixes the Scale Constraint to make it effective in both
positive/negative side and correctly handle cross-zero scaling.

The original problem was that the old implementation didn't take
negative scaling into account, the constraint then appears to limit
scaling to a range of (-max, max) if no minimum scaling limit is
enabled, and it ambiguously allows scaling to be in (-max, -min) and
(min, max) ranges when minimum limit is enabled, cause confusion.

What does this change affect:

It only affects the behaviour of the Scale Constraint, also affects the
transformation operators on objects which has Scale Constraint.

Limitations of this patch:

It can't correctly handle objects that has scaling factors that are not
all positive or all negative, due to the limited information available.
It will break some old files where they have set a limit of (a, b) where
(a!=0 && b!=0). Previously it allows a transformation range of
(-b, -a) and (a, b), now only (a, b) is accepted. Files with (a==0) is
converted into (-b, b) automatically to better preserve old behaviour.

To fully overcome limitation 1, additional scale factors are needed
alongside the transformation matrix, this can be considered if the
constraint system needs a refactor in the future.

This patch fixes the Scale Constraint to make it effective in both positive/negative side and correctly handle cross-zero scaling. The original problem was that the old implementation didn't take negative scaling into account, the constraint then appears to limit scaling to a range of (-max, max) if no minimum scaling limit is enabled, and it ambiguously allows scaling to be in (-max, -min) and (min, max) ranges when minimum limit is enabled, cause confusion. What does this change affect: It only affects the behaviour of the Scale Constraint, also affects the transformation operators on objects which has Scale Constraint. Limitations of this patch: 1. It can't correctly handle objects that has scaling factors that are not all positive or all negative, due to the limited information available. 2. It will break some old files where they have set a limit of (a, b) where (a!=0 && b!=0). Previously it allows a transformation range of (-b, -a) and (a, b), now only (a, b) is accepted. Files with (a==0) is converted into (-b, b) automatically to better preserve old behaviour. To fully overcome limitation 1, additional scale factors are needed alongside the transformation matrix, this can be considered if the constraint system needs a refactor in the future.

👍 1

YimingWu added the

Module

Modeling

label 2023-05-15 08:39:27 +02:00

YimingWu added this to the Modeling project 2023-05-15 08:39:34 +02:00

Pratik Borhade commented

2023-05-15 10:36:12 +02:00

@ideasman42 , can you check?

YimingWu commented

2023-05-15 10:41:06 +02:00

Also note: This might potentially break older files which may have taken advantage of the old "incorrect" behaviour.

Hans Goudey changed title from ~~Fix #107924: Scale limit respect negative values.~~ to Fix #107924: Scale limit respect negative values

2023-05-15 14:57:45 +02:00

Campbell Barton reviewed 2023-05-16 06:56:38 +02:00

source/blender/blenkernel/intern/constraint.c Outdated

						
				@ -1734,3 +1734,2 @@

				  mat4_to_size(size, cob->matrix);

				  mat4_to_size(obsize, cob->matrix);

				  mat4_to_size_handed(size, cob->matrix);

Campbell Barton commented

2023-05-16 06:56:38 +02:00

Instead of a new scale function, the size could be flipped if the matrix is negative:

   mat4_to_size(size, cob->matrix);
+  if (is_negative_m4(cob->matrix)) {
+    negate_v3(size);
+  }

Although I think this might not be the way to go, I'll reply to the PR.

Instead of a new scale function, the size could be flipped if the matrix is negative: ``` mat4_to_size(size, cob->matrix); + if (is_negative_m4(cob->matrix)) { + negate_v3(size); + } ``` Although I think this might not be the way to go, I'll reply to the PR.

ChengduLittleA marked this conversation as resolved

Campbell Barton commented

2023-05-16 07:25:58 +02:00

The problem with supporting negative scales is we can't know which scale axes are negated from the matrix alone.

We could accept supporting entirely positive or entirely negative scales in that case the code-comments & user documentation should note that mixed negative/positive scale aren't supported.

This will change behavior for existing files, as negative scales aren't so common the fix could be deemed acceptable.
Or, we could document the current behavior with negative scales as a known limitation.

Negative scales could be supported by adding a toggle to optionally change behavior but it seems quite obscure to have it's own option.

I'll leave this up to the animation team to decide.

Code review:

Prefer not to add mat4_to_size_handed, since negating all axes isn't extracting the sign of each scale properly. Flipping the scale for negative matrices can be done inline - with an explanation for why the approximation is acceptable.
Code comments should note flipping the size only correctly handles all scales being negative or none.
Code comments should note flipping the size is effectively flipping the limit range (as dividing the clamped scale by the clamped cancels out the negation).

The problem with supporting negative scales is we can't know which scale axes are negated from the matrix alone. We could accept supporting entirely positive or entirely negative scales in that case the code-comments & user documentation should note that mixed negative/positive scale aren't supported. This will change behavior for existing files, as negative scales aren't so common the fix could be deemed acceptable. Or, we could document the current behavior with negative scales as a known limitation. Negative scales could be supported by adding a toggle to optionally change behavior but it seems quite obscure to have it's own option. I'll leave this up to the animation team to decide. ---- Code review: - Prefer not to add `mat4_to_size_handed`, since negating all axes isn't extracting the sign of each scale properly. Flipping the scale for negative matrices can be done inline - with an explanation for why the approximation is acceptable. - Code comments should note flipping the size only correctly handles all scales being negative or none. - Code comments should note flipping the size is effectively flipping the limit range (as dividing the clamped scale by the clamped cancels out the negation).

Campbell Barton requested review from Sybren A. Stüvel 2023-05-16 07:27:30 +02:00

YimingWu force-pushed limit_scale_fix from 16a2e3c2ce to ac953cbab7

2023-05-25 08:28:55 +02:00

Compare

Sybren A. Stüvel commented

2023-06-12 13:54:53 +02:00

@ChengduLittleA please add more info to the PR description (ref: Ingredients of a PR). Most importantly, the PR description doesn't describe what the impact of these changes could be. Is it limited to only constraints (as suggested by the title of #107924), or would it cause more wide-spread changes to Blender's behaviour? For example, I can imagine current users of the Limit Scale constraint to disallow growing but allow shrinking and mirroring, and have that set up with max{X,Y,Z} = 1.

@ChengduLittleA please add more info to the PR description (ref: [Ingredients of a PR](https://wiki.blender.org/wiki/Process/Contributing_Code#Ingredients_of_a_Pull_Request)). Most importantly, the PR description doesn't describe what the impact of these changes could be. Is it limited to only constraints (as suggested by the title of #107924), or would it cause more wide-spread changes to Blender's behaviour? For example, I can imagine current users of the Limit Scale constraint to disallow growing but allow shrinking and mirroring, and have that set up with `max{X,Y,Z} = 1`.

👍 1

YimingWu force-pushed limit_scale_fix from ac953cbab7 to 84b0735703

2023-06-14 17:08:41 +02:00

Compare

Sybren A. Stüvel added

and removed

labels 2023-06-22 11:20:10 +02:00

Sybren A. Stüvel modified the project from Modeling to Animation & Rigging

2023-06-22 11:20:14 +02:00

YimingWu force-pushed limit_scale_fix from 84b0735703 to a9f65464d8

2023-07-04 07:49:30 +02:00

Compare

Sybren A. Stüvel commented

2023-07-06 11:03:50 +02:00

It will break old files with unmodified setup. A versioning code is put in place to change old files that has a limit range of (0, a) to (-a, a) to match previous behaviour.

So will this break old files? Because the quoted paragraph says both (it will break, but also it will match previous behavior).

> It will break old files with unmodified setup. A versioning code is put in place to change old files that has a limit range of (0, a) to (-a, a) to match previous behaviour. So will this break old files? Because the quoted paragraph says both (it _will_ break, but also it will match previous behavior).

YimingWu commented

2023-07-06 11:07:00 +02:00

@dr.sybren Ah I should rephrase it... It will only keep the behaviour of old files who has limits set to (0,a), but will break if min is not set / not 0 and when user takes advantage of the old behaviour to use (a, b) and (-b, -a) range. Hummm do you have suggestion on how should I say it more clearly?

@dr.sybren Ah I should rephrase it... It will only keep the behaviour of old files who has limits set to (0,a), but will break if min is not set / not 0 _and_ when user takes advantage of the old behaviour to use (a, b) and (-b, -a) range. Hummm do you have suggestion on how should I say it more clearly?

YimingWu commented

2023-07-06 11:21:59 +02:00

Edited to better clarify the impact.

Sybren A. Stüvel commented

2023-07-06 12:00:09 +02:00

It will break some old files except those that has a limit range of (0, a), it will be converted to (-a, a) and to match previous behaviour.

This could have some explanation of what will break, exactly. Maybe something like this?

Files will be versioned such that (a, b) ranges will be converted to (-c, c), where c = max(abs(a), abs(b)). Since before this patch, a range of (a, b) would also allow the range (-b, -a), this may introduce a small difference in allowed ranges when (... you fill this out, as you know more about this exact case than I do ... )

To fully fix problem 1

Just a nitpick, but the list was about limitations, not problems. When a concept has been given a name, it's better to stick to that name, as it'll be easier to read & understand.

> It will break some old files except those that has a limit range of (0, a), it will be converted to (-a, a) and to match previous behaviour. This could have some explanation of what will break, exactly. Maybe something like this? Files will be versioned such that `(a, b)` ranges will be converted to `(-c, c)`, where `c = max(abs(a), abs(b))`. Since before this patch, a range of `(a, b)` would also allow the range `(-b, -a)`, this may introduce a small difference in allowed ranges when (_... you fill this out, as you know more about this exact case than I do ..._ ) > To fully fix problem 1 Just a nitpick, but the list was about *limitations*, not *problems*. When a concept has been given a name, it's better to stick to that name, as it'll be easier to read & understand.

Sybren A. Stüvel commented

2023-09-26 14:19:53 +02:00

After re-testing this PR, it seems that it only works when set to World Space. In Local Space it still allows flipping, even when the minimum scale is set to (0, 0, 0).

👀 1

YimingWu commented

2023-09-27 04:29:08 +02:00

In Local Space it still allows flipping, even when the minimum scale is set to (0, 0, 0).

Humm this sounds pretty nasty... will check again

> In Local Space it still allows flipping, even when the minimum scale is set to (0, 0, 0). Humm this sounds pretty nasty... will check again

Sybren A. Stüvel requested changes 2023-09-28 11:54:06 +02:00

Sybren A. Stüvel left a comment

Marking with 'request changes' so that this PR is off my review queue.

This pull request has changes conflicting with the target branch.

source/blender/blenkernel/intern/constraint.c
source/blender/blenloader/intern/versioning_400.cc

View command line instructions.

Checkout

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

git fetch -u limit_scale_fix:ChengduLittleA-limit_scale_fix

git checkout ChengduLittleA-limit_scale_fix

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 #107924: Scale limit respect negative values #107937

Checkout