Skip to content

Improve formatting for execution order ambiguity reports #6071

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

Closed
wants to merge 11 commits into from

Conversation

joseph-gio
Copy link
Member

Related to #4299

Objective

This is how ambiguities are reported currently.

 * Parallel systems:
 -- "bevy_render::camera::camera::camera_system<bevy_render::camera::projection::Projection>" and "bevy_ui::widget::text::text_system"
    conflicts: ["bevy_asset::assets::Assets<bevy_render::texture::image::Image>"]
 -- "bevy_render::view::visibility::update_frusta<bevy_render::camera::projection::OrthographicProjection>" and "bevy_render::view::visibility::update_frusta<bevy_render::camera::projection::PerspectiveProjection>"
    conflicts: ["bevy_render::primitives::Frustum"]
 -- "bevy_render::view::visibility::update_frusta<bevy_render::camera::projection::OrthographicProjection>" and "bevy_render::view::visibility::update_frusta<bevy_render::camera::projection::Projection>"
    conflicts: ["bevy_render::primitives::Frustum"]
 -- "bevy_render::view::visibility::update_frusta<bevy_render::camera::projection::PerspectiveProjection>" and "bevy_render::view::visibility::update_frusta<bevy_render::camera::projection::Projection>"
    conflicts: ["bevy_render::primitives::Frustum"]
 -- "bevy_text::text2d::update_text2d_layout" and "bevy_ui::widget::image::image_node_system"
    conflicts: ["bevy_asset::assets::Assets<bevy_render::texture::image::Image>"]
 -- "bevy_text::text2d::update_text2d_layout" and "bevy_ui::widget::text::text_system"
    conflicts: ["bevy_asset::assets::Assets<bevy_render::texture::image::Image>", "bevy_asset::assets::Assets<bevy_sprite::texture_atlas::TextureAtlas>", "bevy_asset::assets::Assets<bevy_text::font_atlas_set::FontAtlasSet>", "bevy_text::pipeline::TextLayoutInfo", "bevy_text::pipeline::TextPipeline"]
 -- "bevy_ui::widget::image::image_node_system" and "bevy_ui::widget::text::text_system"
    conflicts: ["bevy_asset::assets::Assets<bevy_render::texture::image::Image>", "bevy_ui::ui_node::CalculatedSize"]

Solution

  • Group systems that are all ambiguous with one another.
  • Break up noise with whitespace.
  • Use vertical lists, so names have a consistent horizontal position. This makes scanning for a specific name far easier.
  • Assign a number to each ambiguity, to make them easier to discuss in a team.

Example output

(9) Ambiguous system ordering - Parallel systems
 * bevy_text::text2d::update_text2d_layout
 * bevy_ui::widget::text::text_system

 Data access conflicts:
 * bevy_asset::assets::Assets<bevy_render::texture::image::Image>
 * bevy_asset::assets::Assets<bevy_sprite::texture_atlas::TextureAtlas>
 * bevy_asset::assets::Assets<bevy_text::font_atlas_set::FontAtlasSet>
 * bevy_text::pipeline::TextPipeline
 * bevy_text::pipeline::TextLayoutInfo

(10) Ambiguous system ordering - Parallel systems
 * bevy_ui::widget::image::image_node_system
 * bevy_ui::widget::text::text_system

 Data access conflicts:
 * bevy_asset::assets::Assets<bevy_render::texture::image::Image>
 * bevy_ui::ui_node::CalculatedSize

(11) Ambiguous system ordering - Parallel systems
 * bevy_render::view::visibility::update_frusta<bevy_render::camera::projection::OrthographicProjection>
 * bevy_render::view::visibility::update_frusta<bevy_render::camera::projection::PerspectiveProjection>
 * bevy_render::view::visibility::update_frusta<bevy_render::camera::projection::Projection>

 Data access conflicts:
 * bevy_render::primitives::Frustum

(12) Ambiguous system ordering - Parallel systems
 * bevy_pbr::light::assign_lights_to_clusters
 * bevy_pbr::light::update_directional_light_frusta

 Data access conflicts:
 * bevy_render::primitives::Frustum

@alice-i-cecile 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 23, 2022
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.

I think ultimately we probably want:

  1. String-based formatting
  2. tracing warn! macro
  3. Use of shortnames by default

but I'm fine to defer that work to a follow-up PR(s). I like the error output much better now.

(Also: yay for tests being merged so I don't need to worry about whether the core logic was broken.)

@alice-i-cecile alice-i-cecile added this to the Bevy 0.9 milestone Sep 23, 2022
Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

Seems OK, but I'm not confident enough to assess the implementation since I'm not familiar with ambiguity detection internals.

@hymm hymm self-requested a review September 27, 2022 18:46
@alice-i-cecile
Copy link
Member

[3:23 PM]JoJoJet: I don't think it's worth pursuing right now. it has a bug in the grouping algorithm and i haven't been able to figure out what that is

From Discord.

@alice-i-cecile alice-i-cecile removed this from the Bevy 0.9 milestone Oct 22, 2022
@joseph-gio joseph-gio closed this Dec 5, 2022
@joseph-gio joseph-gio deleted the group-ambiguity branch January 13, 2023 02:30
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-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants