Skip to content

Run RenderStartup in/before extract instead of after it. #19926

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
Jul 7, 2025

Conversation

andriyDev
Copy link
Contributor

Objective

Solution

  • First, allow extraction function to be FnMut instead of Fn. FnMut is a superset of Fn anyway, and we only ever call this function once at a time (we would never call this in parallel for different pairs of worlds or something).
  • Run the RenderStartup schedule in the extract function with a flag to only do it once.
  • Remove all the MainRender stuff.

One sad part here is that now the RenderStartup blocks extraction. So for pipelined rendering, our simulation will be blocked on the first frame while we set up all the rendering resources. I don't see this as a big loss though since A) that is fundamentally what we want here - extraction has to run after RenderStartup, and the only way to do better is to somehow run RenderStartup in parallel with the first simulation frame, and B) without RenderStartup the entire app was blocked on initializing render resources during Plugin construction - so we're not really losing anything here.

Testing

@andriyDev andriyDev added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen A-ECS Entities, components, systems, and events D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 2, 2025
@andriyDev andriyDev added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 2, 2025
@andriyDev andriyDev force-pushed the render-startup-order branch from 62980f9 to db24864 Compare July 2, 2025 18:33
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

I'm okay with blocking extraction on that part personally. We can't really render anything anyway until those resources are initialized.

I'd prefer if the ecs folks reviewed this change though because I have no idea if it's correct to use FnMut here or if this approach fits in the context of everything else.

@andriyDev andriyDev requested a review from ItsDoot July 2, 2025 19:36
Copy link
Contributor

@kristoff3r kristoff3r left a comment

Choose a reason for hiding this comment

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

Looks like a good solution to me, you kinda have to block on the first frame and FnMut is hardly a restriction in this context.

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.

One nit, but fundamentally this strategy is totally fine from an ECS perspective. Mutating state in the ExtractFn doesn't cause any problems.

That said, the migration guide is missing advice on what happened to MainRender.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 7, 2025
@alice-i-cecile alice-i-cecile added this to the 0.17 milestone Jul 7, 2025
@andriyDev
Copy link
Contributor Author

andriyDev commented Jul 7, 2025

@alice-i-cecile MainRender was introduced in #19841 which happened after 0.16, so there's no need for a migration guide for this.

We also didn't add a migration guide for #19841, but I will write one as well as a release note before 0.17 (in a separate PR). I wanted to make more progress on these ports before I write it up.

@andriyDev andriyDev added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 7, 2025
@andriyDev andriyDev requested a review from alice-i-cecile July 7, 2025 00:59
@alice-i-cecile alice-i-cecile removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 7, 2025
@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 Jul 7, 2025
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 7, 2025
Merged via the queue into bevyengine:main with commit 2c6cf9a Jul 7, 2025
38 checks passed
@github-project-automation github-project-automation bot moved this to Done in Rendering Jul 7, 2025
@andriyDev andriyDev deleted the render-startup-order branch July 7, 2025 03:21
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 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
Status: Done
Development

Successfully merging this pull request may close these issues.

RenderStartup runs after ExtractSchedule
4 participants