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

Add determinant to Transform2D #770

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ughuuu
Copy link

@Ughuuu Ughuuu commented Jun 16, 2024

Would be nice to have this function. It's also available and public in Godot, as can be seen here: Transform2D. Also, the function can be used to check for Transform2D invalid so that you don't call eg. affine_inverse on an invalid matrix and get an assert.

@Ughuuu Ughuuu changed the title Add determinant to Transform 2D Add determinant to Basis2D Jun 16, 2024
@Ughuuu Ughuuu changed the title Add determinant to Basis2D Add determinant to Transform2D Jun 16, 2024
@Ughuuu Ughuuu force-pushed the expose-determinant-in-2d-transform branch from 7473618 to fdffe7f Compare June 16, 2024 19:18
@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels Jun 16, 2024
@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-770

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

itest/rust/src/builtin_tests/geometry/transform2d_test.rs Outdated Show resolved Hide resolved
itest/rust/src/builtin_tests/geometry/transform2d_test.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/transform2d.rs Outdated Show resolved Hide resolved
@Ughuuu Ughuuu requested a review from Bromeon June 19, 2024 09:04
@Ughuuu
Copy link
Author

Ughuuu commented Jun 21, 2024

Enabled the actions on my fork to see how to fix the CI. will write here when I have it passing.

@Ughuuu
Copy link
Author

Ughuuu commented Jun 21, 2024

Hm, not sure why the test fails. Any idea how to run it locally?

@lilizoey
Copy link
Member

lilizoey commented Jun 21, 2024

Hm, not sure why the test fails. Any idea how to run it locally?

Since it's a 4.0 godot test failing, the best way i think is to:

  1. Grab a 4.0 godot binary
  2. Run cargo build --p itest --features="godot/api-custom" with GODOT4_BIN pointing to the 4.0 godot binary
  3. Go into itest/godot
  4. Run godot4 --headless

It may be possible to use ./check.sh itest with GODOT4_BIN pointing to the right binary, but i've found the above more reliable. but feel free to try it

@Bromeon
Copy link
Member

Bromeon commented Jun 22, 2024

It's not necessary to run on Godot 4.0.

The error is very clear:

error[E0599]: no method named `determinant` found for struct `InnerTransform2D` in the current scope
  --> itest/rust/src/builtin_tests/geometry/transform2d_test.rs:44:30
   |
44 |         real::from_f64(inner.determinant()),
   |                              ^^^^^^^^^^^ method not found in `InnerTransform2D<'_>`

Then, navigate to Godot 4 Transform2D docs to confirm.
And you'll see that indeed, determinant() is missing. I has only been added later.


I'm fine to just add it for newer Godot versions. You can do this with #[cfg(since_api = "4.1")], as the method is available from 4.1.

Please also squash your commits and make sure there's a newline at the end of the .gitignore.

@Ughuuu Ughuuu force-pushed the expose-determinant-in-2d-transform branch from 47202e2 to 52e0ddd Compare June 22, 2024 08:50
@Ughuuu
Copy link
Author

Ughuuu commented Jun 22, 2024

Oh, so that was it, thanks, thats perfect. Didn't think it would be that simple, thought I had some configuration issue.

Update transform2d.rs

update again, fix error

expose the determinant for Transform2D

Update transform2d_test.rs

lint

Update .gitignore

fix test

update feedback

Update transform2d_test.rs

Update .gitignore

Co-Authored-By: Jan Haller <[email protected]>
@Ughuuu Ughuuu force-pushed the expose-determinant-in-2d-transform branch from 52e0ddd to 8573e6f Compare June 22, 2024 08:54
@@ -23,3 +23,6 @@ gen
*.bat
config.toml
exp.rs

# Mac specific
.DS_Store
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI you can add this to your ~/.gitconfig to avoid needing to add this to every project's .gitignore

[core]
	excludesfile = /home/<USER>/.gitignore

Copy link
Contributor

Choose a reason for hiding this comment

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

And then populate ~/.gitignore with the relevant ignores

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants