Fix #117927: Limit rotation constraint flipping #118502

Christoph Lendenfeld · 2024-02-20T13:52:53+01:00

2024-02-20 13:52:53 +01:00

The issue from the bug report mentions that the Limit Rotation constraint snaps
back 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.

min max of 0 / 360
angle of 185

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!

The issue from the bug report mentions that the `Limit Rotation` constraint snaps back 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. * min max of 0 / 360 * angle of 185 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!**

🎉 1

Christoph Lendenfeld added the

Module

Animation & Rigging

label 2024-02-20 13:52:53 +01:00

Christoph Lendenfeld added 1 commit 2024-02-20 13:53:05 +01:00

buildbot/vexp-code-patch-lint Build done. Details

buildbot/vexp-code-patch-darwin-arm64 Build done. Details

buildbot/vexp-code-patch-darwin-x86_64 Build done. Details

buildbot/vexp-code-patch-linux-x86_64 Build done. Details

buildbot/vexp-code-patch-windows-amd64 Build done. Details

buildbot/vexp-code-patch-coordinator Build done. Details

ced21c6739 the fix

Christoph Lendenfeld commented

2024-02-20 13:53:34 +01:00

@blender-bot package

Blender Bot commented

2024-02-20 13:53:39 +01:00

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR118502) when ready.

Demeter Dzadik commented

2024-02-20 15:19:43 +01:00

Thanks for the fix, tested!

Setting a limit of 190 and then rotating a bone with the transform operator results in it looping over from 180 to -180 as usual, and then stopping at -170, which is the best equivalent to +190 that we're gonna get out of the transform operator, so this is what is expected. Fair enough.
Setting the limits to a range of exactly 360 (eg. min 0 max 360) locks the bone in place, which is a bit counter-intuitive, but can be fixed by nudging either value so it's not the biggest deal ever. I wonder if it's a matter of replacing an => with a > or vice versa?

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.

For a limit constraint like the original reporter's use case, it would be more useful if one day we could limit the actual transform properties themselves. There's no way to do this in Blender, even though custom properties do have customizable hard limits, so this feels like it should be possible too.
The current method of limiting transforms with a Limit Rotation constraint can be bypassed via the N panel. That also means the "Affect Transforms" option of the limit constraints isn't really doing what it says. (It only applies the limit when using the transform operator.)
The fact that the transform operator always uses matrix mult also means that setting a limit of 0-360 is meaningless, because:
- The transform operator can't comprehend greater than pos/neg half circle rotation anyways
- You're left with having to input your desired rotation via the N panel, which is not affected by the limits.

Thanks for the fix, tested! - Setting a limit of 190 and then rotating a bone with the transform operator results in it looping over from 180 to -180 as usual, and then stopping at -170, which is the best equivalent to +190 that we're gonna get out of the transform operator, so this is what is expected. Fair enough. - Setting the limits to a range of exactly 360 (eg. min 0 max 360) locks the bone in place, which is a bit counter-intuitive, but can be fixed by nudging either value so it's not the biggest deal ever. I wonder if it's a matter of replacing an => with a > or vice versa? ------------- 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. - For a limit constraint like the original reporter's use case, it would be more useful if one day we could limit the actual transform properties themselves. There's no way to do this in Blender, even though custom properties do have customizable hard limits, so this feels like it should be possible too. - The current method of limiting transforms with a Limit Rotation constraint can be bypassed via the N panel. That also means the "Affect Transforms" option of the limit constraints isn't really doing what it says. (It only applies the limit when using the transform operator.) - The fact that the transform operator always uses matrix mult also means that setting a limit of 0-360 is meaningless, because: - The transform operator can't comprehend greater than pos/neg half circle rotation anyways - You're left with having to input your desired rotation via the N panel, which is not affected by the limits.

Nathan Vegdahl requested changes 2024-02-22 11:58:28 +01:00

