Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Aabb2 rotation broken #153

Closed
ArgentiApparatus opened this issue Oct 11, 2015 · 7 comments
Closed

Aabb2 rotation broken #153

ArgentiApparatus opened this issue Oct 11, 2015 · 7 comments

Comments

@ArgentiApparatus
Copy link
Contributor

Aabb2 z = new Aabb2.minMax(new Vector2(1.0, 1.0), new Vector2(2.0, 2.0));

print(z.min); // [1.0,1.0]
print(z.max); // [2.0,2.0]

z.rotate(new Matrix3.rotationZ((PI/2))); // Rotate 90 degrees

print(z.min); // still [1.0,1.0]
print(z.max); // still [2.0,2.0]

I believe this is because an absolute rotation is being applied to min and max separately.

@johnmccutchan
Copy link
Collaborator

Hi,

It would be great if you could send a pull request with the fix and a unit test to make sure we don't regress!

Thanks,
John

@ArgentiApparatus
Copy link
Contributor Author

I'd like to write a fix, I'll see what I can do with my available time.

I see there is a pull request #136 for Aabb2, which presumably implements rotations differently. I have not tested that code to see if it fixes this problem.

I understand that you would like to move to the center + half-extents internal representation that #136 provides, but this involves removal of the min and max setters (replaced with setMinMax()) which is a breaking change (I don't see why the min and max setters need to be removed though).

What are you thoughts on accepting #136? Would it be better to develop ceronman's implementation of Aabb2?

@Fox32
Copy link
Contributor

Fox32 commented Oct 13, 2015

@ArgentiApparatus Silly question, but what behavior do you expect from the Aabb2.rotate method? As your Aabb2 is one by one in size, shouldn't it remain the same if you rotate by a multiple of 90° around the z axis? Or would you expect its center to move, then Aabb2.transform should be the right choice.

Right now the behavior is the following:

  Aabb2 z = new Aabb2.minMax(new Vector2(1.0, 1.0), new Vector2(2.0, 3.0));

  print(z.min); // [1.0,1.0]
  print(z.max); // [2.0,2.0]

  z.rotate(new Matrix3.rotationZ((PI/2))); // Rotate 90 degrees

  print(z.min); // [0.5,1.5]
  print(z.max); // [2.5,2.5]

This means it applies the rotation to the box with the rotation axis at the center of the box. For me it's what I would expect from the method, for more complex things I would use Aabb2.transform.

@ArgentiApparatus
Copy link
Contributor Author

Ah, this may be my understanding then. I expected Aabb,rotate() to implementation rotation around an arbitary point, in my example the origin, so I would expect the result:

print(z.min); // [-2.0,1.0]
print(z.max); // [-1.0,2.0]

I think I thought that because Aabb,rotate() takes a Matrix3.

@Fox32
Copy link
Contributor

Fox32 commented Oct 13, 2015

I didn't wrote the method and never used it. But that is the only explanation I have ;-)
But in that case transform() should fit your needs?

In case it's not a bug, the function still needs better documentation...

@ArgentiApparatus
Copy link
Contributor Author

I've looked into Aabb.rotate(Matrix3) and Aabb2.transform(Matrix3) a bit more.to see what they are intended to do.

Aabb.rotate(Matrix3) applies the rotation encoded into input Matrix3 to the half-extents vector. That seems correct now that I think about it more.

Aabb2.transform(Matrix3) applies the transform to the center vector - obviously only the translation has any effect - then applies the rotation.

I'm not entirely sure that is completely correct behavior. What if the transform included a scaling or shear transformation? Shouldn't that be applied to the half-extents vector?

I note that Aabb.rotate() is based on Matrix3.absoluteRotate2(Vector2) There is a problem with this method. (AlsoMatrix3.absoluteRotate(Vector3).)

I'm not sure what the effect of 'absolute rotation on a vector' is supposed to be. Rotate the vector then take absolute value? Rotate through angle % π/2?

Matrix3.absoluteRotate2(Vector2) just takes absolute values of the matrix elements that apply rotation.

Vector2 absoluteRotate2(Vector2 arg) {
  double m00 = _m3storage[0].abs();
  double m01 = _m3storage[3].abs();
  double m10 = _m3storage[1].abs();
  double m11 = _m3storage[4].abs();
  ...

If you apply this to [x, 0], [-x, 0], [0, x] or [0, -x] it applies a rotation that 'folds back' at +/- π/2, so I suppose that was what 'absolute rotation' was intended to be. The effects are weird if one coordinate is not zero though.

I'd be happy to work up a pull request if we can get some questions answered:

  • I think the fix for Aabb.rotate(Matrix3) is to simply rotate the half-extents vector then take the absolute value.
  • I'm not sure what to do with Aabb2.transform(Matrix3). What do you think the behavior should be with respect to scaling and shear?
  • What should the behavior for Matrix3.absoluteRotate2(Vector2) and Matrix3.absoluteRotate(Vector3) be?

I'd like some options for Aabb.rotate():

  • Aabb.rotate3(Matrix3)
  • Aabb.rotate2(Matrix2)
  • Aabb.rotate(double)

But that would a breaking change. Perhaps a separate pull request for version 2?

BTW, what are the plans for a version 2 release?

@ArgentiApparatus
Copy link
Contributor Author

Can I get some opinions / answers to the questions posed above? I'd really like to create PR but want to make sure I'm doing the right thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants