-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
bevy_ecs: forward type_id
in InfallibleSystemWrapper
#18931
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
base: main
Are you sure you want to change the base?
bevy_ecs: forward type_id
in InfallibleSystemWrapper
#18931
Conversation
`bevy_mod_debugdump` uses the `SystemTypeSet::system_type` to look up constrains like `(system_1, system_2.after(system_1))`. For that it needs to find the type id in `schedule.graph().systems()` Now with systems being wrapped in an `InfallibleSystemWrapper` this association was no longer possible.
@@ -30,6 +30,10 @@ impl<S: System<In = ()>> System for InfallibleSystemWrapper<S> { | |||
self.0.name() | |||
} | |||
|
|||
fn type_id(&self) -> core::any::TypeId { |
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.
fn type_id(&self) -> core::any::TypeId { | |
/// Type id of the wrapped system. | |
fn wrapped_type_id(&self) -> core::any::TypeId { |
Let's not shadow <InfallibleSystemWrapper as Any>::type_id
with a different meaning (previously using the same name was fine, as it always returned the same result).
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 rename the method, I'd vote for system_type
because not every System
is a wrapper. It also aligns with SystemSet::system_type
Do you want to add a unit test that verifies that the type ids are the same, with a comment that Alternately, I think Bevy is internally matching things up using |
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.
It's a little weird that type id won't return the actual type id, but I think this fix makes sense. The internal uses are just to identify apply_deffered systems and getting systems for system stepping. And I can't think of any ways this would mess things up for external consumers.
Oh nice, that works as well: fn system_of_system_type(&self, set: &dyn SystemSet) -> Option<NodeId> {
self.graph.systems().find_map(|(id, system, _)| {
let is_system_set = system.default_system_sets().iter().any(|s| s.0 == set);
is_system_set.then_some(id)
})
} In that case I don't need this PR at all and can release |
similar to #12030
Objective
bevy_mod_debugdump
uses theSystemTypeSet::system_type
to look up constrains like(system_1, system_2.after(system_1))
. For that it needs to find the type id inschedule.graph().systems()
Now with systems being wrapped in an
InfallibleSystemWrapper
this association was no longer possible.Solution
By forwarding the type id in
InfallibleSystemWrapper
,bevy_mod_debugdump
can resolve the dependencies as before, and the wrapper is an unnoticable implementation detail.Testing
cargo test -p bevy_ecs
I'm not sure what exactly could break otherwise.