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

Resolve Inconsistency in Matrix3 and Matrix4 rotateY Implementations #317

Merged
merged 2 commits into from
Mar 6, 2024

Conversation

tlserver
Copy link
Contributor

@tlserver tlserver commented Mar 5, 2024

This PR addresses the issues highlighted in #261 and is related to the discussions in #262 and #300, as well as the historical context provided by #69.

In reference to the conversation in #262, two primary concerns were identified:

  1. The change introduced test failures.
  2. There was an inconsistency between the rotateY methods in the Matrix3 and Matrix4 classes.

Here's a summary of the remedial actions taken in this PR:

This PR is similar to #262. The only difference is that this PR also includes a negated rotation angle in the obb3 test.

For issue (1), the test expectations have been updated to reflect the correct signs, ensuring that all tests now pass.

Regarding issue (2), upon further investigation, it was found that the Matrix4 class already had the correct implementation since August 23, 2014. However, the Matrix3 class has not been updated to match this, leading to a longstanding inconsistency between the two. This PR rectifies the Matrix3 rotateY method to align with the Matrix4 implementation, bringing them into consistency.

  // Matrix3
  /// Turns the matrix into a rotation of [radians] around Y
  void setRotationY(double radians) {
    final c = math.cos(radians);
    final s = math.sin(radians);
    _m3storage[0] = c;
    _m3storage[1] = 0.0;
    _m3storage[2] = -s;
    _m3storage[3] = 0.0;
    _m3storage[4] = 1.0;
    _m3storage[5] = 0.0;
    _m3storage[6] = s;
    _m3storage[7] = 0.0;
    _m3storage[8] = c;
  }
  
  // Matrix4
  /// Sets the upper 3x3 to a rotation of [radians] around Y
  void setRotationY(double radians) {
    final c = math.cos(radians);
    final s = math.sin(radians);
    _m4storage[0] = c;
    _m4storage[1] = 0.0;
    _m4storage[2] = -s;
    _m4storage[4] = 0.0;
    _m4storage[5] = 1.0;
    _m4storage[6] = 0.0;
    _m4storage[8] = s;
    _m4storage[9] = 0.0;
    _m4storage[10] = c;
    _m4storage[3] = 0.0;
    _m4storage[7] = 0.0;
    _m4storage[11] = 0.0;
  }

This PR is similar to #262. The only difference is that this PR also includes a negated rotation angle in the obb3 test.

By merging this PR, we will resolve the inconsistencies and adhere to the mathematical definitions of rotation matrices, as expected by the users of the vector_math.dart library.

I look forward to your feedback and to further improving the library.

Copy link
Collaborator

@spydon spydon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm, I think this is quite important to land now when 3D is more and more used in Flutter

Comment on lines +202 to +222
void testMatrix3RotationX() {
final rotX = Matrix3.rotationX(math.pi / 2);
final input = Vector3(0.0, 1.0, 0.0);

relativeTest(rotX.transformed(input), Vector3(0.0, 0.0, 1.0));
}

void testMatrix3RotationY() {
final rotY = Matrix3.rotationY(math.pi / 2);
final input = Vector3(0.0, 0.0, 1.0);

relativeTest(rotY.transformed(input), Vector3(1.0, 0.0, 0.0));
}

