resolve conflicts by insertion order

[hymm]

Opening this as draft as I'd prefer to merge #9822 first and this is likely to have conflicts with that pr. Also might add write-before-read resolution in this pr rather than a follow up pr. I haven't decided which is better yet.

Objective

• Make a stable order of system execution when there are ambiguities. Ambiguities lead to hard to diagnose behavior as the symptoms only occur randomly per run of the app.

Solution

• Depend on system insertion order to resolve ambiguities. The NodeId was already being generated when inserting new systems.
• Make a new enum AmbiguityDetection that has a Resolve variant added. Otherwise has the same variants as LogLevel.
• Make Resolve the default instead of Ignore. Having the random behavior was hard for users to understand. Now if they have an incorrect ambiguity it will always be present.
• Bugs with ambiguity will now manifest in a new way. Say a user reorders their plugins. This could now introduce new behavior even the user didn't change anything else with their app. Users should be taught to turn on ambiguity detection in these situations. I think this is better than the status quo, but not ideal.

---

Changelog

• resolve ambiguities in system execution with insertion order

Migration Guide

• ScheduleBuildSetings::ambiguity_detection now takes a AmbiguityDetection that has an extra variant Resolve over LogLevel.
• The default ambiguity detection level was changed from Ignore to Resolve. If you want the previous behavior you'll need to use schedule.set_build_settings to change it back.

Future Work

I'd like to also add in resolving by placing writes before reads, but this is more work as this information is not currently available in the reported conflicts, so this will require more extensive changes. This feature should also help a bit with the confusing bugs by reordering plugins mentioned above.

There will be systems that can't be resolved by write-before-read ordering. They could be writing the same component or one will write one component while the other reads that component, but writes another. In these cases we can fall back to system insertion order, but we provide the options to error or warn in these situations.

[alice-i-cecile]

Hmm: I'd definitely like to make these deterministic. However, I still don't love falling back to insertion order. Can we use a seeded RNG, with user control over the seed? Or even better, some seed that can be used for a more stable mapping?

That gives us reproducibility without the painful implicitness of insertion order, and lets users test for insertion order bugs more easily.

[joseph-gio]

I agree with Alice's seeded RNG idea. If we use insertion order to make it deterministic, then many users will start relying on that implicit behavior as the primary way to order systems. We want to encourage users to always use explicit ordering when it matters.

[james7132]

If there's a need for a RNG source, fastrand is already in tree for bevy_ecs, and should be sufficient for this use case.

[alice-i-cecile]

Sure, I'm happy enough with this as a compromise, especially with strong warnings in the docs.

[hymm]

This PR isn't going to work correctly with the current algorithm. It can add cycles into the graph fairly easily. We'd have to check for existing transitive dependencies each time we add an edge to avoid this. Probably too expensive to be worth it.

I'm not sure this is the right solution to the problem anyways, given that this just hides bugs that are easy to expose during refactors. I think we should:

1. Cleanup all internal ambiguities.
2. Add a way of ignore ambiguities by type.
3. Reintroduce warning verbosity i.e. show only the number of conflicts as a warning, and do that by default.

I'll leave this PR open for a little bit in case someone who sees it thinks of a way of adding edges without introducing cycles, but will otherwise close this PR soonish.

[alice-i-cecile]

Hmm. If we add the edges one at a time, checking for ambiguities each time, it should be impossible to introduce cycles. Obviously that would be very slow, but there's definitely an algorithm here.

[joseph-gio]

How are random system orderings currently chosen? Could we take the current approach and make it seeded? Apologies if this has an obvious answer, I haven't done any due diligence before asking.

[alice-i-cecile]

They're currently incidental based on how the topological sort shakes out.

[maniwani]

🤔

I don't know if there's a "good" solution that can circumvent a dependence on insertion order.

We can prevent executor variation by choosing an ordering for ambiguous nodes that follows their topological order, but we can't guarantee the topological order itself will be the same every time.

The topological sort is essentially a DFS, so any randomness in its output comes from the order DFS visits nodes, which ultimately derives from (1) the order nodes and edges were inserted and (2) the first node visited by the sorting algorithm. I believe the first node visited in the petgraph impl is just the first node inserted, so it really all comes down to (1).

To control (1), we'd need to reassemble the graph with a new insertion order that comes from something stable, like UUIDs.

---

Let's say we give up on that and look for a solution that can be applied after a "random" topological sort. I don't know if one that is particularly cheap exists. The best I can come up with is this.

1. Sort the list of ambiguous pairs by topological depth.
   • Could use a seed to get a deterministic ordering of this list and the elements of each pair.
2. Resolve pairs in that order.
   • If A -> B is applied to resolve (A, B), skip any following (descendant of A, B) or (B, descendant of A) pairs.

[hymm]

closing this as any future work will require a new pr.