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 math interop features to bevy_math #5270

Closed
wants to merge 1 commit into from
Closed

Add math interop features to bevy_math #5270

wants to merge 1 commit into from

Conversation

heavyrain266
Copy link

Objective

This is mostly useful for plugin authors when external libraries are using e.g. cgmath, vek and other popular linear algebra crates.

For example in bevy_fbx we have to duplicate glam as dependency only to add mint feature for converting fbxcel-dom PointX types into glam's FooVecX used by bevy_math.

Solution

  • Enable "mint" feature in bevy_math

Changelog

  • Add "mint" feature in bevy_math to interop between math libraries

Migration Guide

Not required

@nicopap
Copy link
Contributor

nicopap commented Jul 10, 2022

See nicopap/bevy_mod_fbx#2

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I don't really like this implementation. Presently, we don't depend on mint at all, so this is just adding more compile time for no benefit for all current users.

I wouldn't mind adding a feature to bevy_math which just forwards to this feature, however. That way, you don't need to care which version of glam bevy uses internally.

As a side note, I'm not sure why this doesn't change Cargo.lock - it should be adding new dependencies.

@tim-blackbird
Copy link
Contributor

@DJMcNab There is no Cargo.lock in the repo. It's ignored.

@Nilirad Nilirad added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Math Fundamental domain-agnostic mathematical operations X-Controversial There is active debate or serious implications around merging this PR labels Jul 10, 2022
@mockersf
Copy link
Member

I don't think this should be added to Bevy. Definitely not by default, and as feature are additives it doesn't help that much / is less explicit to add a feature for it in Bevy than to let others add the feature for glam themselves

@heavyrain266
Copy link
Author

Current workaround is to add glam as dependency in Cargo.toml with same version as used by bevy and then add mint feature which "patches" it instead of duplicating deps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants