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

[Merged by Bors] - Expose mint feature in bevy_math/glam #5857

Closed
wants to merge 4 commits into from

Conversation

harudagondi
Copy link
Member

@harudagondi harudagondi commented Sep 2, 2022

Objective

Solution

  • Added features in bevy_math, bevy_internal, bevy
  • Updated docs/cargo_features.md

Changelog

Added

  • mint feature in bevy_math to allow interoperation of glam types with mint-compatible libraries.

@harudagondi
Copy link
Member Author

This is different from #5270 as this is an opt-in feature.

@mockersf
Copy link
Member

mockersf commented Sep 2, 2022

It would make more sense to me for bevy_oddio to enable the feature on their side.

@mockersf mockersf added the A-Math Fundamental domain-agnostic mathematical operations label Sep 2, 2022
@DJMcNab
Copy link
Member

DJMcNab commented Sep 2, 2022

It would make more sense to me for bevy_oddio to enable the feature on their side.

I don't think that's right, since making users manually keep their glam version up to date seems not optimal.

Why not just expose all of the glam features in bevy_math

I don't think we should filter this feature all the way up to the root - it should be bevy_math exclusive.

@harudagondi
Copy link
Member Author

It would make more sense to me for bevy_oddio to enable the feature on their side.

I'm using Vec3 internally. Also, trying to enable something like "bevy_math/glam/mint", I get this error:

error: failed to parse manifest at `C:\<path to>\bevy_oddio\Cargo.toml`

Caused by:
  feature `bevy_math/glam/mint` in dependency `bevy` is not allowed to contain slashes
  If you want to enable features of a transitive dependency, the direct dependency needs to re-export those features from the `[features]` table.

@mockersf
Copy link
Member

mockersf commented Sep 2, 2022

What DJMcNab is suggesting is exposing a mint feature (and other glam features) on bevy_math but not bubbling it up to bevy, which I would prefer 👍

I worry about the multiplication of features exposed by Bevy, this suggestion would fix it

@harudagondi
Copy link
Member Author

but not bubbling it up to bevy

Sorry if this is wrong, as I am not really familiar with how rust features work. If my library (e.g. bevy_oddio) directly uses bevy as a dependency, how would I enable mint for bevy_math as a feature? Do I need to reimport bevy_math (with the mint feature) along with bevy?

@mockersf
Copy link
Member

mockersf commented Sep 2, 2022

Sorry if this is wrong, as I am not really familiar with how rust features work.

It is not wrong, it's just that features in Bevy requires some work/maintenance due to how the crates are organised. As you saw in that PR you need to modify three crates to expose a feature.
In this case, I feel that mint compatibility is not so much a feature of Bevy as a feature of the math layer, that's why it feel not good to me to expose it on Bevy directly. Others may feel differently and it would be OK to them.

If my library (e.g. bevy_oddio) directly uses bevy as a dependency, how would I enable mint for bevy_math as a feature? Do I need to reimport bevy_math (with the mint feature) along with bevy?

Yup, you would have as dependency:

bevy = { version = "0.8", default-features = false, features = ["..."] }
bevy_math = { version = "0.8", features = ["mint"] }

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

This strategy looks good to me now.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Sep 2, 2022
@alice-i-cecile
Copy link
Member

@harudagondi I'll merge this once a) merge conflicts are resolved and b) you add the new line :)

harudagondi and others added 2 commits September 3, 2022 09:57
Co-authored-by: François <mockersf@gmail.com>
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 3, 2022
# Objective

- Expose `mint` feature of `glam` in `bevy_math`.
- Unblocks harudagondi/bevy_oddio#22
	- [`oddio::SpatialOptions`] uses mint types

[`oddio::SpatialOptions`]: https://docs.rs/oddio/latest/oddio/struct.SpatialOptions.html

## Solution

- Added features in `bevy_math`, ~`bevy_internal`, `bevy`~
- ~Updated `docs/cargo_features.md`~

---

## Changelog

### Added

- `mint` feature in `bevy_math` to allow interoperation of glam types with mint-compatible libraries.
@bors bors bot changed the title Expose mint feature in bevy_math/glam [Merged by Bors] - Expose mint feature in bevy_math/glam Sep 3, 2022
@bors bors bot closed this Sep 3, 2022
@harudagondi harudagondi deleted the expose-mint branch September 23, 2022 00:43
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

- Expose `mint` feature of `glam` in `bevy_math`.
- Unblocks harudagondi/bevy_oddio#22
	- [`oddio::SpatialOptions`] uses mint types

[`oddio::SpatialOptions`]: https://docs.rs/oddio/latest/oddio/struct.SpatialOptions.html

## Solution

- Added features in `bevy_math`, ~`bevy_internal`, `bevy`~
- ~Updated `docs/cargo_features.md`~

---

## Changelog

### Added

- `mint` feature in `bevy_math` to allow interoperation of glam types with mint-compatible libraries.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Expose `mint` feature of `glam` in `bevy_math`.
- Unblocks harudagondi/bevy_oddio#22
	- [`oddio::SpatialOptions`] uses mint types

[`oddio::SpatialOptions`]: https://docs.rs/oddio/latest/oddio/struct.SpatialOptions.html

## Solution

- Added features in `bevy_math`, ~`bevy_internal`, `bevy`~
- ~Updated `docs/cargo_features.md`~

---

## Changelog

### Added

- `mint` feature in `bevy_math` to allow interoperation of glam types with mint-compatible libraries.
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants