Skip to content

Conversation

@hukasu
Copy link
Contributor

@hukasu hukasu commented Feb 26, 2025

Objective

Closes #17713

Solution

Create a crate bevy_label and update re-exports of DynEq on bevy_ecs, bevy_app, and bevy_render.

Testing

cargo run -p ci
cargo check -p bevy_ecs --no-default-features --features edge_executor,critical-section,bevy_debug_stepping,bevy_reflect --target x86_64-unknown-none

Migration Guide

Moved bevy_ecs::label to bevy_label and bevy_ecs::intern to bevy_label::intern

  • Merge conflicts

@mockersf
Copy link
Member

#17713 lacks motivation apart from "it can be done". What advantages to you expect from this?

Copy link
Contributor

@bushrat011899 bushrat011899 left a comment

Choose a reason for hiding this comment

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

Documentation and use of std need to be changed IMO.

Regarding whether we should do this: the most popular internment crate, internment, currently has zero support for no_std, but is otherwise mature and widely used in the Rust ecosystem already. Either:

  1. We should polish this crate further, potentially trying to surpass internment and make bevy_label a fully standalone crate (e.g., disqualified). Or;
  2. We should abandon Bevy's interning solution and improve internment based on our needs (e.g., no_std support).

I do think there is merit in trying to remove the labelling system from bevy_ecs; it's an already very large crate with a lot of responsibilities across a wide range of domains. Anything which can be split out without sacrificing DX should be pursued by virtue of making bevy_ecs itself easier to maintain. The sheer quantity of items in bevy_ecs which are pub(crate) alone makes it extremely daunting to make breaking changes unless you're well versed in how the whole crate works.

@@ -1,5 +1,14 @@
//! Traits used by label implementations
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're going to pull this out, this documentation should be far more fleshed out with some examples as to how people are supposed to use this crate.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that i will leave open until a decision is reached on whether we will continue with this or change to internment, so maybe add the Needs docs label so that we don't forget

@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change A-Utils Utility functions and types X-Controversial There is active debate or serious implications around merging this PR labels Feb 27, 2025
@alice-i-cecile
Copy link
Member

I think my preference here is to spin out the functionality into its own Bevy-org crate, or improve an existing one enough that we can pull a dependency on it. This is useful to the broader ecosystem, loosely coupled and pretty stable.

@hukasu
Copy link
Contributor Author

hukasu commented Feb 27, 2025

#17713 lacks motivation apart from "it can be done". What advantages to you expect from this?

@ItsDoot can you answer that?

@hukasu
Copy link
Contributor Author

hukasu commented Feb 27, 2025

Also, discursion moves faster as a PR that as an issue, the issue existed for 3 weeks with no discursion whatsoever

@ItsDoot
Copy link
Contributor

ItsDoot commented Feb 27, 2025

I do think there is merit in trying to remove the labelling system from bevy_ecs; it's an already very large crate with a lot of responsibilities across a wide range of domains. Anything which can be split out without sacrificing DX should be pursued by virtue of making bevy_ecs itself easier to maintain. The sheer quantity of items in bevy_ecs which are pub(crate) alone makes it extremely daunting to make breaking changes unless you're well versed in how the whole crate works.

FWIW, this^ is the motivation I was thinking of when making the issue. Whether that is worthy-enough motivation to split it is up to someone else 👍

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 27, 2025

Whether that is worthy-enough motivation to split it is up to someone else 👍

Someone else here: that's worthy enough motivation to me ;) We're also reusing similar stuff across rendering, and may see this pattern crop-up elsewhere in the engine.

@bushrat011899
Copy link
Contributor

As an FYI, internment has accepted my no_std PR, which opens up the possibility of outsourcing that aspect of labels entirely.

@janhohenheim
Copy link
Member

Triage: has merge conflicts
@hukasu do you want to update this PR or should I tag it S-Needs-Adoption? :)

@janhohenheim janhohenheim added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 17, 2025
@hukasu
Copy link
Contributor Author

hukasu commented May 17, 2025

I can update

@janhohenheim janhohenheim added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 19, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-Utils Utility functions and types C-Code-Quality A section of code that is hard to understand or change S-Needs-Review Needs reviewer attention (from anyone!) to move forward 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.

Pull bevy_ecs::intern out into its own separate crate

6 participants