-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Create bevy_label
crate
#18059
base: main
Are you sure you want to change the base?
Create bevy_label
crate
#18059
Conversation
#17713 lacks motivation apart from "it can be done". What advantages to you expect from this? |
There was a problem hiding this 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:
- We should polish this crate further, potentially trying to surpass
internment
and makebevy_label
a fully standalone crate (e.g.,disqualified
). Or; - 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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. |
Also, discursion moves faster as a PR that as an issue, the issue existed for 3 weeks with no discursion whatsoever |
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 👍 |
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. |
Objective
Closes #17713
Solution
Create a crate
bevy_label
and update re-exports ofDynEq
onbevy_ecs
,bevy_app
, andbevy_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
tobevy_label
andbevy_ecs::intern
tobevy_label::intern