#290: Adding missing methods.

[halzy]

Adding to Basis:

• invert()
• inverse()
• transpose()
• transposed()
• flip_x()
• flip_y()
• flip_z()

[halzy]

Looking for a touch of feedback before getting too far into the changes. Is this the right direction? I've yet to add tests to compare with the functions from Godot.

I'm not sure if flip_x/y/z() are expected to be like this, or if they are useful.

[ghost]

Thanks for the PR! I thought FLIP_* are constants in the GDScript interface, so maybe they can be implemented as associated constants if you want to add them?

impl Basis {
    ///  The basis that will flip something along the X axis when used in a transformation.
    pub const FLIP_X: Basis = Basis {
        elements: [
            Vector3 { /* .. */ },
            Vector3 { /* .. */ },
            Vector3 { /* .. */ },
        ],
    };

    // - snip -
}

It's annoying that you can't use Vector3 constructors here because const fns aren't stabilized yet.

[ghost]

However, identity is also implemented as a function, so functions could work as well. I don't think I'm particularly favoring either of them to the other.

[halzy]

Added:

• determinant()
• get_euler()
• get_rotation_quat()
• is_equal_approx()
• orthonormalized()
• scaled()
• xform()
• xform_inv()

Not sure on the need for

• get_orthogonal_index(), seems to be used by the editor.

Still needed:

• get_scale()
• rotated()
• slerp()

[ghost]

On the clippy errors: approx wss added to current master as an optional dependency. If you need approximate float comparisons, consider rebasing on master and making approx unconditional.

[ghost]

Thanks for putting in the work! Just read through the code, and found a few classes of problems that probably resulted from my expression being imprecise. My apologizes! I left review comments on a few instances, but not all of them because that will clutter up the view. To clarify:

Generally, when adding extra methods on core types:

• Unlike in generated bindings, where we have to use the original names, it's not required to match the Godot method / argument names exactly. If there's a better, Rust-y name that can be considered, use that instead.
• You may feel free to expose useful methods that aren't present on the Godot types.
• If you can afford to use a regular assertion, do not use a debug assertion.
• It's better to be explicit about the costs of method calls. Take self if you're going to Copy or Clone it anyway.
• Add doc-comments to methods, not normal comments.

I understand that this is a lot of changes to do, along with the sanity tests that need to be added. There is no need to hurry, though! Even if the PR misses 0.8.1, it can still get into future versions.

[halzy]

Thank you for your feedback! I'll go through it today.

I've added some sanity tests. The one for invert() is commented out, only 5 of the 9 values are calculated correctly. I've had extra eyes on it and we haven't figured out where it is breaking. Could you take a look or suggest someone that could help?

Thanks!

[ghost]

I will take a look later!

As an aside, please don't work around the CI by commenting out code and tests. Having broken code in comments accidentally merged can complicate maintenance later. If you want to get other methods merged first, take the code out and put it in a new PR instead.

[ghost]

You might have seen other commented code in the repo while working on it, but keep in mind that these are relics from the early days. We don't do that anymore.

[ghost]

Sorry for the long comments, but apparently some of the issues were missed the last time, so I've left review comments on each of the cases this time. Please make sure to check the comments folded by GitHub as well.

[halzy]

@toasteater Thank you for the detailed review! Updated and squashed.

[ghost]

Looks good to me. Thanks!