Skip to content

prepare_view_upscaling_pipelines is ambiguous with dozens of other systems #14770

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
alice-i-cecile opened this issue Aug 15, 2024 · 1 comment · Fixed by #14772
Closed

prepare_view_upscaling_pipelines is ambiguous with dozens of other systems #14770

alice-i-cecile opened this issue Aug 15, 2024 · 1 comment · Fixed by #14772
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Contentious There are nontrivial implications that should be thought through

Comments

@alice-i-cecile
Copy link
Member

Bevy version

a2fc9de

What you did

cargo run --example ambiguity_detection

What went wrong

prepare_view_upscaling_pipelines is ambiguous with dozens of other systems, such as prepare_preprocess_bind_groups

This system uses ResMut<PipelineCache>, while every other system uses Res (via interior mutability).

Additional information

This was discovered as part of #7386. This system should run after all of its siblings, but doing so is quite intrusive.

@JMS55 suggests that we tackle this by making PipelineCache::block_on use interior mutability. Overall, the design here doesn't seem perfect. I'm inclined to just silence the ambiguity for now (which will have a similar effect) until a better solution is found.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! D-Straightforward Simple bug fixes and API improvements, docs, test and examples X-Contentious There are nontrivial implications that should be thought through labels Aug 15, 2024
@alice-i-cecile
Copy link
Member Author

Both ambiguous_with_all and allow_ambiguous_resource are reasonable, but I'd prefer the former because it's more localized.

github-merge-queue bot pushed a commit that referenced this issue Aug 16, 2024
# Objective

The `prepare_view_upscaling_pipelines` system has dozens of ambiguities,
which makes it harder to spot and prevent new ambiguities.

Closes #14770.

## Solution

Just exclude the system from ambiguity detection. See the linked issue
for more context on why this resolution was chosen.

## Testing

Running the `ambiguity_detection` example now reports dozens fewer
`Render` app ambiguities.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant