Skip to content

Add RenderSet::FinalCleanup for World::clear_entities #14764

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

Merged
merged 5 commits into from
Aug 15, 2024

Conversation

alice-i-cecile
Copy link
Member

@alice-i-cecile alice-i-cecile commented Aug 15, 2024

Objective

World::clear_entities is ambiguous with all of the other systems in RenderSet::Cleanup because it access &mut World.

Solution

I've added another system set variant, and made sure that this runs after everything else.

Testing

The ambiguity_detection example

Migration Guide

World::clear_entities is now part of RenderSet::PostCleanup rather than RenderSet::Cleanup. Your cleanup systems should likely stay in RenderSet::Cleanup.

Additional context

Spotted when working on #7386: this was responsible for a large number of ambiguities.

This should be removed if / when #14449 is merged: there's no need to call clear_entities at all if the rendering world is retained!

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 15, 2024
Copy link
Contributor

@JMS55 JMS55 left a comment

Choose a reason for hiding this comment

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

Kinda gross, but retained rendering world will fix it (and give very nice perf improvements!)

@ItsDoot
Copy link
Contributor

ItsDoot commented Aug 15, 2024

Nit: PostCleanup matches our schedule naming scheme better

@alice-i-cecile alice-i-cecile requested a review from ItsDoot August 15, 2024 18:18
Copy link
Contributor

@ItsDoot ItsDoot left a comment

Choose a reason for hiding this comment

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

Don't forget to update the migration guide 👍

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 15, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 15, 2024
Merged via the queue into main with commit a2fc9de Aug 15, 2024
26 checks passed
@JMS55
Copy link
Contributor

JMS55 commented Aug 15, 2024

I actually dislike PostCleanup more, as it implies that all cleanup is actually done and that it's not doing cleanup itself... But this whole thing is sucky, and hopefully fixed soon. We really need to get retained render world merged asap imo.

@mockersf mockersf deleted the final-cleanup branch October 22, 2024 22:56
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-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants