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

System order ambiguity time_system and event_update_system #14524

Closed
thmxv opened this issue Jul 29, 2024 · 5 comments · Fixed by #14544
Closed

System order ambiguity time_system and event_update_system #14524

thmxv opened this issue Jul 29, 2024 · 5 comments · Fixed by #14544
Labels
A-Time Involves time keeping and reporting C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!

Comments

@thmxv
Copy link

thmxv commented Jul 29, 2024

Bevy version

0.14.0 and git main branch most recent commit ca3d8e8

What you did

My app crashes because of a system order ambiguity.
My app has the following code to detect ambiguity:

fn main() {
    let mut app = App::new();
    app.configure_schedules(ScheduleBuildSettings {
        ambiguity_detection: LogLevel::Error,
        ..default()
    });
    ...
}

I reproduced this error/crash in bevy only by adding this code to the time/time.rs example.

What went wrong

thread 'main' panicked at crates/bevy_ecs/src/schedule/schedule.rs:383:33:
Error when initializing schedule First: Systems with conflicting access have indeterminate run order.
1 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these:
 -- time_system (in set TimeSystem) and event_update_system (in set EventUpdates)
    conflict on: bevy_ecs::world::World

Additional information

My app does have this crash but with 2 pairs of systems, not just the one. This crash comes from a dependency of a dependency of my app: bevy_mod_raycast

2 pairs of systems with conflicting data access have indeterminate execution order. Consider adding `before`, `after`, or `ambiguous_with` relationships between these:
 -- time_system (in set TimeSystem) and event_update_system (in set EventUpdates)
    conflict on: bevy_ecs::world::World
 -- update_cursor_ray and event_update_system (in set EventUpdates)
    conflict on: bevy_ecs::world::World

The update_cursor_ray system comes from bevy_mod_raycast. But because event_update_system comes from Bevy and is common to the 2 pairs of systems, I do not know if I should report a similar issue there.

@thmxv thmxv added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Jul 29, 2024
@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! A-Time Involves time keeping and reporting and removed S-Needs-Triage This issue needs to be labelled labels Jul 29, 2024
@alice-i-cecile
Copy link
Member

I don't mind resolving the ambiguity either way: do you have a preference?

@thmxv
Copy link
Author

thmxv commented Jul 29, 2024

I do not have any preference. I suppose the time_system and event_update_system are ambiguous but this is not a problem. In this case maybe using ambiguous_with() and not before() nor after() is the "correct" way. But I have no real knowledge of those systems and new to Bevy, so you sure know better than me what is better.

@alice-i-cecile
Copy link
Member

Hmm yeah, I actually think that these two systems aren't actually ambiguous in meaningful ways, since the event system work won't touch time despite having world access. Yeah, marked as ambiguous with is the right approach here.

For the actual crash, you need an ordering in bevy_mod_raycast :)

@thmxv
Copy link
Author

thmxv commented Jul 29, 2024

Thanks. To avoid the crash, I changed from LogLevel::Error to LogLevel::Warn, which only prints warning messages but do no crash the app. I will report the issue in bevy_mod_raycast.

@richchurcher
Copy link
Contributor

As this issue is marked with trivial, I might take a stab at closing it 🤞

richchurcher added a commit to richchurcher/bevy that referenced this issue Jul 31, 2024
richchurcher added a commit to richchurcher/bevy that referenced this issue Jul 31, 2024
github-merge-queue bot pushed a commit that referenced this issue Jul 31, 2024
# Objective

Resolve possible ambiguity detection panic between `time_system` and
`event_update_system`.

Fixes #14524

## Solution

Sets `.ambiguous_with(event_update_system)` on `time_system`. This is
slightly new territory for me, so please treat with scepticism.

## Testing

As described in the issue, added
```
        .configure_schedules(ScheduleBuildSettings {
            ambiguity_detection: LogLevel::Error,
            ..default()
        })
```
to the `time` example and ran it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Time Involves time keeping and reporting C-Bug An unexpected or incorrect behavior D-Trivial Nice and easy! A great choice to get started with Bevy S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants