Skip to content

[discussion] Moving away from euclid in favor of local types #594

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

Closed
ghost opened this issue Sep 20, 2020 · 8 comments · Fixed by #713
Closed

[discussion] Moving away from euclid in favor of local types #594

ghost opened this issue Sep 20, 2020 · 8 comments · Fixed by #713
Labels
breaking-change Issues and PRs that are breaking to fix/merge.
Milestone

Comments

@ghost
Copy link

ghost commented Sep 20, 2020

On multiple occasions, there were users complaining on GitHub and Discord about how euclid's typed interface, with its distinction between Points, Vectors, Sizes and Scales, gets in the way when the Godot API itself is untyped. While there is the simple solution of conversion (among euclid types, or through mint to types from another library), new users often aren't aware of them, and the method calls can get quite verbose at times. There is also the issue with porting, where someone will attempt to port some GDScript code to Rust, and want to access the Godot versions of vector methods. We're currently handling those with extension traits, but local types can do that a little bit better.

The use of euclid types in the public interface also caused version conflict problems in user code, leading to recurring PRs to bump them: #216, #523, #588

Therefore, I think it might be valuable to eventually replace euclid types with local newtypes in our public interface instead. This is not a suggestion that we should reinvent the wheel: we can just pick any vector math library we want to use as the underlying implementation, as long as its types have the correct layout. For conversion to types from other libraries, we can add (optional?) mint support.

This is obviously too late for inclusion in 0.9, but we can consider it for 0.10.

@ghost ghost added the breaking-change Issues and PRs that are breaking to fix/merge. label Sep 20, 2020
@ghost ghost added this to the 0.10 milestone Sep 20, 2020
@Demindiro
Copy link
Contributor

Has anyone started working on this yet? If not, I'm willing to give it a go.

@Bromeon
Copy link
Member

Bromeon commented Mar 15, 2021

Thanks for offering help! Before delving straight into an implementation, we should probably discuss the

This is not a suggestion that we should reinvent the wheel: we can just pick any vector math library we want to use as the underlying implementation, as long as its types have the correct layout.

part 🙂

@ghost
Copy link
Author

ghost commented Mar 17, 2021

True. I think glam can be an option given that it directly advertises repr(C): https://docs.rs/glam/0.13.0/glam/index.html, but I haven't explored its API enough yet. I'm not sure if it suits our use cases enough.

Other popular libraries include nalgebra, but repr(C) does not seem to be guaranteed, and we don't really need generics for our use case.

@Bromeon
Copy link
Member

Bromeon commented Mar 17, 2021

Just quickly made a test: glam has no required dependencies, a cargo new crate with it compiles in 9s.
nalgebra has 13 indirect dependencies, compiles in 1m 20s.

Huge difference, and I'm definitely a fan of lightweight libraries 🙂 it also seems to be specifically for game development, not a general-purpose linear algebra package.


What would the general approach be?

  1. We would offer "front-end" vector types

     #[repr(C)]
     pub struct Vector3 {    
         pub x: f32,
         pub y: f32,
         pub z: f32,
     }
    
     impl Vector3 {    
         // map Godot API 1:1
     }

    and then "memcpy" them to glam::Vec3 everywhere and back.
    Could even use mint::Vector3<f32>, but not sure if the generics contribute a lot (maybe feature-gate).

  2. The local newtypes you mentioned

    pub struct Vector3(glam::Vec3);
    
    impl Vector3 {    
        // map Godot API 1:1
    }

    This would avoid conversion, but there's no more direct access to the fields.

  3. Use selected glam types directly in the API. Would be easiest for people already using glam, but repeats the problems with euclid (versioning, different function names).

@ghost
Copy link
Author

ghost commented Mar 17, 2021

Indeed, I wasn't considering compile time in my previous comment. If we take that into consideration then glam is definitely more competitive here.

offer "front-end" vector types

Good idea! Direct field access is definitely something good to have. We can have private methods for value and reference conversion to glam types and delegate any appropriate calls to the library. rustc should be able to optimize all of them out.

Use selected glam types directly in the API. Would be easiest for people already using glam, but repeats the problems with euclid (versioning, different function names).

Indeed. That's what we're trying not to do here.

@Bromeon
Copy link
Member

Bromeon commented Mar 17, 2021

Sounds good! Since vector types are used across the entire library, such a refactoring is quite an endeavour. Maybe it would make sense to split the efforts into steps, with intermediate working versions?

I could imagine something like this:

  1. Only change the definitions, not the usages.
    This would mean adding glam, removing euclid and defining our own type, with methods that have the same names as the euclid ones. We could even leave the Vector*Godot traits for the time being.
    Could be done just by starting with an struct with zero methods, read compile errors and implement the missing methods using glam conversions.

  2. Rename methods
    After everything still works, all methods can be renamed to fit the Godot counterparts.
    There might be a few manual interventions where the signature differs, but for the most part I'd expect a good IDE to do all the work here.

  3. Port missing Godot methods
    There are probably methods which are part of GDScript's API, but not Rust's.
    They can be implemented.

Also, the above parts could be done first for Vector2 as a proof-of-concept, and then for Vector3 (or vice versa).

Then @Demindiro would also be more free to choose if he still wants to work on it, and if yes on what parts.

@Bromeon
Copy link
Member

Bromeon commented Mar 17, 2021

Thinking about this a bit more, if we provide our own types, we still need to deal with a lot of code duplication just for delegating (just from the operators alone). It would be almost as much as providing our own implementation, since most vector operations are one-liners. But if it helps for clarity and consistency with Godot, it's probably worth it.

Regarding back-end math library, there are a few contenders:

There's also nalgebra, nalgebra_glm, vek and quite a few others, but on first sight they don't seem to have a "simplicity first" philosophy, and a lot of the available crates get lost in advanced mathematics irrelevant to gamedev.

Also, arewegameyet provides an overview of crates, and mathbench-rs some benchmarks. I think the API and "bloatedness" is more decisive for our use case, but having good performance is definitely a nice to have.

@ghost
Copy link
Author

ghost commented Mar 19, 2021

ultraviolet seems to be focused on SIMD, which is a bit removed from our use case. It's also not clear whether the types are repr(C) without extra alignment, a deal-breaker in our case.

Since Demindiro has made a PR based on glam I think it's fine to go with it for now. We can switch to another implementation any time later.

Thinking about this a bit more, if we provide our own types, we still need to deal with a lot of code duplication just for delegating (just from the operators alone). It would be almost as much as providing our own implementation, since most vector operations are one-liners. But if it helps for clarity and consistency with Godot, it's probably worth it.

While I still prefer nominal type systems for most use cases, this is one of the niches where a structural typing system would really shine. Without a "standard" implementation widely agreed upon, we're bound to repeat the old problem if we want to provide at least some ease of use. If we ignore that though, there's the ggez way of only providing mint types (that don't support any operations on their own).

bors bot added a commit that referenced this issue Mar 30, 2021
713: Replace euclid with glam (proof-of-concept) r=toasteater a=Demindiro

I had some time to take a look at #594. I decided to go with `glam` since it compiles the fastest.

I went with the "front-end" approach as I believe that is the most flexible approach.

I did rename a few things (like `zero()` to `ZERO`, `quaternion()` to `new()`, ...) but that is easy to revert if required.

It doesn't seem like many changes are required. Wrapping all the methods in `glam` is easy to do with extensive use of `glam()` and `into()`. The crate does seem to miss implementations for a few types and methods like `Rect2` and `rotated`, so we'll have to implement those from scratch.

the `impl Mul/Div/Add/...`s for `Vector2` and `Vector3` are pretty much identical, maybe there is some way to reduce duplication?

Closes #594
Closes #670 (`Vector3` is fully implemented)

Co-authored-by: David Hoppenbrouwers <[email protected]>
Co-authored-by: toasteater <[email protected]>
@bors bors bot closed this as completed in 3ad9b65 Mar 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change Issues and PRs that are breaking to fix/merge.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants