-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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
Remove the ability to use strings as labels #4409
Remove the ability to use strings as labels #4409
Conversation
I think I'm onboard with this change. Can you flesh out the example in the migration guide section, so that we can see exactly what the extra boilerplate over using strings would entail? |
@hymm done :) In most cases the boilerplate change is negative: explicit labels should be reserved for when you're creating an API for others to hook into a plugin (in Bevy, the ecosystem or in a large project). |
Firstly, I don't like the argument that the existence of stringly labels encourages bad code, and that users need to be forced to use typed labels. I think everyone is well aware of the downsides and upsides. Now, I am personally firmly against removing stringly labels for the time being, at least until we at least land a few other important changes. So, there are currently 4 things (afaik) where labels are used:
All your current discussion so far seems to be focused on systems. I understand the argument that we now have an even more convenient way of ordering systems, using the function. No complaints here. Run Criteria are going to be completely redesigned with Stageless (and will no longer need labels), and in their current state are largely unusable in practice, so nothing is lost here. Ambiguity Sets I cannot comment on, because I have never personally used any Bevy APIs related to ambiguities. Stages, however, are still a necessary reality of life. Until Stageless is done, they are the only way to accomplish a lot of practical use cases. Personally, this is still my major use case for stringly labels, as it can be a big productivity booster. Saves a lot of time while prototyping, and greatly improved my game jam experience. :) I propose we do not remove stringly labels until after Stageless. |
To counter arguments like "it's just a little more typing!" and "you already don't mind derive component", I responded to that in Discord, and I will quote it here:
To improve the user experience, we should focus on addressing these usability issues with typed labels, first. If typed labels (and state types for that matter) didn't require a long magic list of derives, that would be a pretty big usability improvement. |
# Objective - Closes #4954 - Reduce the complexity of the `{System, App, *}Label` APIs. ## Solution For the sake of brevity I will only refer to `SystemLabel`, but everything applies to all of the other label types as well. - Add `SystemLabelId`, a lightweight, `copy` struct. - Convert custom types into `SystemLabelId` using the trait `SystemLabel`. ## Changelog - String literals implement `SystemLabel` for now, but this should be changed with #4409 . ## Migration Guide - Any previous use of `Box<dyn SystemLabel>` should be replaced with `SystemLabelId`. - `AsSystemLabel` trait has been modified. - No more output generics. - Method `as_system_label` now returns `SystemLabelId`, removing an unnecessary level of indirection. - If you *need* a label that is determined at runtime, you can use `Box::leak`. Not recommended. ## Questions for later * Should we generate a `Debug` impl along with `#[derive(*Label)]`? * Should we rename `as_str()`? * Should we remove the extra derives (such as `Hash`) from builtin `*Label` types? * Should we automatically derive types like `Clone, Copy, PartialEq, Eq`? * More-ergonomic comparisons between `Label` and `LabelId`. * Move `Dyn{Eq, Hash,Clone}` somewhere else. * Some API to make interning dynamic labels easier. * Optimize string representation * Empty string for unit structs -- no debug info but faster comparisons * Don't show enum types -- same tradeoffs as asbove.
# Objective - Closes #4954 - Reduce the complexity of the `{System, App, *}Label` APIs. ## Solution For the sake of brevity I will only refer to `SystemLabel`, but everything applies to all of the other label types as well. - Add `SystemLabelId`, a lightweight, `copy` struct. - Convert custom types into `SystemLabelId` using the trait `SystemLabel`. ## Changelog - String literals implement `SystemLabel` for now, but this should be changed with #4409 . ## Migration Guide - Any previous use of `Box<dyn SystemLabel>` should be replaced with `SystemLabelId`. - `AsSystemLabel` trait has been modified. - No more output generics. - Method `as_system_label` now returns `SystemLabelId`, removing an unnecessary level of indirection. - If you *need* a label that is determined at runtime, you can use `Box::leak`. Not recommended. ## Questions for later * Should we generate a `Debug` impl along with `#[derive(*Label)]`? * Should we rename `as_str()`? * Should we remove the extra derives (such as `Hash`) from builtin `*Label` types? * Should we automatically derive types like `Clone, Copy, PartialEq, Eq`? * More-ergonomic comparisons between `Label` and `LabelId`. * Move `Dyn{Eq, Hash,Clone}` somewhere else. * Some API to make interning dynamic labels easier. * Optimize string representation * Empty string for unit structs -- no debug info but faster comparisons * Don't show enum types -- same tradeoffs as asbove.
# Objective - Closes bevyengine#4954 - Reduce the complexity of the `{System, App, *}Label` APIs. ## Solution For the sake of brevity I will only refer to `SystemLabel`, but everything applies to all of the other label types as well. - Add `SystemLabelId`, a lightweight, `copy` struct. - Convert custom types into `SystemLabelId` using the trait `SystemLabel`. ## Changelog - String literals implement `SystemLabel` for now, but this should be changed with bevyengine#4409 . ## Migration Guide - Any previous use of `Box<dyn SystemLabel>` should be replaced with `SystemLabelId`. - `AsSystemLabel` trait has been modified. - No more output generics. - Method `as_system_label` now returns `SystemLabelId`, removing an unnecessary level of indirection. - If you *need* a label that is determined at runtime, you can use `Box::leak`. Not recommended. ## Questions for later * Should we generate a `Debug` impl along with `#[derive(*Label)]`? * Should we rename `as_str()`? * Should we remove the extra derives (such as `Hash`) from builtin `*Label` types? * Should we automatically derive types like `Clone, Copy, PartialEq, Eq`? * More-ergonomic comparisons between `Label` and `LabelId`. * Move `Dyn{Eq, Hash,Clone}` somewhere else. * Some API to make interning dynamic labels easier. * Optimize string representation * Empty string for unit structs -- no debug info but faster comparisons * Don't show enum types -- same tradeoffs as asbove.
# Objective - Closes bevyengine#4954 - Reduce the complexity of the `{System, App, *}Label` APIs. ## Solution For the sake of brevity I will only refer to `SystemLabel`, but everything applies to all of the other label types as well. - Add `SystemLabelId`, a lightweight, `copy` struct. - Convert custom types into `SystemLabelId` using the trait `SystemLabel`. ## Changelog - String literals implement `SystemLabel` for now, but this should be changed with bevyengine#4409 . ## Migration Guide - Any previous use of `Box<dyn SystemLabel>` should be replaced with `SystemLabelId`. - `AsSystemLabel` trait has been modified. - No more output generics. - Method `as_system_label` now returns `SystemLabelId`, removing an unnecessary level of indirection. - If you *need* a label that is determined at runtime, you can use `Box::leak`. Not recommended. ## Questions for later * Should we generate a `Debug` impl along with `#[derive(*Label)]`? * Should we rename `as_str()`? * Should we remove the extra derives (such as `Hash`) from builtin `*Label` types? * Should we automatically derive types like `Clone, Copy, PartialEq, Eq`? * More-ergonomic comparisons between `Label` and `LabelId`. * Move `Dyn{Eq, Hash,Clone}` somewhere else. * Some API to make interning dynamic labels easier. * Optimize string representation * Empty string for unit structs -- no debug info but faster comparisons * Don't show enum types -- same tradeoffs as asbove.
# Objective - Closes bevyengine#4954 - Reduce the complexity of the `{System, App, *}Label` APIs. ## Solution For the sake of brevity I will only refer to `SystemLabel`, but everything applies to all of the other label types as well. - Add `SystemLabelId`, a lightweight, `copy` struct. - Convert custom types into `SystemLabelId` using the trait `SystemLabel`. ## Changelog - String literals implement `SystemLabel` for now, but this should be changed with bevyengine#4409 . ## Migration Guide - Any previous use of `Box<dyn SystemLabel>` should be replaced with `SystemLabelId`. - `AsSystemLabel` trait has been modified. - No more output generics. - Method `as_system_label` now returns `SystemLabelId`, removing an unnecessary level of indirection. - If you *need* a label that is determined at runtime, you can use `Box::leak`. Not recommended. ## Questions for later * Should we generate a `Debug` impl along with `#[derive(*Label)]`? * Should we rename `as_str()`? * Should we remove the extra derives (such as `Hash`) from builtin `*Label` types? * Should we automatically derive types like `Clone, Copy, PartialEq, Eq`? * More-ergonomic comparisons between `Label` and `LabelId`. * Move `Dyn{Eq, Hash,Clone}` somewhere else. * Some API to make interning dynamic labels easier. * Optimize string representation * Empty string for unit structs -- no debug info but faster comparisons * Don't show enum types -- same tradeoffs as asbove.
&str no longer implements label (SystemSet) after #6587 |
Objective
Solution
Changelog
AppLabel
,StageLabel
,SystemLabel
,AmbiguitySetLabel
orRunCriteriaLabel
.Migration Guide
String-like types can no longer be used as labels for
AppLabel
,StageLabel
,SystemLabel
, AmbiguitySetLabelor
RunCriteriaLabel. Use a constructed type (including a type that stores data) instead. In the most common case of a
SystemLabel, you should strongly consider using an implicit
SystemTypeIdLabel` instead.For example, suppose your initial code was:
This could either become:
Or:
Prefer implicit system labels over explicit ones (for clarity and brevity) whenever they do not need to be exposed to an external consumer. By contrast, explicit system labels can be useful to expose a thoughtful and stable external API that consumers can rely upon without excessive breakage when you modify the internals of your plugin.
Status