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

Fix C# operator *(Transform3D, Aabb) #97208

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

Conversation

kleonc
Copy link
Member

@kleonc kleonc commented Sep 19, 2024

The current C# implemention of static Aabb operator *(Transform3D transform, Aabb aabb) (added in #64729) is pretty-much a copy-paste of the core/C++ implementation of AABB Transform3D::xform(const AABB &p_aabb). The issue is that such implemenation uses Basis[int] operator and:

  • in core/C++ this operator returns Basis row,
  • in GDscript/C# this operator returns Basis column (axis).

This discrepancy was not taken into account, hence incorrect calculations. This PR fixes that.

Fixes #97197.

@kleonc kleonc added bug topic:dotnet cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Sep 19, 2024
@kleonc kleonc added this to the 4.4 milestone Sep 19, 2024
@kleonc kleonc requested review from a team as code owners September 19, 2024 22:24
Comment on lines -44 to +48
_FORCE_INLINE_ const Vector3 &operator[](int p_axis) const {
return rows[p_axis];
_FORCE_INLINE_ const Vector3 &operator[](int p_row) const {
return rows[p_row];
}
_FORCE_INLINE_ Vector3 &operator[](int p_axis) {
return rows[p_axis];
_FORCE_INLINE_ Vector3 &operator[](int p_row) {
return rows[p_row];
Copy link
Member Author

Choose a reason for hiding this comment

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

p_axis is confusing in there, AFAICT everywhere else in there axis refers to column.

@kleonc kleonc changed the title Fix C# operator *(Transform3D, AABB) Fix C# operator *(Transform3D, Aabb) Sep 19, 2024
Copy link
Member

@aaronfranke aaronfranke left a comment

Choose a reason for hiding this comment

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

This is correct. The confusing thing is that (as the PR description states) in Godot's C++ code, the [] operator uses rows, while in GDScript and C#, the [] operator uses columns. The rename of p_axis to p_row helps too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release topic:dotnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Transform3D Aabb operator * does not handle rotations properly [C#]
2 participants