blender
/
blender
136
407
Fork 589
Code Issues 6.3k Pull Requests 767 Projects 19 Wiki Activity

Fix #117927: Limit rotation constraint flipping #118502

Merged
Christoph Lendenfeld merged 12 commits from ChrisLend/blender:fix_limit_rot_constraint into main 2024-04-04 16:31:08 +02:00
Conversation 12 Commits 12 Files Changed 2 +73 -27
2 changed files with 73 additions and 27 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

82
source/blender/blenkernel/intern/constraint.cc
View File

@ -1659,6 +1659,64 @@ static bConstraintTypeInfo CTI_LOCLIMIT = {
/* -------- Limit Rotation --------- */
/**
* Wraps a number to be in [-PI, +PI].
*/
static inline float wrap_rad_angle(const float angle)
Nathan Vegdahl commented 2024-02-27 17:00:24 +01:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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.
{
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-02-22 11:49:53 +01:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
const float b = angle * (0.5 / M_PI) + 0.5;
return ((b - std::floor(b)) - 0.5) * (2.0 * M_PI);
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-02-27 17:02:52 +01:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

used wrap_rad_angle. Felt clearer to me

used `wrap_rad_angle`. Felt clearer to me
👍 1
}
/**
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-02-23 10:43:21 +01:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

ah yeah you are right

ah yeah you are right
* Clamps an angle between min and max.
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-02-22 11:46:36 +01:00
Outdated
Review
Copy Link

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

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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

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
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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.
*
* All angles are in radians.
*
* This function treats angles as existing in a looping (cyclic) space, and is therefore
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-02-27 17:20:06 +01:00
Outdated
Review
Copy Link

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`.
* 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`.
*/
static float clamp_angle(const float angle, const float min, const float max)
{
/* If the allowed range exceeds 360 degrees no clamping can occur. */
if ((max - min) >= (2 * M_PI)) {
return angle;
}
/* Invalid case, just return min. */
if (max <= min) {
return min;
}
/* 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 min_wrapped = wrap_rad_angle(min - angle);
const float max_wrapped = wrap_rad_angle(max - angle);
nathanvegdahl marked this conversation as resolved Outdated
Nathan Vegdahl commented 2024-02-27 20:24:39 +01:00
Outdated
Review
Copy Link

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
Outdated
Review
Copy Link

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
/* 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 (min_wrapped < max_wrapped) {
return angle + std::clamp(0.0f, min_wrapped, max_wrapped);
}
/* At this point we know that `min_wrapped` >= `max_wrapped`, meaning the boundary is crossed.
* With that we know that no clamping is needed in the following case. */
if (max_wrapped >= 0.0 || min_wrapped <= 0.0) {
return angle;
}
/* If zero is outside of the range, we clamp to the closest of `min_wrapped` or `max_wrapped`. */
if (std::fabs(max_wrapped) < std::fabs(min_wrapped)) {
return angle + max_wrapped;
}
return angle + min_wrapped;
nathanvegdahl marked this conversation as resolved
Nathan Vegdahl commented 2024-02-29 16:22:54 +01:00
Review
Copy Link

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
Review
Copy Link

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

I know and I feel the same, but I just was corrected on this recently....
👍 1
}
static void rotlimit_evaluate(bConstraint *con, bConstraintOb *cob, ListBase * /*targets*/)
{
bRotLimitConstraint *data = static_cast<bRotLimitConstraint *>(con->data);
@ -1693,31 +1751,13 @@ static void rotlimit_evaluate(bConstraint *con, bConstraintOb *cob, ListBase * /
/* limiting of euler values... */
if (data->flag & LIMIT_XROT) {
if (eul[0] < data->xmin) {
eul[0] = data->xmin;
}
if (eul[0] > data->xmax) {
eul[0] = data->xmax;
}
eul[0] = clamp_angle(eul[0], data->xmin, data->xmax);
}
if (data->flag & LIMIT_YROT) {
if (eul[1] < data->ymin) {
eul[1] = data->ymin;
}
if (eul[1] > data->ymax) {
eul[1] = data->ymax;
}
eul[1] = clamp_angle(eul[1], data->ymin, data->ymax);
}
if (data->flag & LIMIT_ZROT) {
if (eul[2] < data->zmin) {
eul[2] = data->zmin;
}
if (eul[2] > data->zmax) {
eul[2] = data->zmax;
}
eul[2] = clamp_angle(eul[2], data->zmin, data->zmax);
}
loc_eulO_size_to_mat4(cob->matrix, loc, eul, size, rot_order);

18
source/blender/makesrna/intern/rna_constraint.cc
View File

@ -2639,37 +2639,43 @@ static void rna_def_constraint_rotation_limit(BlenderRNA *brna)
prop = RNA_def_property(srna, "min_x", PROP_FLOAT, PROP_ANGLE);
RNA_def_property_float_sdna(prop, nullptr, "xmin");
RNA_def_property_range(prop, -1000.0, 1000.0f);
RNA_def_property_ui_text(prop, "Minimum X", "Lowest 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, "Minimum X", "Lower X angle bound");
RNA_def_property_update(prop, NC_OBJECT | ND_CONSTRAINT, "rna_Constraint_update");
prop = RNA_def_property(srna, "min_y", PROP_FLOAT, PROP_ANGLE);
RNA_def_property_float_sdna(prop, nullptr, "ymin");
RNA_def_property_range(prop, -1000.0, 1000.0f);
RNA_def_property_ui_text(prop, "Minimum Y", "Lowest Y 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, "Minimum Y", "Lower Y angle bound");
RNA_def_property_update(prop, NC_OBJECT | ND_CONSTRAINT, "rna_Constraint_update");
prop = RNA_def_property(srna, "min_z", PROP_FLOAT, PROP_ANGLE);
RNA_def_property_float_sdna(prop, nullptr, "zmin");
RNA_def_property_range(prop, -1000.0, 1000.0f);
RNA_def_property_ui_text(prop, "Minimum Z", "Lowest Z 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, "Minimum Z", "Lower Z angle bound");
RNA_def_property_update(prop, NC_OBJECT | ND_CONSTRAINT, "rna_Constraint_update");
prop = RNA_def_property(srna, "max_x", PROP_FLOAT, PROP_ANGLE);
RNA_def_property_float_sdna(prop, nullptr, "xmax");
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);
Nathan Vegdahl commented 2024-04-04 15:08:11 +02:00
Outdated
Review
Copy Link

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

Nit: instead of "higher" I think "upper" is the more common word to use when talking about bounds.
RNA_def_property_ui_text(prop, "Maximum X", "Upper X angle bound");
RNA_def_property_update(prop, NC_OBJECT | ND_CONSTRAINT, "rna_Constraint_update");
prop = RNA_def_property(srna, "max_y", PROP_FLOAT, PROP_ANGLE);
RNA_def_property_float_sdna(prop, nullptr, "ymax");
RNA_def_property_range(prop, -1000.0, 1000.0f);
RNA_def_property_ui_text(prop, "Maximum Y", "Highest Y 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 Y", "Upper Y angle bound");
RNA_def_property_update(prop, NC_OBJECT | ND_CONSTRAINT, "rna_Constraint_update");
prop = RNA_def_property(srna, "max_z", PROP_FLOAT, PROP_ANGLE);
RNA_def_property_float_sdna(prop, nullptr, "zmax");
RNA_def_property_range(prop, -1000.0, 1000.0f);
RNA_def_property_ui_text(prop, "Maximum Z", "Highest Z 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 Z", "Upper Z angle bound");
RNA_def_property_update(prop, NC_OBJECT | ND_CONSTRAINT, "rna_Constraint_update");
prop = RNA_def_property(srna, "euler_order", PROP_ENUM, PROP_NONE);