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

Adding Transform3D operations - fixes issues/290 #898

Merged
merged 3 commits into from
Oct 18, 2022

Conversation

Hexorg
Copy link
Contributor

@Hexorg Hexorg commented Jun 14, 2022

I wanted to get more people's eyes on this. Everything but the linear interpolation unit test works. The problem with linear interpolation is that godot seems to do it differently..? I opened up godot source and I swear I've implemented it the same, yet the engine gives me a different interpolation result for the same inputs.

@Bromeon Bromeon added feature Adds functionality to the library c: core Component: core (mod core_types, object, log, init, ...) labels Jun 14, 2022
@Bromeon Bromeon added this to the v0.10.1 milestone Jun 14, 2022
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.

Thank you very much for this valuable PR! 👍

It looks very good, I added some smaller comments. Thanks also for adding tests!

The Godot source code is here; I also don't see an immediate difference to yours 🤔 we had something like this in the past, and it turned out there was a bug in one of the other components (could e.g. be Quat).

If you run cargo fmt on the code base, it will automatically format everything and make the rustfmt GitHub Action happy 🙂
To run the tests and clippy locally, there's also a script in the project root:

sh check.sh test clippy

gdnative-core/src/core_types/geom/basis.rs Show resolved Hide resolved
gdnative-core/src/core_types/geom/transform.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/geom/transform.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/geom/basis.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/geom/basis.rs Outdated Show resolved Hide resolved
@Bromeon Bromeon linked an issue Jun 14, 2022 that may be closed by this pull request
gdnative-core/src/core_types/geom/transform.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/geom/transform.rs Outdated Show resolved Hide resolved
gdnative-core/src/core_types/geom/transform.rs Outdated Show resolved Hide resolved
@@ -87,6 +87,9 @@ impl Transform {
}

/// Returns this transform, with its origin moved by a certain `translation`
#[deprecated = "`translated` is not relative to the transform's coordinate system \
and thus inconsistent with GDScript. Please use translated2() instead. \
This method will be removed in gdnative 0.11."]
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't enough time to take a closer look, but isn't that exactly what we wanted to fix for a very long time in godotengine/godot#55923

Currently GDScript definitely has a major inconsistency between translated on the one hand and rotated + scaled on the other hand, see godotengine/godot-proposals#1336. Basically everyone in the discussion is on the same page that we want to resolve it, but the direction is which to resolve was discussed to death, which is why the PR is stuck at the moment. I still hope to get it resolved one way or the other, so I'm not sure if it is very crucial to stick to the status quo of GDScript in this case due to the inconsistency.

Copy link
Member

Choose a reason for hiding this comment

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

@bluenote10 I even upvoted that issue a while ago, thanks a lot for reminding me in this context!

I'm totally with you in that case -- we should probably keep the current behavior and not deprecate anything. However, this discrepancy from GDScript (at least in Godot 3) should be documented.

What do you think about adding a method transform_global which implements the current GDScript way (would that even be the right name?)

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you think about adding a method transform_global which implements the current GDScript way (would that even be the right name?)

I think it was the other way around: scaled + rotated + translated should have global semantics (and no suffix in the name) and the local variants all get a _local suffix, because that's how Basis already does it. But I'm a bit short on time and without actually looking into it again I may be getting it wrong now ;) For the desired target state I'd refer to code in godotengine/godot#55923

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, godotengine/godot#55923 has now been merged! 🚀

@Bromeon Bromeon modified the milestones: v0.10.1, v0.10.2 Jul 16, 2022
@Hexorg
Copy link
Contributor Author

Hexorg commented Jul 27, 2022

Ok, so I follow @bluenote10 points, but @Bromeon wanted to maintain backward-compatibility with previous release for now. So I'm calling it translated_global() for now. And in 0.11 I propose to rename translated_global() to translated() and current translated() to translated_local().

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 for the implementation!

Maybe once you feel it's ready, you could squash the commits to 1 or a handful (where each commit contains related changes).

#[inline]
pub fn orthogonalize(&mut self) {
let scale = self.scale();
self.orthonormalize();
self.scale_local(scale);
}
Copy link
Member

Choose a reason for hiding this comment

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

The reasoning mentioned for transpose() applies here, too; please make this method private.

Comment on lines 121 to 137
pub fn inverse(&self) -> Self {
let basis_inv = self.basis.transposed();
let origin_inv = basis_inv.xform(-self.origin);

Self {
basis: basis_inv,
origin: origin_inv,
Transform {
basis: self.basis.transposed(),
origin: self.basis.xform(-self.origin),
}
Copy link
Member

Choose a reason for hiding this comment

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

This changes semantics; origin has now value
self.basis.xform(-self.origin) instead of
self.basis.transposed().xform(-self.origin).

Is that intended?

Comment on lines 189 to 212
/// In-place translates the transform by the given offset, relative to
/// the transform's basis vectors.
#[inline]
fn translate_withbasis(&mut self, translation: Vector3) {
// Note: Godot source uses origin + basis dot translation,
// but self.translate() uses only origin + translation
self.origin.x += self.basis.elements[0].dot(translation);
self.origin.y += self.basis.elements[1].dot(translation);
self.origin.z += self.basis.elements[2].dot(translation);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should this not be named translate_global, as it's used as-is in translated_global()?

/// Interpolates the transform to other Transform by
/// weight amount (on the range of 0.0 to 1.0).
#[inline]
pub fn interpolate_with(&self, other: &Transform, weight: f32) -> Self {
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should just call this method lerp and the sphere one slerp, for consistency with others.

Godot is a mess here; there are 3 namings:

  • lerp + slerp for simple types
  • linear_interpolate for vectors and colors (now renamed to lerp + slerp)
    • Vector3 also has cubic_interpolate and bezier_interpolate, which might explain the origins
  • interpolate_with for transforms; in Godot 4 also sphere_interpolate_with

I'm aware that we currently also have Transform2D::interpolate_with(), but maybe we can deprecate and rename that. In an ideal world, all those would be called lerp and slerp.

Comment on lines 253 to 256
#[inline]
fn lerp_f(from: f32, to: f32, weight: f32) -> f32 {
from + (to - from) * weight
}
Copy link
Member

Choose a reason for hiding this comment

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

Comment on lines 260 to 274
result.elements[0] *= lerp_f(
self.elements[0].length(),
other.elements[0].length(),
weight,
);
result.elements[1] *= lerp_f(
self.elements[1].length(),
other.elements[1].length(),
weight,
);
result.elements[2] *= lerp_f(
self.elements[2].length(),
other.elements[2].length(),
weight,
);
Copy link
Member

Choose a reason for hiding this comment

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

Loop?

@Bromeon Bromeon modified the milestones: v0.10.1, v0.10.2 Sep 3, 2022
@Bromeon
Copy link
Member

Bromeon commented Sep 3, 2022

Postponed to v0.10.2 🔜

@Bromeon
Copy link
Member

Bromeon commented Oct 1, 2022

@Hexorg any feedback regarding my suggestions?

@Bromeon Bromeon modified the milestones: v0.10.2, v0.11.x Oct 1, 2022
@Bromeon
Copy link
Member

Bromeon commented Oct 18, 2022

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 18, 2022

Build succeeded:

@bors bors bot merged commit 553bf87 into godot-rust:master Oct 18, 2022
@chitoyuu chitoyuu modified the milestones: v0.11.x, v0.11.1 Dec 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Component: core (mod core_types, object, log, init, ...) feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement missing Basis and Transform methods
4 participants