Skip to content
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

Add Explicit Ordering for DefaultPlugins Systems #9828

Conversation

bushrat011899
Copy link
Contributor

Objective

Solution

  • Added before and after statements as required to remove system order ambiguity.

Notes

This PR is a bit on the rough side of things and probably serves better as a reference point for the kind of work required to properly resolve #9511. A number of system ambiguities were between areas of Bevy that previously didn't need to reference each other. In those instances, I chose to add the crate and resolve the ambiguity directly. A better approach would probably be to add some better sets for sharing between these crates.

Also, I had some troubles on my machine actually diagnosing the original ambiguity errors using the nondeterministic_system_order example. I ended up working around this issue by explicitly configuring all the schedules run by Main separately:

// ...
.edit_schedule(Main, |schedule| {
    schedule.set_build_settings(ScheduleBuildSettings {
        ambiguity_detection: LogLevel::Error,
        ..default()
    });
})
.edit_schedule(PreStartup, |schedule| {
    schedule.set_build_settings(ScheduleBuildSettings {
        ambiguity_detection: LogLevel::Error,
        ..default()
    });
})
.edit_schedule(Startup, |schedule| {
    schedule.set_build_settings(ScheduleBuildSettings {
        ambiguity_detection: LogLevel::Error,
        ..default()
    });
})
// ... etc.

If this is merged, I do think the CI tests should be updated to include ambiguity detection to prevent regression.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior X-Controversial There is active debate or serious implications around merging this PR A-ECS Entities, components, systems, and events labels Sep 17, 2023
@alice-i-cecile alice-i-cecile self-requested a review September 17, 2023 15:58
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should avoid adding new dependencies between crates just for ambiguity resolution. Instead, I think we should add a plugin to DefaultPlugins, whose only job is to resolve these ambiguities using public labels.

@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Sep 17, 2023
@NiklasEi
Copy link
Member

Instead of ordering everything, some systems could also be added to ambiguity sets to allow for more parallelism. E.g. the animation system might be OK with running in parallel to layout systems.

@bushrat011899 bushrat011899 reopened this Sep 17, 2023
@bushrat011899 bushrat011899 marked this pull request as draft September 17, 2023 22:44
@hymm
Copy link
Contributor

hymm commented Sep 22, 2023

FYI I have a couple PR's that will affect ambiguitites.

@alice-i-cecile
Copy link
Member

Closing in favor of #10411<3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bevy contains internal system-order ambiguities
4 participants