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 Label type #1423

Closed
wants to merge 27 commits into from
Closed

Conversation

TheRawMeatball
Copy link
Member

Maybe potentially solves #103 and some similar issues.
Using these for RenderGraph nodes and slots is planned.

@TheRawMeatball TheRawMeatball marked this pull request as draft February 9, 2021 21:00
@Ratysz
Copy link
Contributor

Ratysz commented Feb 10, 2021

This looks way more complex than it needs to be.

Why not have separate derivable traits for labels of different types, instead of requiring an extra argument for the derive macro? E.g.,

#[derive(Debug, Hash, PartialEq, Eq, Clone, IntoLabel)]
#[label_type(StageLabel)]
enum Stages {
    SceneStage,
}

could be just

#[derive(Debug, Hash, PartialEq, Eq, Clone, StageLabel)]
enum Stages {
    SceneStage,
}

and Label<StageLabel> could then be reduced to

type BoxedStageLabel = Box<dyn StageLabel>;

(I'm not sure why does it have to be an Arc in the PR). Ghastly-named trait FullIntoLabel then disappears entirely, leaving only label: impl StageLabel or label: impl SystemLabel in it's place.

StageLabel and SystemLabel (and whichever else we might need) can share most of their machinery, you'll only need to implement PartialEq, Eq, and Hash on each dyn of them. Clone can also be arranged. See here for reference.

To prevent getting stuck beyond the bikeshedding event horizon of infinite potential, I would leave replacing all labels out of the PR, and instead implemented StageLabel (and maybe SystemLabel) on Into<Cow<'static, str>>. This should allow leaving them as is for now, and change them out slowly as we need - chances are we'll ditch stages entirely before such need arises anyway.

Unrelated, system labels don't seem to be plumbed in everywhere.

@alice-i-cecile
Copy link
Member

Being able to refer to other systems by their type in some form rather than relying on always having to explicitly label your systems would be a really impressive win from an ergonomics perspective.

This would make the beginner experience a lot nicer and avoid really tedious boilerplate.

You'll usually only have one system for each function, and dependencies will usually be defined between systems in the same module.

I'm not super qualified on how you'd do so (default the label field using reflection?) but feel free to bug #ecs if you need mad science help and I can dig in.

Otherwise, Ratys's comments look solid.

@Ratysz
Copy link
Contributor

Ratysz commented Feb 15, 2021

We don't have a better way of doing this. Type name, or anything derived from just the signature, is about the only unambiguous thing we can get from a function without IntoSystem-ing it.

In any case, this is insufficient, because we may need multiple instances of the same type: not everything that's a system is a distinct function. It also muddles encapsulation of systems' labels and encapsulation of systems themselves: a plugin might not want to expose a system, to prevent multiple instances of it (potentially a safety concern), but would like to allow its users to order their systems relative to it.

This would make the beginner experience a lot nicer and avoid really tedious boilerplate.

I'll raise my point from Discord again: putting the schedule together is not something a user would be doing constantly. Barring pathological cases, any boilerplate we may introduce will stand out only in sterile demo code.

Copy link
Contributor

@Ratysz Ratysz left a comment

Choose a reason for hiding this comment

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

I still think that changing all the labels everywhere should be a separate PR: it's a lot of small, bikesheddable changes everywhere, it distracts from the implementation itself.

Tests should definitely not be touched: we continue to enable the simple stringly-typed labels for a reason.

crates/bevy_app/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_app/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_core/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/label.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/stage.rs Show resolved Hide resolved
crates/bevy_ecs/src/schedule/system_descriptor.rs Outdated Show resolved Hide resolved
@TheRawMeatball
Copy link
Member Author

I still think that changing all the labels everywhere should be a separate PR: it's a lot of small, bikesheddable changes everywhere, it distracts from the implementation itself.

They're changes that need to be done eventually, why wait? They're already done

Tests should definitely not be touched: we continue to enable the simple stringly-typed labels for a reason.

That's a fair point though, I'll revert the changes in tests

@Ratysz
Copy link
Contributor

Ratysz commented Feb 16, 2021

They're changes that need to be done eventually, why wait? They're already done

Yes, I forgot to add "but they're already done, so never mind" - I don't think you should revert them now, but I haven't changed my opinion on them in general.

@TheRawMeatball TheRawMeatball marked this pull request as ready for review February 16, 2021 11:34
crates/bevy_ecs/src/schedule/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/stage.rs Outdated Show resolved Hide resolved
examples/ecs/ecs_guide.rs Outdated Show resolved Hide resolved
crates/bevy_transform/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_transform/src/lib.rs Outdated Show resolved Hide resolved
@Ratysz
Copy link
Contributor

Ratysz commented Feb 17, 2021

I've started experimenting on this branch, and some things keep nagging at me.

The derive macro is #[derive(ThingLabel)], but ThingLabel is a type, not a trait - this is not how derive is normally used. It's bound to confuse someone.

All methods using labels take impl Into<ThingLabel>. This can be minsconstrued as requiring to implement Into<ThingLabel> in order to use something as a label for a thing - which is what actually happens in the end, albeit indirectly. Deriving a ThingLabel is, again, not the first thing that would come to mind.

In my original prototype ThingLabel is a trait, which makes the derive macro a natural fit and allows the arguments to be impl ThingLabel (and &impl ThingLabel, which could probably be &dyn ThingLabel) instead. I'll see what it takes to adapt this branch to that approach, just so we have something to compare against.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

This looks good to me, but I agree with @Ratysz's assessment of the trait/derive.

We should also discuss removing the Eq constraint from labels (and changing DynEq to DynPartialEq). Its not strictly needed and would cut down on label declaration boilerplate. However (1) that might not work with @Ratysz 's proposed changes and (2) Eq is still correct "semantically", which imo is the main argument to include it.

I'm also about to push some (hopefully) non-contentious code style changes.

crates/bevy_app/src/app_builder.rs Outdated Show resolved Hide resolved
/// The names of the default App startup stages
pub mod startup_stage;
#[derive(Debug, Hash, PartialEq, Eq, Clone, StageLabel)]
pub enum StartupStage {
Copy link
Member

Choose a reason for hiding this comment

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

I like to use the following order for imports:

  • mod declarations
  • public imports
  • private imports
  • code

crates/bevy_ui/src/lib.rs Show resolved Hide resolved
examples/ecs/ecs_guide.rs Outdated Show resolved Hide resolved
@TheRawMeatball
Copy link
Member Author

These labels are used in hashmaps which need Eq, so I don't think removing that is an option.

For the derives, I initally had an IntoLabel trait, and had its derive ask for a custom #[label_type] attribute, but @Ratysz changed my mind, so I'm not going to act on that yet.

@Ratysz
Copy link
Contributor

Ratysz commented Feb 18, 2021

I'll see what it takes to adapt this branch to that approach, just so we have something to compare against.

Here are the results: diff with Bevy master, diff with this branch. I'm biased, but I think this is cleaner.

Things that can be tweaked:

  1. Label traits' guts are implemented via a plain macro. This is not as useful right now, but in the future we will likely have more kinds of different labels. The traits' definition itself can also be moved into the macro, but I opted to leave it out for ease of refactoring.
  2. DynClone functionality is incorporated into the label traits themselves; this creates minor code duplication - although, seemingly less than when trying to lift it into a separate trait. I've opted to #[doc(hidden)] the dyn_clone().
  3. type BoxedThingLabel = Box<dyn ThingLabel> is used internally everywhere (I think). It is not exported, because it bloats the API surface and is trivial.
  4. Some impl ThingLabel arguments could be replaced with &dyn ThingLabel, like here (this one is done in order to get rid of an extra generic whenever these methods are used). I've opted to not do that yet, because it adds &s to user code; this would be in line with how std keys are used, however: take when inserting, borrow when querying.

fn derive_label(input: DeriveInput, label_type: Ident) -> TokenStream2 {
let ident = input.ident;

let manifest = Manifest::new().unwrap();
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't work for examples and doc tests if bevy_ecs is used in the main dependencies and bevy in the dev-dependencies.
Searching in the main deps first and otherwise in the dev-dependencies like here should work better.

Copy link
Contributor

@Ratysz Ratysz Feb 18, 2021

Choose a reason for hiding this comment

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

This test passes, for what it's worth.

Copy link
Contributor

Choose a reason for hiding this comment

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

If bevy was added to the dev-dependencies it would fail. Or rather, the test would work but the other derives would fail.

Copy link
Member

Choose a reason for hiding this comment

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

yeah its probably worth adopting that method here.

@cart
Copy link
Member

cart commented Feb 18, 2021

@TheRawMeatball

These labels are used in hashmaps which need Eq, so I don't think removing that is an option.

But the user-defined label type isn't actually the type used in the hashmap (in the original impl we were discussing). We could remove the Eq impl in the user-defined type and still implement Eq on the wrapper type that is actually stored. I know this works because I implemented it before leaving the comment 😄

@Ratysz
I'm also partial to this impl. Definitely simpler in general. I also do like get_stage(label: &dyn StageLabel), not because of parity with std (afaik refs are only used in those interfaces because they work best generically. imo copy types shouldn't use refs in general). I like it because it makes calling the relevant methods less weird when it comes to generic type parameters.

@Ratysz
Copy link
Contributor

Ratysz commented Feb 18, 2021

So... What should we do then? &dyn ThingLabel where appropriate then pull my branch into this one?

@cart
Copy link
Member

cart commented Feb 18, 2021

Hmm I'm guessing you don't have the ability to push commits here. I think the only important outcome is both authors getting credited. So if you create a new pr with @TheRawMeatball's commits in the history, then that works. Or alternatively @TheRawMeatball can apply your commits to this pr.

@cart
Copy link
Member

cart commented Feb 18, 2021

And to be clear, i think we should only use &dyn ThingLabel when it makes the "calling ergonomics" situation more natural. We should accept impl ThingLabel wherever possible imo.

@YohDeadfall
Copy link
Member

Or alternatively @TheRawMeatball can apply your commits to this pr.

It's also possible to open a PR to his repo and merge changes with rebase, so no merge commit will be in the history of this PR.

@Ratysz
Copy link
Contributor

Ratysz commented Feb 18, 2021

Something went very, very wrong during conflict resolution. The PR lives on as #1473.

cart pushed a commit that referenced this pull request Feb 18, 2021
@Ratysz
Copy link
Contributor

Ratysz commented Feb 18, 2021

1473 is now merged, closing.

@Ratysz Ratysz closed this Feb 18, 2021
@TheRawMeatball TheRawMeatball deleted the zst-labels branch February 18, 2021 21:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps A-ECS Entities, components, systems, and events P-High This is particularly urgent, and deserves immediate attention 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.

7 participants