Dismissed

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

						
				@ -1661,0 +1663,4 @@

				 * considered a valid range.*/

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

				{

				  if (min == max) {

Nathan Vegdahl commented

2024-02-22 11:49:53 +01:00

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.

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.

Christoph Lendenfeld commented

2024-02-22 12:19:32 +01:00

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.

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

Nathan Vegdahl commented

2024-02-22 12:47:40 +01:00

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.

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.

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.

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.

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

Christoph Lendenfeld commented

2024-02-22 14:14:36 +01:00

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

Changed that now

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

👍 1

nathanvegdahl marked this conversation as resolved

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

						
				@ -1661,0 +1669,4 @@

				  float angle_unit_circle[2];

				  angle_unit_circle[0] = cos(angle);

				  angle_unit_circle[1] = sin(angle);

Nathan Vegdahl commented

2024-02-22 11:46:36 +01:00

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

Christoph Lendenfeld commented

2024-02-22 12:11:08 +01:00

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.

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.

Nathan Vegdahl commented

2024-02-22 12:14:58 +01:00

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

Christoph Lendenfeld commented

2024-02-22 13:25:48 +01:00

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

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

Nathan Vegdahl commented

2024-02-26 12:49:51 +01:00

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.

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.

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 1 commit 2024-02-22 14:14:00 +01:00

548d7a90f2 don't constrain if max<=min or range>2pi

Christoph Lendenfeld requested review from Nathan Vegdahl 2024-02-22 14:14:43 +01:00

Nathan Vegdahl reviewed 2024-02-23 10:43:21 +01:00

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

						
				@ -1661,0 +1668,4 @@

				  }

				  if (max <= min) {

				    return angle;

Nathan Vegdahl commented

2024-02-23 10:43:21 +01:00

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.

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.

Christoph Lendenfeld commented

2024-02-23 12:26:54 +01:00

ah yeah you are right

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 1 commit 2024-02-23 12:23:58 +01:00

b70d15e7ea Merge branch 'blender-v4.1-release' into fix_limit_rot_constraint

Christoph Lendenfeld added 1 commit 2024-02-23 12:26:26 +01:00

8b7e074058 clamp to min

Demeter Dzadik commented

2024-02-26 13:00:56 +01:00

Extra polish suggestions unless they're already in there:

Can the constraint either display a warning when its configured in an invalid way, or be prevented from being configured in an invalid way, at least in the UI? Eg. the Scene Start/End frame values nudge each other using update callbacks. Min/max here could do the same.
Also if it can't support values outside of some range (I guess -360 to +360?), then those min/max values should just be set for the properties too, imo.

Extra polish suggestions unless they're already in there: - Can the constraint either display a warning when its configured in an invalid way, or be prevented from being configured in an invalid way, at least in the UI? Eg. the Scene Start/End frame values nudge each other using update callbacks. Min/max here could do the same. - Also if it can't support values outside of some range (I guess -360 to +360?), then those min/max values should just be set for the properties too, imo.

Nathan Vegdahl commented

2024-02-27 11:19:15 +01:00

@ChrisLend Here's a purely angle-based implementation of clamp_angle() you can test out:

#include <cmath>
#include <algorithm>

/**
 * Wraps a number to be in [-PI, +PI].
 *
 * Can also be implemented as `std::remainder(a, M_PI * 2.0)`.
 * Not sure which is faster.
 */
static float pi_wrap(const float a) {
  const float b = a * (0.5 / M_PI) + 0.5;
  const float c = b - std::floor(b);
  return (c - 0.5) * (2.0 * M_PI);
}

/**
 * Clamps an angle between min and max. ...
 */
static float clamp_angle(const float angle, const float min, const float max)
{
  if ((max - min) >= (2 * M_PI)) {
    return angle;
  }

  if (max <= min) {
    return min;
  }

  /* For the code below, see: https://www.desmos.com/calculator/awzjzund1u */

  /* Move min and max into a space where `angle == 0.0`, and wrap them to
   * [-PI, +PI] in that space. */
  const float tmin = pi_wrap(min - angle);
  const float tmax = pi_wrap(max - angle);

  /* Some shenanigans to handle the various special cases of
   * clamping in a cyclic space. */
  float tclamped = 0.0;
  if (tmin < tmax) {
    tclamped = std::min(std::max(0.0f, tmin), tmax);
  } else if (tmax < 0.0 && 0.0 < tmin) {
    if (std::fabs(tmax) < std::fabs(tmin)) {
      tclamped = tmax;
    } else {
      tclamped = tmin;
    }
  }

  return tclamped + angle;
}

@ChrisLend Here's a purely angle-based implementation of `clamp_angle()` you can test out: ```c #include <cmath> #include <algorithm> /** * Wraps a number to be in [-PI, +PI]. * * Can also be implemented as `std::remainder(a, M_PI * 2.0)`. * Not sure which is faster. */ static float pi_wrap(const float a) { const float b = a * (0.5 / M_PI) + 0.5; const float c = b - std::floor(b); return (c - 0.5) * (2.0 * M_PI); } /** * Clamps an angle between min and max. ... */ static float clamp_angle(const float angle, const float min, const float max) { if ((max - min) >= (2 * M_PI)) { return angle; } if (max <= min) { return min; } /* For the code below, see: https://www.desmos.com/calculator/awzjzund1u */ /* Move min and max into a space where `angle == 0.0`, and wrap them to * [-PI, +PI] in that space. */ const float tmin = pi_wrap(min - angle); const float tmax = pi_wrap(max - angle); /* Some shenanigans to handle the various special cases of * clamping in a cyclic space. */ float tclamped = 0.0; if (tmin < tmax) { tclamped = std::min(std::max(0.0f, tmin), tmax); } else if (tmax < 0.0 && 0.0 < tmin) { if (std::fabs(tmax) < std::fabs(tmin)) { tclamped = tmax; } else { tclamped = tmin; } } return tclamped + angle; } ```

Christoph Lendenfeld added 2 commits 2024-02-27 16:49:06 +01:00

fed69e65b5 Merge branch 'blender-v4.1-release' into fix_limit_rot_constraint

ecbadc0e22 use nathans code

Christoph Lendenfeld commented

2024-02-27 16:51:44 +01:00

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

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

👍 1

Nathan Vegdahl commented

2024-02-27 16:58:55 +01:00

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

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

😆 1

Nathan Vegdahl requested changes 2024-02-27 20:24:44 +01:00

Dismissed

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

						
				@ -1661,0 +1662,4 @@

				/**

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

				 *

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

Nathan Vegdahl commented

2024-02-27 17:00:24 +01:00

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.

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.

Christoph Lendenfeld commented

2024-02-29 12:18:15 +01:00

I changed the argument name to angle
For b I am not sure if there is a good name

I changed the argument name to `angle` For `b` I am not sure if there is a good name

👍 1

Nathan Vegdahl commented

2024-02-29 16:00:12 +01:00

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

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

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

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

Nathan Vegdahl commented

2024-02-27 17:02:52 +01:00

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.

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.

Christoph Lendenfeld commented

2024-02-29 12:17:31 +01:00

used wrap_rad_angle. Felt clearer to me

used `wrap_rad_angle`. Felt clearer to me

👍 1

nathanvegdahl marked this conversation as resolved

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

						
				@ -1661,0 +1673,4 @@

				}

				/**

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

Nathan Vegdahl commented

2024-02-27 17:20:06 +01:00

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 not equivalent 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.

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 **not** equivalent 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`.

nathanvegdahl marked this conversation as resolved

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

						
				@ -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) {

Nathan Vegdahl commented

2024-02-27 20:24:39 +01:00

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:

static float clamp_angle(const float angle, const float min, const float max)
{
  if ((max - min) >= (2 * M_PI)) {
    return angle;
  }

  if (max <= min) {
    return min;
  }

  /* For the code below, see: https://www.desmos.com/calculator/awzjzund1u */

  /* Move min and max into a space where `angle == 0.0`, and wrap them to
   * [-PI, +PI] in that space.  This simplifies the cases below, as we can
   * just use 0.0 in place of `angle` and know that everything is in
   * [-PI, +PI]. */
  const float tmin = pi_wrap(min - angle);
  const float tmax = pi_wrap(max - angle);


  /* If the range defined by `min`/`max` doesn't contain the boundary at
   * PI/-PI.  This is the simple case, because it means we can do a simple
   * clamp. */
  if (tmin < tmax) {
    return angle + std::clamp(0.0f, tmin, tmax);
  }

  /* Now for the cases where the PI/-PI boundary *is* contained. */

  /* If zero is outside of the range, we clamp to the closest of tmin/tmax. */
  if (tmax < 0.0 && 0.0 < tmin) {
    if (std::fabs(tmax) < std::fabs(tmin)) {
      return angle + tmax;
    }
    else {
      return angle + tmin;
    }
  }

  /* Zero is inside the range, so there's no clamping. */
  return angle;
}

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: ```c static float clamp_angle(const float angle, const float min, const float max) { if ((max - min) >= (2 * M_PI)) { return angle; } if (max <= min) { return min; } /* For the code below, see: https://www.desmos.com/calculator/awzjzund1u */ /* Move min and max into a space where `angle == 0.0`, and wrap them to * [-PI, +PI] in that space. This simplifies the cases below, as we can * just use 0.0 in place of `angle` and know that everything is in * [-PI, +PI]. */ const float tmin = pi_wrap(min - angle); const float tmax = pi_wrap(max - angle); /* If the range defined by `min`/`max` doesn't contain the boundary at * PI/-PI. This is the simple case, because it means we can do a simple * clamp. */ if (tmin < tmax) { return angle + std::clamp(0.0f, tmin, tmax); } /* Now for the cases where the PI/-PI boundary *is* contained. */ /* If zero is outside of the range, we clamp to the closest of tmin/tmax. */ if (tmax < 0.0 && 0.0 < tmin) { if (std::fabs(tmax) < std::fabs(tmin)) { return angle + tmax; } else { return angle + tmin; } } /* Zero is inside the range, so there's no clamping. */ return angle; } ```

Christoph Lendenfeld commented

2024-02-29 12:16:03 +01:00

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 early

Also I renamed pi_wrap -> wrap_rad_angle

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` early Also I renamed `pi_wrap` -> `wrap_rad_angle`

👍 1

nathanvegdahl marked this conversation as resolved

Christoph Lendenfeld added 1 commit 2024-02-29 12:13:36 +01:00

976beda1ee comments and code simplification

Christoph Lendenfeld requested review from Nathan Vegdahl 2024-02-29 12:18:22 +01:00

Christoph Lendenfeld commented

2024-02-29 13:17:25 +01:00

Can the constraint either display a warning when its configured in an invalid way, or be prevented from being configured in an invalid way, at least in the UI? Eg. the Scene Start/End frame values nudge each other using update callbacks. Min/max here could do the same.

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

Also if it can't support values outside of some range (I guess -360 to +360?), then those min/max values should just be set for the properties too, imo.

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

> - Can the constraint either display a warning when its configured in an invalid way, or be prevented from being configured in an invalid way, at least in the UI? Eg. the Scene Start/End frame values nudge each other using update callbacks. Min/max here could do the same. In theory yes, but I'd like to not do that in this PR to not go down any rabbit holes > - Also if it can't support values outside of some range (I guess -360 to +360?), then those min/max values should just be set for the properties too, imo. Hm I don't see why not, @nathanvegdahl do you have any reservations about that?

Nathan Vegdahl commented

2024-02-29 16:13:29 +01:00

@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 should merge 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 knowingly depending on the old behavior, but it's possible they might be accidentally depending 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.

> @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 *should* merge 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 *knowingly* depending on the old behavior, but it's possible they might be *accidentally* depending 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.

👍 1

Nathan Vegdahl reviewed 2024-02-29 16:22:54 +01:00

source/blender/blenkernel/intern/constraint.cc

						
				@ -1661,0 +1714,4 @@

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

				    return angle + max_wrapped;

				  }

				  return angle + min_wrapped;

Nathan Vegdahl commented

2024-02-29 16:22:54 +01:00

I'm specifically not asking 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'm specifically *not* asking 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.

Christoph Lendenfeld commented

2024-02-29 16:34:37 +01:00

I know and I feel the same, but I just was corrected on this recently....

👍 1

nathanvegdahl marked this conversation as resolved

ChrisLend changed target branch from blender-v4.1-release to main

2024-03-01 10:18:21 +01:00

Christoph Lendenfeld commented

2024-03-01 10:21:42 +01:00

now 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

now targeting main since it was discussed in yesterdays [Animation & Rigging](https://devtalk.blender.org/t/2024-02-29-animation-rigging-module-meeting/33614#patches-review-decision-time-5) module meeting that we shouldn't land this that late in the 4.1 release cycle

Christoph Lendenfeld added 1 commit 2024-03-01 12:20:30 +01:00

8ddd9e2b1e Merge branch 'main' into fix_limit_rot_constraint

Christoph Lendenfeld added 2 commits 2024-03-01 12:37:42 +01:00

4ced0e146b remove timing code

9e63b195c5 set soft limits

Christoph Lendenfeld commented

2024-03-01 12:38:03 +01:00

Added soft limits of -360 to 360 to the properties

Nathan Vegdahl reviewed 2024-04-04 15:08:11 +02:00

source/blender/makesrna/intern/rna_constraint.cc Outdated

						
				@ -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");