Ignore ambiguous components or resources by hymm · Pull Request #9895 · bevyengine/bevy

hymm · 2023-09-22T00:03:57Z

Objective

Fixes Ignore system order ambiguities from specific components and resources. #9884
Add API for ignoring ambiguities on certain resource or components.

Solution

Add a IgnoreSchedulingAmbiguitiy resource to the world which holds the ComponentIds to be ignored
Filter out ambiguities with those component id's.

Changelog

add allow_ambiguous_component and allow_ambiguous_resource apis for ignoring ambiguities

alice-i-cecile · 2023-09-22T00:12:47Z

@rj00a can I get your review on this as a user please?

alice-i-cecile

We should probably use this internally in this PR for some of our asset collection ambiguities.

hymm · 2023-09-22T01:42:54Z

@alice-i-cecile I added exceptions for Assets<Image> and Assets<TextureAtlas>, but do you think we should do this for all Assets<T>?

crates/bevy_ecs/src/schedule/schedule.rs

alice-i-cecile · 2023-09-22T03:16:09Z

crates/bevy_ecs/src/schedule/schedule.rs


+/// list of [`ComponentId`]'s to ignore when reporting ambiguity conflicts between systems
+#[derive(Resource, Default, Clone)]
+pub struct IgnoreSchedulingAmbiguities(Vec<ComponentId>);


This should have two distinct fields: one for resources, and one for components. Otherwise the API below doesn't make sense.

The difference between one or two fields can't be observed in the API, so this seems alright to me. Only reason I could think to split it would be for a nicer Debug impl.

I'm not a fan of splitting it up, makes the logic for filtering more complicated.

crates/bevy_ecs/src/schedule/schedule.rs

alice-i-cecile · 2023-09-22T03:23:59Z

@alice-i-cecile I added exceptions for Assets<Image> and Assets<TextureAtlas>, but do you think we should do this for all Assets<T>?

I think so: @cart's opinion was that basically all of these ambiguities are noise. Probably include it in the docs for asset registration too?

rj00a

The approach looks good to me.

crates/bevy_app/src/app.rs

crates/bevy_ecs/src/schedule/schedule.rs

Co-authored-by: Ryan Johnson <ryanj00a@gmail.com>

crates/bevy_ecs/src/schedule/schedule.rs

alice-i-cecile · 2023-09-28T17:52:07Z

crates/bevy_asset/src/lib.rs

    /// * Registering the [`Asset`] in the [`AssetServer`]
    /// * Initializing the [`AssetEvent`] resource for the [`Asset`]
    /// * Adding other relevant systems and resources for the [`Asset`]
+    /// * Ignoring schedule ambiguities in [`Assets`] resource. Any time a system takes


Nit: a unit test to validate that these docs stay correct would be nice.

james7132

Not sure how I like the idea of using a resource for this purpose. It seems trivial to alter behavior at runtime by mutating or removing the resource. IMO this should be a part of the World itself instead of included as an item we store in it.

alice-i-cecile · 2023-10-02T01:00:54Z

We already store Schedules as a resource to be fair. I'd rather make this a field of that than move it up to the World level.

hymm · 2023-10-03T02:57:14Z

I actually started with it as part of Schedules and moved it to a separate resource since it's felt like more a property of the world to be ambiguous on a component. I'm fine with moving it back into Schedules if that seems more appropriate or even making it a part of World. I don't have a strong opinion on this one.

alice-i-cecile · 2023-10-03T10:15:13Z

I think that this is tightly coupled to the scheduling information: long-term, I think that all of our cross-schedule config belongs under Schedules. Move it there and then I think we can merge.

# Objective - Fixes bevyengine#9884 - Add API for ignoring ambiguities on certain resource or components. ## Solution - Add a `IgnoreSchedulingAmbiguitiy` resource to the world which holds the `ComponentIds` to be ignored - Filter out ambiguities with those component id's. ## Changelog - add `allow_ambiguous_component` and `allow_ambiguous_resource` apis for ignoring ambiguities --------- Co-authored-by: Ryan Johnson <ryanj00a@gmail.com>

hymm added 4 commits September 21, 2023 12:54

ignore ambiguities

d7dbb52

refactor to store ignored to resource on world

5c56cbc

clean up

e8339f6

add methods on app

8354ee9

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 labels Sep 22, 2023

alice-i-cecile requested changes Sep 22, 2023

View reviewed changes

fix docs

e0af218

alice-i-cecile reviewed Sep 22, 2023

View reviewed changes

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

alice-i-cecile reviewed Sep 22, 2023

View reviewed changes

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

alice-i-cecile reviewed Sep 22, 2023

View reviewed changes

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

alice-i-cecile reviewed Sep 22, 2023

View reviewed changes

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

ignore ambiguities on Assets<T>

d5f0864

hymm force-pushed the ambiguous-components branch from 24e1525 to d5f0864 Compare September 22, 2023 05:12

rj00a reviewed Sep 22, 2023

View reviewed changes

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved

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

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

hymm mentioned this pull request Sep 22, 2023

Add Explicit Ordering for DefaultPlugins Systems #9828

Closed

hymm and others added 5 commits September 22, 2023 13:32

ignore -> ignored

5b36699

use BTreeSet instead of Vec

fc34034

add pretty printer for ignored ambiguities

3330c4a

Apply suggestions from code review

e93b97a

Co-authored-by: Ryan Johnson <ryanj00a@gmail.com>

reword docs

a2db340

rj00a reviewed Sep 23, 2023

View reviewed changes

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

return impl Iterator

3d407c5

hymm force-pushed the ambiguous-components branch from 14a6808 to 3d407c5 Compare September 26, 2023 16:39

rj00a approved these changes Sep 27, 2023

View reviewed changes

alice-i-cecile reviewed Sep 28, 2023

View reviewed changes

alice-i-cecile approved these changes Sep 28, 2023

View reviewed changes

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 Sep 28, 2023

add test for ignoring assets ambiguities

4e988c1

james7132 reviewed Oct 2, 2023

View reviewed changes

alice-i-cecile added this to the 0.12 milestone Oct 2, 2023

move ignore ambiguities into schedules

84da2af

alice-i-cecile added this pull request to the merge queue Oct 4, 2023

Merged via the queue into bevyengine:main with commit 7c5b324 Oct 4, 2023

rj00a mentioned this pull request Oct 4, 2023

Resolve system order ambiguities valence-rs/valence#542

Open

mockersf mentioned this pull request Oct 4, 2023

Allow overriding global wireframe setting. #7328

Merged

hymm deleted the ambiguous-components branch October 5, 2023 16:12

cart mentioned this pull request Oct 13, 2023

News: Release 0.12 bevyengine/bevy-website#754

Merged

43 tasks

Uh oh!

Conversation

hymm commented Sep 22, 2023

Objective

Solution

Changelog

Uh oh!

alice-i-cecile commented Sep 22, 2023

Uh oh!

alice-i-cecile left a comment

Choose a reason for hiding this comment

Uh oh!

hymm commented Sep 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

rj00a Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

hymm Sep 22, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile commented Sep 22, 2023

Uh oh!

rj00a left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

alice-i-cecile Sep 28, 2023

Choose a reason for hiding this comment

Uh oh!

james7132 left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Oct 2, 2023

Uh oh!

hymm commented Oct 3, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alice-i-cecile commented Oct 3, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

hymm commented Sep 22, 2023 •

edited

Loading

hymm commented Oct 3, 2023 •

edited

Loading