Simple improvements to ambiguity reporting clarity #2766

afonsolage · 2021-09-02T21:24:39Z

Objective

Simple improvements to ambiguity reporting clarity, as discussed on #1484.

Solution

…team

alice-i-cecile · 2021-09-02T22:25:42Z

Thanks a ton for doing this! This should help a ton.

@davidscherer @concavesphere I'd love reviews from you two on this once it's ready; I think this will be a very nice incremental improvement to a friendlier scheduler.

…t on the same data as a group.

afonsolage · 2021-09-03T00:33:03Z

I wasn't able to find a reliable way to get the system signature as stated in task "Display function signatures of conflicting systems.".

Rust doesn't provide a way to get function signature and I wasn't able to do so using bevy internals. The most acceptable way was using component_access and using this to reconstruct the function signature, but it's not accurate, since the following function signature:

pub fn camera_system<T: CameraProjection + Component>(
    mut window_resized_events: EventReader<WindowResized>,
    mut window_created_events: EventReader<WindowCreated>,
    windows: Res<Windows>,
    mut queries: QuerySet<(
        QueryState<(Entity, &mut Camera, &mut T)>,
        QueryState<Entity, Added<Camera>>,
    )>,
) {

is reported as:

"bevy_render::camera::camera::camera_system<bevy_render::camera::projection::PerspectiveProjection>"("bevy_ecs::event::Events<bevy_window::event::WindowResized>", "bevy_ecs::event::Events<bevy_window::event::WindowCreated>", "bevy_window::windows::Windows", "bevy_render::camera::camera::Camera", "bevy_render::camera::projection::PerspectiveProjection")

If anyone knows a reliable way to reconstruct or get the system function signature, I'll be happy to address the task 2.

…ompletely in the ambiguity checker.

…ent ambiguity sets without being forced to invent an ambiguity set label.

…efault

afonsolage · 2021-09-04T11:26:03Z

The current implementation of task 7 (Provide an "X unresolved ambiguities detected" message that runs by default) currently produces the following output:

Sep 04 08:25:06.304  WARN bevy_ecs::schedule::stage: 1 unresolved ambiguities detected
Sep 04 08:25:06.306  WARN bevy_ecs::schedule::stage: 1 unresolved ambiguities detected
Sep 04 08:25:06.307  WARN bevy_ecs::schedule::stage: 12 unresolved ambiguities detected
Sep 04 08:25:06.341  WARN bevy_ecs::schedule::stage: 24 unresolved ambiguities detected
Sep 04 08:25:06.345  WARN bevy_ecs::schedule::stage: 3 unresolved ambiguities detected

This isn't very descriptive for new users, but I wasn't able to find a way to get the current stage label, so we would need to add this information on SystemStage in order to have a better output message, but I'm afraid this change would be out of the scope of this PR.

The following code currently produces the mentioned warnings:

use bevy::prelude::*;

fn main() {
    env_logger::init();
    App::new()
        .add_plugins(DefaultPlugins)
        .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;
}

afonsolage · 2021-09-04T13:28:51Z

The main work is done, waiting for reviews. I'll just add those changes on ReportExecutionOrderAmbiguities docs and add a new example showing how to use those new features.

alice-i-cecile · 2021-09-04T15:01:52Z

crates/bevy_ecs/src/schedule/stage.rs

@@ -45,7 +52,39 @@ impl_downcast!(Stage);
 ///
 /// The checker may report a system more times than the amount of constraints it would actually need
 /// to have unambiguous order with regards to a group of already-constrained systems.
-pub struct ReportExecutionOrderAmbiguities;
+pub struct ReportExecutionOrderAmbiguities {
+    level: AmbiguityReportLevel,


I would be fine just exposing this field directly. That would save on boilerplate and the enum will give us a type-safe API.

Is there some reason why this isn't safe?

On the latest commit, I made it public, so users which wanna more control over ReportExecutionOrderAmbiguities may create the resource directly. I've also kept the .off(), .minimal() and .verbose() to keep things easier to user (at least I think so)

pub struct ReportExecutionOrderAmbiguities { pub level: AmbiguityReportLevel, pub ignore_crates: Vec<String>, }

I've also kept the .off(), .minimal() and .verbose() to keep things easier to user (at least I think so)

I'd like to avoid API duplication where possible. I'd definitely accept it if Cart feels otherwise in a final review though, so don't worry about it for now.

crates/bevy_ecs/src/schedule/stage.rs

afonsolage · 2021-09-04T17:05:16Z

I think everything is done, I'll set it as ready for review. I've added an example also. For reference, here is the output of the example:

Sep 04 14:04:15.223  WARN bevy_ecs::schedule::stage: 1 unresolved ambiguities detected. Set the level of the ReportExecutionOrderAmbiguities resource to AmbiguityReportLevel::Verbose for more details.
Sep 04 14:04:15.223  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.0 - Systems:
     ["ambiguity_checker::startup_system_a", "ambiguity_checker::startup_system_b"]
   - Conflicts on the following components/resources:
     ["ambiguity_checker::MyStartupResource"]

Sep 04 14:04:15.224  WARN bevy_ecs::schedule::stage: 4 unresolved ambiguities detected. Set the level of the ReportExecutionOrderAmbiguities resource to AmbiguityReportLevel::Verbose for more details.
Sep 04 14:04:15.224  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.0 - Systems:
     ["ambiguity_checker::system_b", "ambiguity_checker::system_d"]
   - Conflicts on the following components/resources:
     ["i32", "ambiguity_checker::MyResource"]
1.1 - Systems:
     ["ambiguity_checker::system_b", "ambiguity_checker::system_e", "ambiguity_checker::system_d", "ambiguity_checker::system_c"]
   - Conflicts on the following components/resources:
     ["ambiguity_checker::MyResource"]

crates/bevy_ecs/src/schedule/stage.rs

cart · 2021-09-10T19:44:13Z

crates/bevy_ecs/src/schedule/stage.rs

+    }
+
+    /// Adds the given crates to be ignored by ambiguity checker. Check [`ReportExecutionOrderAmbiguities`] for more details.
+    pub fn ignore(mut self, create_prefix: &[&str]) -> Self {


I think this should take a single string instead of a list. We could also have an "ignore_all" function that takes an iterator.

Done. Renamed this one to ignore_all and added a new one ignore which just takes a single &str

cart · 2021-09-10T19:50:52Z

crates/bevy_ecs/src/schedule/stage.rs

+    pub ignore_crates: Vec<String>,
+}
+
+const BEVY_CRATES: [&str; 109] = [


Having thought about this a bit more, I think this is moving us in the wrong direction for a number of reasons:

Moves "bevy engine" crate info "upstream" into bevy_ecs, which should be standalone

Compiles a big list of strings into bevy_ecs binaries

Centralizes what should be decentralized

I personally think we should remove this list in favor of each individual crate calling ambiguities.ignore("CRATE_NAME_HERE") if we agree that ambiguities should be ignored for a given crate.

Clever. I think this is the best path forward here.

Fair points, but since the ignored crates list are stored in a resource, all crates should have a startup system which will add their crate names to it?

Also, we should add this to all bevy internal crates or only the ones which already have ambiguities reported?

And lastly, is on the scope of this PR add this info on all needed crates (I can do it, just wanna organize)?

Fair points, but since the ignored crates list are stored in a resource, all crates should have a startup system which will add their crate names to it?

I think it should be a part of Plugin init (prior to the schedule running).

Also, we should add this to all bevy internal crates or only the ones which already have ambiguities reported?

Yeah I think this is the right call. It encourages us to be more precise about suppressing these. It also cuts down on the amount of work we're doing at startup.

And lastly, is on the scope of this PR add this info on all needed crates (I can do it, just wanna organize)?

Whatever works best for you!

Done. I've added the ignore call on following plugins:

bevy_audio

bevy_render

bevy_sprite

bevy_text

bevy_ui

bevy_transform

bevy_winit

The only "trick" part is if users add the ReportExecutionOrderAmbiguities after adding DefaultPlugins or so, the default ignored crates will be overwritten. I've added this on docs and on example, informing users that if they need to customize the resource, it must be done before the plugins

…s, bevy_text, bevy_ui, bevy_transform, bevy_winit

… doesn't exists anymore

afonsolage · 2021-09-12T23:06:24Z

Maybe I'll need that help again with the spelling 😃

afonsolage · 2021-09-17T16:54:37Z

What to do when the CI fails with a code that I haven't touched?

error: field is never read: `message`
  --> crates/bevy_ecs/examples/events.rs:46:5
   |
46 |     pub message: String,
   |     ^^^^^^^^^^^^^^^^^^^
   |
   = note: `-D dead-code` implied by `-D warnings`

error: field is never read: `random_value`
  --> crates/bevy_ecs/examples/events.rs:47:5
   |
47 |     pub random_value: f32,
   |     ^^^^^^^^^^^^^^^^^^^^^

error: could not compile `bevy_ecs` due to 2 previous errors
warning: build failed, waiting for other jobs to finish...
error: build failed
Error: Process completed with exit code 101.

MinerSebas · 2021-09-17T17:11:14Z

That was already fixed by #2811 and can thus be ignored.

alice-i-cecile · 2022-01-30T22:00:18Z

crates/bevy_ecs/src/schedule/system_descriptor.rs

@@ -90,6 +90,12 @@ impl IntoSystemDescriptor<()> for ExclusiveSystemCoerced {
    }
 }

+pub enum AmbiguityDetection {


This should have a default value.

alice-i-cecile · 2022-03-03T18:16:40Z

bors try

bors · 2022-03-03T18:16:42Z

try

Merge conflict.

alice-i-cecile · 2022-03-03T18:17:15Z

@afonsolage; I'd like to try and get this merged early next week. Are you able to fix up the few remaining issues and makes sure CI passes?

maniwani

Report ambiguities where all pairwise combinations of systems conflict on the same data as a group.

This change (as implemented) makes things less clear (increasingly so with larger stages).

maniwani · 2022-03-16T02:18:52Z

crates/bevy_ecs/src/schedule/stage.rs

+            for (index_a, index_b, components) in ambiguities.drain(..) {
+                let systems = ambiguities_map.entry(components).or_default();
+                if !systems.contains(&index_a) {
+                    systems.push(index_a);
+                }
+                if !systems.contains(&index_b) {
+                    systems.push(index_b);
+                }
+            }
+
+            for (idx, (conflicts, systems_indices)) in ambiguities_map.drain().enumerate() {
+                let system_names = systems_indices
+                    .iter()
+                    .map(|index| systems[*index].name())
+                    .collect::<Vec<_>>();
+
+                let system_components = conflicts
+                    .iter()
+                    .map(|id| world.components().get_info(*id).unwrap().name())
+                    .collect::<Vec<_>>();


While it makes for fewer warning messages, "all systems that have a conflict involving T" is not the same as "all systems that conflict on T".

Reporting this way will make it harder to parse which systems actually conflict (which was obvious before), since not all systems being grouped together are actually running at the same time.

I think you maybe intended for "all systems that conflict on T at the same time" but I don't think that's easy to solve.

(edit: I also prefer how we currently report pairs with all their conflicts.)

alice-i-cecile · 2022-03-23T05:23:56Z

Closing in favor of the rebased #4299, which builds on this work. @afonsolage: thank you so, so much for this contribution, I'm very excited to see it in the engine.

Number the reported ambiguities, to make them easier to discuss in a …

a6b0da2

…team

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Sep 2, 2021

Fixed formatting

735abc5

alice-i-cecile requested review from Ratysz and alice-i-cecile September 2, 2021 22:23

alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Sep 2, 2021

afonsolage added 3 commits September 2, 2021 19:35

Explain which components/resources the systems are conflicting on.

889dd59

Fixed wrong output_prefix

b1d49ca

Report ambiguities where all pairwise combinations of systems conflic…

1083583

…t on the same data as a group.

afonsolage added 5 commits September 3, 2021 11:15

Create a simple ambiguous method that causes a system to be ignored c…

cd439af

…ompletely in the ambiguity checker.

Create a simple ambiguous_with system_label method to create two-elem…

362b804

…ent ambiguity sets without being forced to invent an ambiguity set label.

Provide an 'X unresolved ambiguities detected' message that runs by d…

1e70305

…efault

Fixed format issue

fa8f71e

Fixed ecs_guide example

77cf9e9

afonsolage added 2 commits September 4, 2021 10:20

Provide an way to ignore/filter crates when using ambiguity checker.

9c8719b

By default, ambiguity checker should ignore internal bevy crates.

7c24689

alice-i-cecile reviewed Sep 4, 2021

View reviewed changes

crates/bevy_ecs/src/schedule/stage.rs Outdated Show resolved Hide resolved

afonsolage added 4 commits September 4, 2021 13:17

Added better message when warning about unresolved ambiguities

b013b9e

Updated ReportExecutionOrderAmbiguities docs

c20907e

Added ambiguity_checker example

b08aee6

Fixed typo on ReportExecutionOrderAmbiguities docs

e9c787d

afonsolage marked this pull request as ready for review September 4, 2021 17:05

Added instructions to add new crates to BEVY_CRATES array

889a263

afonsolage mentioned this pull request Sep 10, 2021

Removed bevy_egui from reserved crates bevyengine/bevy-crate-reservations#14

Merged

cart reviewed Sep 10, 2021

View reviewed changes

crates/bevy_ecs/src/schedule/stage.rs Outdated Show resolved Hide resolved

cart reviewed Sep 10, 2021

View reviewed changes

crates/bevy_ecs/src/schedule/stage.rs Outdated Show resolved Hide resolved

cart reviewed Sep 10, 2021

View reviewed changes

afonsolage added 6 commits September 12, 2021 19:24

Fixed typo on ReportExecutionOrderAmbiguities ignore function

ee43fd4

Removed BEVY_CRATES const array

0dbb9c4

Added ignore crate on plugins of bevy_audio, bevy_render, bevy_sprite…

76b3542

…s, bevy_text, bevy_ui, bevy_transform, bevy_winit

Updated ambiguity_checker example

09aff2b

Updated ReportExecutionOrderAmbiguities docs

c9aff48

Removed the information of BEVY_CRATES from CONTRIBUTING.md, since it…

4f58ad0

… doesn't exists anymore

alice-i-cecile added the S-Needs-Migration-Guide label Sep 14, 2021

alice-i-cecile approved these changes Sep 14, 2021

View reviewed changes

alice-i-cecile mentioned this pull request Dec 12, 2021

Ambiguity checker emits a lot of warnings with default plugins #1868

Closed

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 15, 2021

alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed S-Needs-Migration-Guide labels Jan 20, 2022

alice-i-cecile reviewed Jan 30, 2022

View reviewed changes

alice-i-cecile mentioned this pull request Jan 31, 2022

Report execution order ambiguities in tests #3818

Open

maniwani reviewed Mar 16, 2022

View reviewed changes

alice-i-cecile mentioned this pull request Mar 23, 2022

Improvements to execution order ambiguity reporting #4299

Closed

4 tasks

alice-i-cecile closed this Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simple improvements to ambiguity reporting clarity #2766

Simple improvements to ambiguity reporting clarity #2766

afonsolage commented Sep 2, 2021 •

edited

Loading

alice-i-cecile commented Sep 2, 2021

afonsolage commented Sep 3, 2021

afonsolage commented Sep 4, 2021

afonsolage commented Sep 4, 2021

alice-i-cecile Sep 4, 2021

afonsolage Sep 4, 2021 •

edited

Loading

alice-i-cecile Sep 5, 2021

afonsolage commented Sep 4, 2021

cart Sep 10, 2021

afonsolage Sep 12, 2021

cart Sep 10, 2021

alice-i-cecile Sep 10, 2021

afonsolage Sep 11, 2021 •

edited

Loading

cart Sep 12, 2021

afonsolage Sep 12, 2021

afonsolage commented Sep 12, 2021

afonsolage commented Sep 17, 2021

MinerSebas commented Sep 17, 2021

alice-i-cecile Jan 30, 2022

alice-i-cecile commented Mar 3, 2022

bors bot commented Mar 3, 2022

alice-i-cecile commented Mar 3, 2022

maniwani left a comment •

edited

Loading

maniwani Mar 16, 2022 •

edited

Loading

alice-i-cecile commented Mar 23, 2022

Simple improvements to ambiguity reporting clarity #2766

Simple improvements to ambiguity reporting clarity #2766

Conversation

afonsolage commented Sep 2, 2021 • edited Loading

Objective

Solution

alice-i-cecile commented Sep 2, 2021

afonsolage commented Sep 3, 2021

afonsolage commented Sep 4, 2021

afonsolage commented Sep 4, 2021

Choose a reason for hiding this comment

afonsolage Sep 4, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afonsolage commented Sep 4, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afonsolage Sep 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afonsolage commented Sep 12, 2021

afonsolage commented Sep 17, 2021

MinerSebas commented Sep 17, 2021

Choose a reason for hiding this comment

alice-i-cecile commented Mar 3, 2022

bors bot commented Mar 3, 2022

try

alice-i-cecile commented Mar 3, 2022

maniwani left a comment • edited Loading

Choose a reason for hiding this comment

maniwani Mar 16, 2022 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented Mar 23, 2022

afonsolage commented Sep 2, 2021 •

edited

Loading

afonsolage Sep 4, 2021 •

edited

Loading

afonsolage Sep 11, 2021 •

edited

Loading

maniwani left a comment •

edited

Loading

maniwani Mar 16, 2022 •

edited

Loading