Exposes Observer's system's name

[hukasu]

Objective

Fixes #18726
Alternative to and closes #18797

Solution

Create a method Observer::system_name to expose the name of the Observer's system

Showcase

// Returns `my_crate::my_observer`
let observer = Observer::new(my_observer);
println!(observer.system_name());

// Returns `my_crate::method::{{closure}}`
let observer = Observer::new(|_trigger: Trigger<...>|);
println!(observer.system_name());

// Returns `custom_name`
let observer = Observer::new(IntoSystem::into_system(my_observer).with_name("custom_name"));
println!(observer.system_name());

TODO

• Achieve cart's approval

[hukasu]

aylmao, the test failed because it uses Trigger<OnAdd, Name> for the test observers, and now i'm add Name for every observer...

[alice-i-cecile]

Awesome, this is really clean. Two small things to clean up:

1. We should probably do something like "type_name Observer" for the name. Make a method on Observer or a free function to define the exact standard name, then call that in the couple of places where we're defining the name.
2. We should lump these changes in with the existing observer_overhaul release notes. Don't worry too much about making it nice; just move the content you have over and add yourself + this PR to the authors list. We'll need a big rewrite at the end of it to cover all of these changes.

[alice-i-cecile]

Left a suggestion to avoid a new public Bundle type. Once that's fixed, this LGTM :) Thanks!

[mgi388]

@alice-i-cecile do you think it would be a reasonable follow up to this PR (but not in it) to make this behind a feature (dunno which, debug or entity_names) so that the observer Name can be excluded in release builds (and I’m making the assumption that it’s sole purpose is for debugging or visualization rather than powering actual observer functionality)?

I definitely don’t want to be prematurely optimizing but also I’ve seen the massive list of observers in inspector egui at least in my game and I’d be surprised if I want all those entities in my release build hanging around with Names. Happy to take this conversation elsewhere but this PR prompted the thought.

[hukasu]

@alice-i-cecile they need to be at least pub(crate) to be visible by the World, Commands and EntityWorldMut

[alice-i-cecile]

@alice-i-cecile do you think it would be a reasonable follow up to this PR (but not in it) to make this behind a feature (dunno which, debug or entity_names) so that the observer Name can be excluded in release builds (and I’m making the assumption that it’s sole purpose is for debugging or visualization rather than powering actual observer functionality)?

I definitely don’t want to be prematurely optimizing but also I’ve seen the massive list of observers in inspector egui at least in my game and I’d be surprised if I want all those entities in my release build hanging around with Names. Happy to take this conversation elsewhere but this PR prompted the thought.

This should be done as part of #19558, or in follow-up after both these PRs are merged. Good thought!

[alice-i-cecile]

Sorry, my request wasn't clear. I just wanted to use the anonymous tuple type, not to make the constructor private :) I've left a final suggestion with my intended fix, then this LGTM.

[chescock]

This is cool!

Should there be any unit tests for this, where we spawn an observer and check that is has a Name component with a useful value? Or maybe they would be too brittle because type_name doesn't have any stability guarantees.

[hukasu]

the rebase wrecked everything... will restart

[hukasu]

i don't know if this new attempt still count as solving the issue raised by #18726

[alice-i-cecile]

i don't know if this new attempt still count as solving the issue raised by #18726

This will be good enough: inspectors can manually call these methods to generate names for now. We can add Name components later behind a feature flag.

[chescock]

Splitting up the System trait like that is a lot of churn, and it doesn't seem like other code benefits from the split. Would it make sense to localize the change by making NamedSystem use a blanket impl instead of being a base trait? You could even merge it with AnyNamedSystem at that point. That is, something like:

trait AnyNamedSystem: Any + Send + Sync + 'static {
    fn system_name(&self) -> Cow<'static, str>;
}
impl<S: System> AnyNamedSystem for S {
    fn system_name(&self) -> Cow<'static, str> { self.name() }
}
// ...
pub struct Observer {
    // ...
    system: Box<dyn AnyNamedSystem>,

[rdrpenguin04]

Should this still have "the", for grammar's sake?