void testMatrix3RotationZ() {
final rotZ = Matrix3.rotationZ(math.pi / 2);
final input = Vector3(1.0, 0.0, 0.0);

relativeTest(rotZ.transformed(input), Vector3(0.0, 1.0, 0.0));
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we have the same tests in Matrix4?

@johnmccutchan
Copy link
Collaborator

Please update this PR to include a change log entry and then I'll merge it.

Thanks for the fixes!

@johnmccutchan johnmccutchan merged commit 1ed8ac6 into google:master Mar 6, 2024
3 checks passed
@coveralls
Copy link

Coverage Status

coverage: 26.33%. remained the same
when pulling 5a190ff on tlserver:issue261
into 3706feb on google:master.

copybara-service bot pushed a commit to dart-lang/sdk that referenced this pull request Mar 7, 2024
…h, web, webdriver, webkit_inspection_protocol

Revisions updated by `dart tools/rev_sdk_deps.dart`.

dartdoc (https://github.com/dart-lang/dartdoc/compare/cec45fb..0de8aff):
  0de8aff3  2024-03-06  Sam Rawlins  Generate docs for enum static methods. (dart-lang/dartdoc#3697)
  9eafdc64  2024-03-06  Sam Rawlins  Rewrite Library.allOriginalModelElementNames. (dart-lang/dartdoc#3698)
  e8f36333  2024-03-06  Sam Rawlins  Fix SDK GitHub action with some stub headers and footers (dart-lang/dartdoc#3704)
  58c065d4  2024-03-05  Sam Rawlins  Bump snippets activated to 0.4.3 (dart-lang/dartdoc#3702)

http (https://github.com/dart-lang/http/compare/470d2c3..8d3c647):
  8d3c647  2024-03-06  Brian Quinlan  Add support for negotiating a subprotocol (dart-lang/http#1150)
  e71e739  2024-03-05  Brian Quinlan  Add `WebSocket.connect` as a cross-platform connection method (dart-lang/http#1149)
  f14b5aa  2024-03-04  Brian Quinlan  Include a description and version number in web_socket pubspec (dart-lang/http#1148)
  5b656a9  2024-03-04  Brian Quinlan  Add a LICENSE file to package:web_socket (dart-lang/http#1147)
  557c420  2024-03-04  Brian Quinlan  Implement WebSocket for the browser (dart-lang/http#1142)

markdown (https://github.com/dart-lang/markdown/compare/dd47c5d..1ca5166):
  1ca5166  2024-03-06  Devon Carew  fix a crash when parsing alert block syntax (dart-lang/markdown#593)

package_config (https://github.com/dart-lang/package_config/compare/4a7042b..3d90e69):
  3d90e69  2024-03-05  Michael Thomsen  Fix typo (dart-lang/package_config#149)

shelf (https://github.com/dart-lang/shelf/compare/da6a69b..1acbc67):
  1acbc67  2024-03-06  Andy  Add shelf_router middleware examples (dart-lang/shelf#417)

vector_math (https://github.com/google/vector_math.dart/compare/3706feb..7e705f7):
  7e705f7  2024-03-06  6y  Fix quaternion negate (google/vector_math.dart#316)
  1ed8ac6  2024-03-06  6y  Resolve Inconsistency in Matrix3 and Matrix4 `rotateY` Implementations (google/vector_math.dart#317)

web (https://github.com/dart-lang/web/compare/8870d04..51e594b):
  51e594b  2024-03-05  Srujan Gaddam  Fix dictionary constructors to accept supertype members and create an empty object when there are no fields (dart-lang/web#197)
  4af904f  2024-03-05  Srujan Gaddam  Publish 0.5.1 (dart-lang/web#196)
  c72ec1a  2024-03-04  Devon Carew  add instructions for re-generating the package (dart-lang/web#195)

webdriver (https://github.com/google/webdriver.dart/compare/2c1b6f8..73a7ac8):
  73a7ac8  2024-03-04  dependabot[bot]  Bump nanasess/setup-chromedriver from 2.2.1 to 2.2.2 (google/webdriver.dart#294)

webkit_inspection_protocol (https://github.com/google/webkit_inspection_protocol.dart/compare/07295b9..153fea4):
  153fea4  2024-03-04  dependabot[bot]  Bump nanasess/setup-chromedriver (google/webkit_inspection_protocol.dart#120)

Change-Id: Ic213677a1e2430a6de56a94e0bfaa1f33e2fc7d4
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/356300
Auto-Submit: Devon Carew <devoncarew@google.com>
Reviewed-by: Konstantin Shcheglov <scheglov@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
@tlserver
Copy link
Contributor Author

tlserver commented Mar 11, 2024

@johnmccutchan

Thank you for merging the PR! Could you please provide an estimated timeline for when version 2.1.5 will be published to include this fix?
Also, #261 can be closed.

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants