Skip to content
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

Fix instance uniform data buffer update delay #79603

Merged
merged 1 commit into from
Jul 25, 2023

Conversation

bitsawer
Copy link
Member

This PR simply moves dirty resource updates after instance updates, as instance updates can cause some resources to become dirty. This can cause an extra frame of delay when updating instance uniforms, multimesh data or particle data. For example in the MRP project in the linked issue, the update order currently goes like this:

  1. User code calls set_instance_shader_parameter(), data is sent to server.
  2. Rendering loop eventually calls RendererSceneCull::update_dirty_instances().
  3. RSG::utilities->update_dirty_resources() updates dirty resources, more importantly in this scenario it calls MaterialStorage::get_singleton()->_update_global_shader_uniforms() which buffer_update()'s dirty instance+global uniform data to GPU.
  4. Rendering code loops through dirty instances, calls _update_dirty_instance() for each of them and marks resources as dirty, including instance uniform buffer.

This PR basically reverses the order of steps 3 and 4. Because step 4 can actually mark resources as dirty but as the dirty resources were already uploaded to GPU in step 3 this will cause one frame update delay.

I tested this PR with some more complex demos and didn't notice any regressions, but these issues can be pretty subtle to notice.

Also, if this PR is accepted, the same reordering should probably done to this PR #77594 as it adds RendererCanvasCull::update_dirty_instances() which is functionally similar to RendererSceneCull implementation.

@bitsawer bitsawer added this to the 4.2 milestone Jul 18, 2023
@bitsawer bitsawer requested a review from a team as a code owner July 18, 2023 09:31
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

I looked carefully through _update_dirty_instance() and I can't see anything that would require the resources to be updated first (i.e. it isn't reading back any data that should be changed).

Therefore, I think this is safe to merge.

Because of the complexity of _update_dirty_instance(), I consider this a higher than normal risk merge, so we should not cherrypick this to a previous version.

@YuriSizov YuriSizov merged commit 0e9e373 into godotengine:master Jul 25, 2023
13 checks passed
@YuriSizov
Copy link
Contributor

Thanks!

@bitsawer bitsawer deleted the fix_instance_uniform_update branch August 28, 2023 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Shader instance uniforms are updated with one frame delay
3 participants