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

Add motion vector support for GPU 3D Particles #80688

Merged

Conversation

DarioSamo
Copy link
Contributor

Add the capability of resizing the transforms buffer for particles to be double its size and alternate where the current output is written to. Only works for particles that use index as their draw order.

@AThousandShips AThousandShips added this to the 4.x milestone Aug 16, 2023
@DarioSamo DarioSamo marked this pull request as ready for review August 17, 2023 14:46
@DarioSamo DarioSamo requested a review from a team as a code owner August 17, 2023 14:46
@DarioSamo
Copy link
Contributor Author

I think this is ready for testing.

@Calinou
Copy link
Member

Calinou commented Aug 17, 2023

Only works for particles that use index as their draw order.

This should be mentioned in the draw_order property description of doc/classes/GPUParticles3D.xml (or ParticleProcessMaterial, I forgot), as well as in the use_taa description in doc/classes/ProjectSettings.xml.

@DarioSamo
Copy link
Contributor Author

DarioSamo commented Aug 17, 2023

I'm not entirely sure if it'll need to be documented that much because users don't have a need to edit that setting unless the draw order is important. And draw order is important only when transparent particles are involved, which shouldn't be involved in the motion vectors pass anyway (which is actually a general current bug because any transparent objects can draw to the motion vector buffer right now).

In the case of opaque particles, there should be no need for the user to ever change this setting from the default. I'm fine with documenting it but it might end up being visual noise as it'll be irrelevant in most cases once the transparency writing motion vectors bug is fixed.

Gonna tag @reduz for some input on this as it's something we discussed.

Add the capability of resizing the transforms buffer for particles to be double its size and alternate where the current output is written to. Only works for particles that use index as their draw order.
@DarioSamo DarioSamo requested a review from a team as a code owner August 28, 2023 13:56
@DarioSamo
Copy link
Contributor Author

I've added a comment for GPU Particles but don't think it'll be necessary for the use_taa setting as it shouldn't cause issues if we fix the transparent particles writing motion vectors properly.

I'll see about addressing that issue in a separate PR before we merge this one.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally, it works as expected.

Testing project: test_pr_80688.zip
Particles on the left use the default 30 Hz update rate + interpolation. Particles on the right update every rendered frame and have interpolation disabled.
This project is capped to 60 FPS by default to ensure TAA appearance is consistent across displays.

Before

simplescreenrecorder-2023-08-28_18.20.54.mp4

After (this PR)

simplescreenrecorder-2023-08-28_18.21.20.mp4

Performance is pretty much identical before and after this PR.

@clayjohn clayjohn modified the milestones: 4.x, 4.2 Aug 29, 2023
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.

Looks great! I reviewed the code, but I didn't test locally as I have a big refactor on my local and I don't want to recompile from scratch twice. I trust the previous reviews, so let's go ahead with this now

@clayjohn clayjohn added cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release bug and removed enhancement labels Aug 29, 2023
@akien-mga akien-mga merged commit 4b69e8b into godotengine:master Aug 29, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

@YuriSizov
Copy link
Contributor

Cherry-picked for 4.1.2.

@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
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.

6 participants