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

Use correct matrix ordering coventions for CFrame #103

Merged

Conversation

kennethloeffler
Copy link
Contributor

@kennethloeffler kennethloeffler commented Sep 17, 2023

Closes #99.

This PR transposes the orientation matrix in a few different places:

  • The CFrame.new(x, y, z, r00, r01, r02, r10, r11, r12, r20, r21, r22) constructor
  • CFrame:GetComponents (also removes the negation on the position's Z component, which I thought was fine to include in this PR - let me know if you'd like it separate!)
  • CFrame's Display implementation (this makes the output have identical ordering to GetComponents)
  • CFrame's From<DomCFrame> implementation
  • DomCFrame's From<CFrame> implementation

I'd like to have more test coverage to prevent regressions in the future, particularly for ensuring that values are correct when (de)serializing places/models, but I'm not sure if I should be doing that in the CFrame datatype test suite... let me know and I can add them!

@kennethloeffler kennethloeffler marked this pull request as ready for review September 17, 2023 21:47
Copy link
Collaborator

@filiptibell filiptibell left a comment

Choose a reason for hiding this comment

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

Changes look great, I think more lua tests would be great too. The CFrame.luau test file is getting pretty large, so we can probably split that out into something like:

> CFrame (dir with luau files inside)
--> test utils
--> constructors
--> instance interactions
--> transforms
--> ...

If you're cool with doing that in this PR too I'll leave it completely up to you how to best structure that and how much to expand the test suite, above is just a sketch

src/roblox/datatypes/types/cframe.rs Outdated Show resolved Hide resolved
@kennethloeffler
Copy link
Contributor Author

If you're cool with doing that in this PR too I'll leave it completely up to you how to best structure that and how much to expand the test suite, above is just a sketch

I think I'd like to save this for another PR since it's rather out of scope for this one. Convince yourself that the implementation is correct here and it should be good to merge

@filiptibell
Copy link
Collaborator

I think I'd like to save this for another PR since it's rather out of scope for this one. Convince yourself that the implementation is correct here and it should be good to merge

Fine with me - I'll go ahead and merge this and we can improve the test suite later

@filiptibell filiptibell merged commit 1dfffdc into lune-org:main Sep 18, 2023
3 of 4 checks passed
@kennethloeffler kennethloeffler deleted the cframe-use-correct-convention branch September 18, 2023 02:54
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.

Lune's CFrame does not always follow the correct ordering convention
2 participants