Simple improvements to ambiguity reporting clarity

[alice-i-cecile]

What problem does this solve or what need does it fill?

Ambiguity reports are hard to decipher.

What solution would you like?

1. Number the reported ambiguities, to make them easier to discuss in a team.
2. Display function signatures of conflicting systems.
3. Explain which component(s) / resource(s) the systems are conflicting on.
4. Report ambiguities where all pairwise combinations of systems conflict on the same data as a group.
5. Create a simple .ambiguous() method that causes a system to be ignored completely in the ambiguity checker. This would only be useful for inconsequential systems like particle effect creation.
6. Create a simple .ambiguous_with(system_label) method to create two-element ambiguity sets without being forced to invent an ambiguity set label.
7. Provide an "X unresolved ambiguities detected" message that runs by default. Rather than toggling ambiguity detection on/off, have three states: off / minimal / verbose. This would provide discoverability that system ambiguities are a thing and let developers see when the change that they just made added / removed some ambiguities at a glance.

What alternative(s) have you considered?

Allow for a verbose mode?

Additional context

Many more complex improvements could be added, but these are deliberately simple to allow them to be patched and merged quickly. This arose from helping @jamadazi with resolving ambiguities in his non-trivial codebase.

Suggestion 6 results in asymmetric declarations of a symmetric property, but having to invent trivial names all over the place is awful, and large ambiguity sets are often overkill / have unintended behavior for simple conflict resolutions.

[alice-i-cecile]

If this isn't sufficient, we should seriously consider implementing a simple form of #1481 to supplement it.

The other natural extension is system graph visualization tools.

[alec-deason]

It would be nice if the ambiguity checker could output to a file rather than stdout. That way it's output isn't mixed with other logging and it's in a place that's easy to work with. Either a environment variable or a command line argument specifying the output path would be awesome.

This seems closely related to this issue that I'm commenting here rather than making a separate issue.

[Ratysz]

It should be possible to selectively write logs to different files. I don't know if Bevy's built-in logging frontend can do that, though.

[jihiggins]

Should probably not spit out ambiguities that are between internal bevy systems by default

[alice-i-cecile]

If anyone is interested in tackling this, the work itself should be quite simple once you know where to look :) Feel free to ask me for directions on here or Discord.

[Seldom-SE]

It could also be useful to be able to specify in some way that the ambiguity checker should not check inaccessible systems from other crates, perhaps by specifying plugins to ignore, or by specifying which plugins to check (ideas for this would be appreciated; I'm still sort of new to Bevy). This will help remove unactionable warnings.

I think I would be interested in taking this on, if no one else has taken it by the time I'm available, in a few days.

[afonsolage]

Given the following simple test:

use bevy::{ecs::schedule::ReportExecutionOrderAmbiguities, log::LogPlugin, prelude::*};

fn main() {
    env_logger::init();
    App::new()
        .add_plugin(LogPlugin::default())
        .insert_resource(ReportExecutionOrderAmbiguities)
        .insert_resource(MyStartupResource(0))
        .add_startup_system(startup_system_a)
        .add_startup_system(startup_system_b)
        .insert_resource(MyParallelResource(0))
        .add_system(parallel_system_a)
        .add_system(parallel_system_b)
        .run();
}

struct MyStartupResource(i32);
fn startup_system_a(mut res: ResMut<MyStartupResource>) {
    res.0 += 1;
}
fn startup_system_b(mut res: ResMut<MyStartupResource>) {
    res.0 += 1;
}

struct MyParallelResource(i32);
fn parallel_system_a(mut res: ResMut<MyParallelResource>) {
    res.0 += 1;
}
fn parallel_system_b(mut res: ResMut<MyParallelResource>) {
    res.0 += 1;
}

Currently prints the following output:

Sep 02 07:27:32.552  INFO bevy_ecs::schedule::stage: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems:
 * Parallel systems:
 -- "ambiguity_test::startup_system_a" and "ambiguity_test::startup_system_b"
    conflicts: ["ambiguity_test::MyStartupResource"]

Sep 02 07:27:32.553  INFO bevy_ecs::schedule::stage: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems:
 * Parallel systems:
 -- "ambiguity_test::parallel_system_a" and "ambiguity_test::parallel_system_b"
    conflicts: ["ambiguity_test::MyParallelResource"]

So if I understood correct, those are the following desired changes:

1. Number the reported ambiguities, to make them easier to discuss in a team.
The ambiguity count should be per stage and not global

Sep 02 07:27:32.552  INFO bevy_ecs::schedule::stage: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems:
1. Parallel systems:
1.1 "ambiguity_test::startup_system_a" and "ambiguity_test::startup_system_b"
    conflicts: ["ambiguity_test::MyStartupResource"]

Sep 02 07:27:32.553  INFO bevy_ecs::schedule::stage: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems:
1. Parallel systems:
1.1 "ambiguity_test::parallel_system_a" and "ambiguity_test::parallel_system_b"
    conflicts: ["ambiguity_test::MyParallelResource"]

2. Display function signatures of conflicting systems.
Just add the function signature on the pair of conflicting systems

Sep 02 07:27:32.552  INFO bevy_ecs::schedule::stage: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems:
1. Parallel systems:
1.1 "ambiguity_test::startup_system_a(ambiguity_test::MyStartupResource)" and "ambiguity_test::startup_system_b(ambiguity_test::MyStartupResource)"
    conflicts: ["ambiguity_test::MyStartupResource"]

[...]

3. Explain which component(s) / resource(s) the systems are conflicting on.
Just add a better explanation, since the info already exists

Sep 02 07:27:32.552  INFO bevy_ecs::schedule::stage: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems:
1. Parallel systems:
1.1 "ambiguity_test::startup_system_a(ambiguity_test::MyStartupResource)" and "ambiguity_test::startup_system_b(ambiguity_test::MyStartupResource)" conflicts on the following components/resources: 
        ["ambiguity_test::MyStartupResource"]

[...]

4. Report ambiguities where all pairwise combinations of systems conflict on the same data as a group.
Given a third parallel_system_c(ambiguity_test::MyParallelResource) added on previous example:_

[...]

Sep 02 07:27:32.553  INFO bevy_ecs::schedule::stage: Execution order ambiguities detected, you might want to add an explicit dependency relation between some of these systems:
1. Parallel systems:
1.1 "[ambiguity_test::parallel_system_a(ambiguity_test::MyParallelResource), ambiguity_test::parallel_system_b(ambiguity_test::MyParallelResource),ambiguity_test::parallel_system_c(ambiguity_test::MyParallelResource)]" conflicts on the following components/resources: 
        ["ambiguity_test::MyParallelResource"]

5. Create a simple .ambiguous() method that causes a system to be ignored completely in the ambiguity checker. This would only be useful for inconsequential systems like particle effect creation.
Any system that calls this method on app build should be ignored from any stage ambiguity checker

[...]
        .insert_resource(MyParallelResource(0))
        .add_system(parallel_system_a.ambiguous()) //ignored by ambiguity checker
        .add_system(parallel_system_b) //since ambiguity checker will only check this one, not output is shown
[...]

6. Create a simple .ambiguous_with(system_label) method to create two-element ambiguity sets without being forced to invent an ambiguity set label.
Any system that calls this method will not be reported as ambiguous for the given label

[...]
        .insert_resource(MyParallelResource(0))
        .add_system(parallel_system_a.label("my_label"))
        .add_system(parallel_system_b.ambiguous_with("my_label") // this system will not be reported since it's marked
         // given this third system has the same requirements as the other two, it'll be reported on ambiguity checker
        .add_system(parallel_system_c)
[...]

7. Provide an "X unresolved ambiguities detected" message that runs by default. Rather than toggling ambiguity detection on/off, have three states: off / minimal / verbose. This would provide discoverability that system ambiguities are a thing and let developers see when the change that they just made added / removed some ambiguities at a glance.
By default ReportExecutionOrderAmbiguities will have a report_level field with the value of AmbiguityReportLevel::Minimal

2 unresolved ambiguities detected

8. Provide an way to ignore/filter crates when using ambiguity checker.
This will exclude from ambiguity checker any system that is inside the specified crates

        .insert_resource(ReportExecutionOrderAmbiguities {
                report_level: AmbiguityReportLevel::Minimal,
                ignored_crates: vec!["bevy", "bevy_rapier"],
        })

9. By default, ambiguity checker should ignore internal bevy crates.
Just add a default impl with the bevy crate as ignored

impl Default for ReportExecutionOrderAmbiguities {
    fn default() -> Self {
        Self {
            report_level: AmbiguityReportLevel::Minimal,
            ignored_crates: vec!["bevy"],
        }
    }
}

Is that right? This doesn't seems complicated, but I want to know if I understood correct and maybe check if some of those requirements changed over the time.

[alice-i-cecile]

@afonsolage that looks fantastic! I agree with the concrete suggestions for all 9 of those bullet items. Are you planning to tackle this?

[afonsolage]

@afonsolage that looks fantastic! I agree with the concrete suggestions for all 9 of those bullet items. Are you planning to tackle this?

Yes, I'm not very used to public repo team working, but I read the CONTRIBUTING.md so I'll start a Draft PR and give it a try.

Maybe I'll need some helping in finding the function signature, but I'll take a closer look again on bevy ecs.

[alice-i-cecile]

This is effectively complete now, closing it out 🎉