Add default system sets based on World accesses

[joseph-gio]

Objective

Resolve #7857.

Give users more tools to order systems based on logical constraints. Allow automatically grouping systems with specific world accesses or system params into sets, which can be used to specify dependencies.

Solution

Any component, resource, or event can optionally be associated with a SystemSet, which will be assigned to any system that accesses that world data.

Example: TODO

---

Changelog

todo

Migration Guide

todo

[alice-i-cecile]

Super interesting. Why limit this to events only? Why not define this for all data accesses, which would then encapsulate events?

For example, I'd like to to ensure that my character controller system always runs after anything that modifies Input<KeyCode>. If this causes a paradox with the other ordering I've applied, I want to know ASAP.

[joseph-gio]

Why not define this for all data accesses, which would then encapsulate events?

This pattern could also be useful for GlobalTransform propagation, I think. We could mark the system with .after(MutSystem::<Transform>::default())

I definitely agree with that direction, but I wanted feedback from a few members before doing the implementation work.

Also, doing that would exacerbate the following concern from #4872:

I think I like the ergonomics, but I'm a bit worried about significant schedule construction costs.

[joseph-gio]

Also, I think that if we make auto-labels ubiquitous, we'll have to consider letting users explicitly opt out of specific auto-labels for their systems.

[alice-i-cecile]

Also, I think that if we make auto-labels ubiquitous, we'll have to consider letting users explicitly opt out of specific auto-labels for their systems.

Agreed, I think this is essential even here. Probably a anti-label method?

[alice-i-cecile]

Bikeshed: I think I prefer WritesTo / ReadFrom as names. These should be clear from context that they're system labels.

[joseph-gio]

Bikeshed: I think I prefer WritesTo / ReadFrom as names. These should be clear from context that they're system labels.

Even better IMO: make the type name the verb, and the method name the preposition. Writes::to::<MyEvent>(). We'd probably need a different preposition other than from, though. (edit: should be fine)

[alice-i-cecile]

I wonder if we want to invert the logic. Rather than proactively generating labels for each resource used, instead special-case this and generate the dependencies directly to the responsible systems.

The former is O(systems * resource_types). The latter is O(systems * resource_types_used), avoiding the scheduler overhead where this feature isn't used. And where it is used, the alternative is manually creating and using these labels, which won't be any faster.

With that in place, I'd be more comfortable expanding this out to a bundle and/or component specific version. Likely in another PR for complexity management though.

[joseph-gio]

I wonder if we want to invert the logic. Rather than proactively generating labels for each resource used, instead special-case this and generate the dependencies directly to the responsible systems.

The former is O(systems * resource_types). The latter is O(systems * resource_types_used), avoiding the scheduler overhead where this feature isn't used. And where it is used, the alternative is manually creating and using these labels, which won't be any faster.

Can we avoid all of this complexity by just storing the labels in a HashSet instead of a Vec? Then, checking if a system has a specific label becomes an O(~1) operation, and we can add as many unused labels as we want without impacting performance much. If we want to save memory we could garbage collect unused labels.

[alice-i-cecile]

My first impression is that I'd be fine with that. The weak benchmarks here definitely make me nervous though.

We could also store two HashSets, one for "labels" and the other for "labels this system definitely doesn't have", and then check reactively.

If we want to save memory we could garbage collect unused labels.

IMO eventually we should have a dynamic schedule modification feature flag that's off by default, and just remove all labels after app.run() entirely.

[joseph-gio]

It appears that labels already use a HashMap for looking up dependencies, so having extra labels shouldn't impact performance too severely.

Using the fancy new benchmarks at #5410, I added 50 distinct, unused labels to every single system.

# of plugins	time	% change
10	1.3344 ms	+10.700%
50	41.205 ms	+2.5983%
100	186.23 ms	+0.4773%

10% regression isn't bad, considering that's 10*20*50=10,000 unused labels. We can also see that the performance impact becomes negligible as you add more and more systems. I would say that adding labels doesn't hurt performance, adding dependencies does.

[joseph-gio]

Thoughts on expanding this to components

• Should be off by default and discouraged
  • Adding dozens of unused labels to every system is gross, even if the benchmarks are fine.
  • Deferred mutations via commands are ubiquitous and invisible to this API, so extra care needs to be taken to ensure everything is correct.
• We should consider Transform/GlobalTransform to be the "canonical" use-case, so we need to make sure this API meets those needs.
  • transform_propagate_system must come after any system marked with Writes<Transform>.
  • The command-flush before transform_propagate_system must come after any system containing Commands.
    • As always, the user may explicitly remove the auto-label for Commands.
  • This is similar to current proposals of automatically placing systems before transform_propogate_system by default. Except here, the dependency is only applied when there's actually a chance that a system might mutate Transform.
  • transform_propogate_system is set to run before any system with Reads<GlobalTransform>.

[alice-i-cecile]

Yeah, I'm wondering if we can configure the opt-in and opt-out behavior via metadata on the Component / Resource / Event traits.

And yes, another point in favor of trying to make commands more transparent.

The other thought is that this PR will become dramatically more useful once cross-stage ordering dependencies work. We may want to fix that before merging this.

[alice-i-cecile]

This would close #7857, correct?

[joseph-gio]

If we decide to go with the macro-based approach, then yes :)

[joseph-gio]

Closing this out, since I don't think the derive macro-based approach is the right way to handle this.