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

Improve handling of motion vectors for multimesh instances. #80414

Merged
merged 1 commit into from
Aug 10, 2023

Conversation

DarioSamo
Copy link
Contributor

Fixes #67287. There was a subtle error where due to how enabling motion vectors for multi-meshes was handled, only the first instance would have a valid transforms buffer and the rest would point to an invalid buffer.

  • This change moves over the responsibility of enabling motion vectors only when changes happen to the individual 3D transforms or the entire buffer itself.
  • It also fixes an unnecessary download of the existing buffer that'd get overwritten by the current cache if it exists.
  • Another fix is handling the case where the buffer was not set, and enabling motion vectors would not cause the buffer to be recreated correctly.

I've done some testing with the multi-mesh project on the referenced issue as well as making a script to animate the instances and check if motion vectors are being drawn.

More extensive testing is recommended on projects that make heavy use of multimesh and also use TAA.

Design quirks

  • I'm not entirely happy with the choice to use the global viewport counter for mesh storage to be able to determine it needs to enable motion vectors, but Juan and I could not determine a better solution due to how the classes interact. This downside is preferable to the previous behavior.
  • There's still instances where the multi-mesh code opts to do GPU readback which is very unnecessary, but RenderingDevice does not provide a way to do a direct buffer copy. I might look into implementing that in a separate PR.

@DarioSamo DarioSamo requested a review from a team as a code owner August 8, 2023 12:52
@Calinou Calinou added bug topic:rendering topic:3d cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Aug 8, 2023
@Calinou Calinou added this to the 4.2 milestone Aug 8, 2023
Fixes godotengine#67287. There was a subtle error where due to how enabling motion vectors for multi-meshes was handled, only the first instance would have a valid transforms buffer and the rest would point to an invalid buffer. This change moves over the responsibility of enabling motion vectors only when changes happen to the individual 3D transforms or the entire buffer itself. It also fixes an unnecessary download of the existing buffer that'd get overwritten by the current cache if it exists. Another fix is handling the case where the buffer was not set, and enabling motion vectors would not cause the buffer to be recreated correctly.
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.

The code looks good to me. Tested locally and it works in the MRP from the linked issue

@akien-mga akien-mga merged commit 67543e9 into godotengine:master Aug 10, 2023
@akien-mga
Copy link
Member

Thanks!

@DarioSamo
Copy link
Contributor Author

It seems this PR introduces a regression where the regular Platformer project's terrain breaks when TAA is enabled. I'm unsure as to why this happens yet but it might be reasonable to revert it until the fix is implemented and re-opening this PR.

@DarioSamo
Copy link
Contributor Author

#80552 should fix the regression.

@DarioSamo
Copy link
Contributor Author

DarioSamo commented Aug 12, 2023

There's another regression that I'm unsure how to fix at the moment introduced where particles are broken when TAA is enabled due to removing the following condition.

if (p_color_pass_flags & COLOR_PASS_FLAG_MOTION_VECTORS) {
    if ((flags & (INSTANCE_DATA_FLAG_MULTIMESH | INSTANCE_DATA_FLAG_PARTICLES)) == INSTANCE_DATA_FLAG_MULTIMESH && RendererRD::MeshStorage::get_singleton()->_multimesh_enable_motion_vectors(inst->data->base)) {
        inst->transforms_uniform_set = mesh_storage->multimesh_get_3d_uniform_set(inst->data->base, scene_shader.default_shader_rd, TRANSFORMS_UNIFORM_SET);
    }
}

As per Juan's recommendation, the condition for enabling multimesh motion vectors was moved to mesh storage itself. However, mesh storage is unable to know whether the multimesh is used by particles or not.

It seems enabling multimesh motion vectors in general will break particles in the current implementation. So this PR also introduces that regression unless it can be figured out how to exclude particles from this or fix the issue which affects the particle transforms in particular.

If we'd like to keep the fix as it is we'd need to introduce a way a new flag for multimesh to support enabling motion vectors or not upon creation. For the moment I'll keep researching into how particle rendering uses this and why it's affected, but if not we might need to revert this before considering this for 4.2.

@DarioSamo
Copy link
Contributor Author

After discussing with @clayjohn I'll be looking at attempting to implement MVs for Particles during the week instead so we can fix the root cause of the problem.

DarioSamo added a commit to DarioSamo/godot that referenced this pull request Aug 13, 2023
…engine#80414.

There was an error in the other branch of the refactored function where the size of the array was not properly multiplied by the size of the float to check against the buffer size. This was only an error in the error-checking itself and not the functionality. There was also an error where the proper notification was not emitted whenever the buffer for the multimesh is recreated to invalidate the previous references the renderer might've created to it. This fixes CPU Particles getting corrupted when they're created without emission being enabled.
@DarioSamo
Copy link
Contributor Author

DarioSamo commented Aug 13, 2023

As it turns out, the issue was not for GPU particles but for CPU particles, so merely introducing the dependency notification to discard the uniform set when the multimesh buffer is recreated seems to have fixed the problem. I've appended the fix in #80552, so if nothing new shows up there should be no need to revert this.

akien-mga added a commit that referenced this pull request Aug 14, 2023
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2 (together with #80552).

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
YuriSizov pushed a commit to YuriSizov/godot that referenced this pull request Sep 21, 2023
…engine#80414.

There was an error in the other branch of the refactored function where the size of the array was not properly multiplied by the size of the float to check against the buffer size. This was only an error in the error-checking itself and not the functionality. There was also an error where the proper notification was not emitted whenever the buffer for the multimesh is recreated to invalidate the previous references the renderer might've created to it. This fixes CPU Particles getting corrupted when they're created without emission being enabled.

(cherry picked from commit 420f389)
mandryskowski pushed a commit to mandryskowski/godot that referenced this pull request Oct 11, 2023
…engine#80414.

There was an error in the other branch of the refactored function where the size of the array was not properly multiplied by the size of the float to check against the buffer size. This was only an error in the error-checking itself and not the functionality. There was also an error where the proper notification was not emitted whenever the buffer for the multimesh is recreated to invalidate the previous references the renderer might've created to it. This fixes CPU Particles getting corrupted when they're created without emission being enabled.
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.

Duplicated Multimesh breaks when TAA is active
6 participants