#
Fix #117927: Limit rotation constraint flipping #118502

No reviewers

**Labels**

No Label

Interest

Alembic

Interest

Animation & Rigging

Interest

Asset Browser

Interest

Asset Browser Project Overview

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 project

No Assignees

**4 Participants**

**Notifications**

**Due Date**

No due date set.

**Dependencies**

No dependencies set.

Reference: blender/blender#118502

Loading…

Reference in New Issue

No description provided.

Delete Branch "ChrisLend/blender:fix_limit_rot_constraint"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?

The issue from the bug report mentions that the

`Limit Rotation`

constraint snapsback to 0 when reaching 180 degrees with a min and max of 0 / 180.

The root cause of this goes a bit deeper though. Because the following also snaps back to 0.

The reason for this is that the clamping logic in the constraint was very simple.

It just took the matrix, decomposed it to euler and clamped the values directly.

However in that process, the euler angles are bound to a range of -180 / 180,

which means that any angle >= 180 would snap back to the min.

The example uses a bone, but the same code is used for objects too!@blender-bot package

Package build started. Download here when ready.

Thanks for the fix, tested!

The rest of my points are probably off-topic, so feel free to ignore, but I'll just leave it here anyways. If nothing else, it can be good to acknowledge these as known limitations of the system.

`@ -1661,0 +1663,4 @@`

`* considered a valid range.*/`

`static float clamp_angle(const float angle, const float min, const float max)`

`{`

`if (min == max) {`

I think this can be

`if (min >= max)`

.I think it would also be good to add the case

`if ((max - min) >= (2 * PI))`

which just always returns`angle`

as-is.The intuition here is that if min and max constrain the angle to 0 degrees or less, it can't move, but if they constraint it to 360 degrees or more that's the same as being unconstrained.

With this PR that's not actually true. Given min and max of 0 and 370, the rotation will be constrained between 0 and 10 degrees.

This is a bit different than the previous behavior, where it would allow values until 180 and then jump back to 0.

Conceptually the constraint doesn't clamp euler angles even though the GUI makes it seem like that.

It decomposes the matrix which will lead to a range of -pi/pi no matter the euler values.

That means the constraint will also work for quaternions, but of course means we can't simply clamp euler values.

What I am trying to do with this PR is to remove the assumption in the code that we have simple euler values.

For context: what initially prompted this suggestion was @Mets confusion about a 0 to 360 range now immobilizing the rotation. I was thinking about what could conceptually make sense to avoid that, and what I came to was the idea that

`max - min`

represents the magnitude of the restricted range.Unless I'm missing something, I think that still preserves all of the expressive power (everything that was representable before can still be represented) while also making things more intuitive for users.

I'm aware that it works by first decomposing the transform matrix to euler, and I think(?) I recognize the various implications of that. And I'm not suggesting clamping the euler values, but rather giving the distance between min and max in their own space semantic meaning. Which we're already doing anyway, otherwise we wouldn't be able to distinguish between an allowed range > 180 degrees vs < 180 degrees.

ah ok I think I missed your point, yes indeed that's a good idea.

Changed that now

nathanvegdahlmarked this conversation as resolved`@ -1661,0 +1669,4 @@`

`float angle_unit_circle[2];`

`angle_unit_circle[0] = cos(angle);`

`angle_unit_circle[1] = sin(angle);`

My gut is saying that going through sin/cos and using dot products etc. might be overkill here. This is definitely doable with just angles, but the thing I'm unsure of is whether that might actually make this more complex with the special cases that come up. (Although I suspect it would at least be computationally cheaper.)

the reason for going through the unit circle is that it avoids the jump around -180/180 (or -pi/pi)

While the values are far apart their actual rotation is identical.

About performance, I assume it will still be super cheap. But I will run a test to get hard numbers.

Yeah, that jump and related things are the special cases that would need to be handled if it's done in terms of angles.

Performance numbers are in. It's about 23% percent slower than pre PR (1730ns to 2100ns).

I assume that trying to do the same fix without

`sin`

and`cos`

will have it's cost too though.I could try to do it only angle based but I'd need your help on this. I thought about it already and don't know how this would work

I've worked out at least one way to do it on Desmos:

https://www.desmos.com/calculator/ptmipo2cmf

However, the limitations of math notation make it look pretty dense. And it also uses a kind of modulus that I'm unsure exists in e.g. the C++ standard library. I'll poke at actually implementing this in C++ the next chance I get (probably tomorrow morning), and then we can see how it compares.

nathanvegdahlmarked this conversation as resolved`@ -1661,0 +1668,4 @@`

`}`

`if (max <= min) {`

`return angle;`

I think this case should still return

`min`

like before, since this is the case that represents immobilizing due to there being no (or negative) space between min and max.ah yeah you are right

nathanvegdahlmarked this conversation as resolvedExtra polish suggestions unless they're already in there:

@ChrisLend Here's a purely angle-based implementation of

`clamp_angle()`

you can test out:@nathanvegdahl I used your code and it works as expected.

In terms of performance it's slightly faster.

yours: ~2200ns

mine: ~2500ns

though with such low numbers I am getting run to run fluctuations that are relatively large.

Did you mean to have those comments in the final code?

