Fix #107924: Scale limit respect negative values #107937

Open
YimingWu wants to merge 3 commits from ChengduLittleA/blender:limit_scale_fix into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.
Member

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.

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

@ideasman42 , can you check?

@ideasman42 , can you check?
Author
Member

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

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
@ -1734,3 +1734,2 @@
mat4_to_size(size, cob->matrix);
mat4_to_size(obsize, cob->matrix);
mat4_to_size_handed(size, cob->matrix);

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

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

@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`.
YimingWu force-pushed limit_scale_fix from ac953cbab7 to 84b0735703 2023-06-14 17:08:41 +02:00 Compare
Sybren A. Stüvel added
Module
Animation & Rigging
and removed
Module
Modeling
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

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).
Author
Member

@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?
Author
Member

Edited to better clarify the impact.

Edited to better clarify the impact.

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.

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

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).
Author
Member

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
Member

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

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

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
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser Project (Legacy)
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#107937
No description provided.