also what does the "t" in

`tclamped`

stand for?I just wanted to get you something to test against, so there's several things I didn't put any thought into. I'll do another review pass to criticize my own code and comments. ;-)

`@ -1661,0 +1662,4 @@`

`/**`

`* Wraps a number to be in [-PI, +PI].`

`*`

`* Can also be implemented as `std::remainder(a, M_PI * 2.0)`.`

This note about

`std::remainder`

was just for you, to also test. When I looked into`std::remainder`

's implementation it looked pretty involved, so I suspected it would be quite a bit slower. But I'm not really sure.I'm also just generally wondering if there might be a faster way to implement this. But again, not sure.

I changed the argument name to

`angle`

For

`b`

I am not sure if there is a good nameYou could rename

`b`

to`angle_normalized`

or something like that, maybe? Basically it's mapping [-pi, pi] to [0, 1] so that we can wrap by subtracting floor. And then the mapping is inverted to bring it back to [-pi, pi] again.But I don't know if that will really make much difference for comprehensibility here. So it's up to you.

`@ -1661,0 +1665,4 @@`

`* Can also be implemented as `std::remainder(a, M_PI * 2.0)`.`

`* Not sure which is faster.`

`*/`

`static inline float pi_wrap(const float a)`

I don't know if

`pi_wrap`

is really a good name for this. Maybe`wrap_angle`

? The idea is that it puts an angle into a canonical space where every angle has precisely one representation, rather than an infinite number from looping.used

`wrap_rad_angle`

. Felt clearer to menathanvegdahlmarked this conversation as resolved`@ -1661,0 +1673,4 @@`

`}`

`/**`

`* Clamps an angle between min and max. ...`

We can definitely document this better, ha ha. Maybe this:

Clamps an angle between

`min`

and`max`

.All angles are in radians.

This function treats angles as existing in a looping (cyclic) space, and is therefore specifically

notequivalent to a simple`clamp(angle, min, max)`

.`min`

and`max`

are treated as a directed range on the unit circle and`angle`

is treated as a point on the unit circle.`angle`

is then clamped to be within the directed range defined by`min`

and`max`

.nathanvegdahlmarked this conversation as resolved`@ -1661,0 +1695,4 @@`

`/* Some shenanigans to handle the various special cases of`

`* clamping in a cyclic space. */`

`float tclamped = 0.0;`

`if (tmin < tmax) {`

I think these cases can be better commented if they're split up to return early. And as I did that, I just generally cleaned up the function a bit (sans the

`min_relative`

renames and such). I also discovered that`std::clamp`

exists:I changed the name of

`tmin`

and`tmax`

to`min_wrapped`

and`max_wrapped`

I also inverted the "zero outside of the range" condition and instead return

`angle`

earlyAlso I renamed

`pi_wrap`

->`wrap_rad_angle`

nathanvegdahlmarked this conversation as resolvedIn theory yes, but I'd like to not do that in this PR to not go down any rabbit holes

Hm I don't see why not, @nathanvegdahl do you have any reservations about that?

Nope, that seems fine to me.

Also, I'm realizing I got a little over-obsessed with getting this ready to merge. Personally, I really want to see this merged. But @dr.sybren prodded me in-person about double checking that we

shouldmerge it.I think this behavior is obviously better than the old behavior, and frankly the old behavior seems buggy to me. But the question is: are there any rigs that depend on the old behavior, and is it worth risking breakage there?

My suspicion is that people are unlikely to be

knowinglydepending on the old behavior, but it's possible they might beaccidentallydepending on it (e.g. someone getting a rig setup working by just playing with stuff until it seems to work, rather than really understanding what's going on).So I'm going to add this to module meeting agenda for today to discuss.

`@ -1661,0 +1714,4 @@`

`if (std::fabs(max_wrapped) < std::fabs(min_wrapped)) {`

`return angle + max_wrapped;`

`}`

`return angle + min_wrapped;`

I'm specifically

notasking for a change here, since this follows Blender's style guidelines. Just a personal note: to me this is a good example of a case where the "no else after return" rule actually makes code less clear. We have have two alternate cases here that conceptually are on the same level and symmetric with each other, but that's no longer clearly reflected in the structure of the code. Alas.I know and I feel the same, but I just was corrected on this recently....

nathanvegdahlmarked this conversation as resolvedblender-v4.1-releasetomainnow targeting main since it was discussed in yesterdays Animation & Rigging module meeting that we shouldn't land this that late in the 4.1 release cycle

Added soft limits of -360 to 360 to the properties

`@ -2658,2 +2661,3 @@`

`RNA_def_property_range(prop, -1000.0, 1000.0f);`

`RNA_def_property_ui_text(prop, "Maximum X", "Highest X value to allow");`

`RNA_def_property_ui_range(prop, -2 * M_PI, 2 * M_PI, 10.0f, 1.0f);`

`RNA_def_property_ui_text(prop, "Maximum X", "Higher X angle bound");`

Nit: instead of "higher" I think "upper" is the more common word to use when talking about bounds.

Just left one nit that you can change while landing. Otherwise looks good to me!

@blender-bot build

ed2408400dintomainfix_limit_rot_constraint