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 massive performance hit due to enabling collision #83749

Merged

Conversation

k0T0z
Copy link
Contributor

@k0T0z k0T0z commented Oct 21, 2023

Fixes #83744

Do we need to modify this patch #76598?

@k0T0z k0T0z force-pushed the fix-enabling-particle-turbulence branch from 4e5ff02 to eb8fb25 Compare October 21, 2023 19:49
@k0T0z k0T0z changed the title Fix performance hit due to enabling collision Fix massive performance hit due to enabling collision Oct 22, 2023
@Chaosus Chaosus added this to the 4.2 milestone Oct 22, 2023
@RPicster
Copy link
Contributor

Looks good to me, I also tested the PR and it works as expected.
In my game it even gave a noticable performance increase.

@k0T0z
Copy link
Contributor Author

k0T0z commented Oct 22, 2023

@RPicster Great, I am gonna add a comment for explanation now. Please approve that we need to make any changes to #76598 as well.

@k0T0z k0T0z force-pushed the fix-enabling-particle-turbulence branch from eb8fb25 to 771bcda Compare October 22, 2023 11:12
@k0T0z k0T0z force-pushed the fix-enabling-particle-turbulence branch from 771bcda to a3460b2 Compare October 22, 2023 14:50
Signed-off-by: Saif Kandil <74428638+k0T0z@users.noreply.github.com>
@k0T0z k0T0z force-pushed the fix-enabling-particle-turbulence branch from a3460b2 to 98db2b4 Compare October 22, 2023 14:54
@k0T0z
Copy link
Contributor Author

k0T0z commented Oct 22, 2023

What I see is that this snippet is a simple calculation going on there, so why there's a massive performance hit happening here? Maybe the rand_from_seed() function? dk for sure

if (!COLLIDED) {
	float vel_mag = length(VELOCITY);
	float vel_infl = clamp(mix(turbulence_influence_min, turbulence_influence_max, rand_from_seed(alt_seed)) * turbulence_influence, 0.0, 1.0);
	VELOCITY = mix(VELOCITY, normalize(noise_direction) * vel_mag * (1.0 + (1.0 - vel_infl) * 0.2), vel_infl);
}

EDIT: I noticed the mix() function here, I read it min(), dk what it means. If someone can explain why this snippet causes these big differences in FPS, I would really appreciate it.

@clayjohn
Copy link
Member

What I see is that this snippet is a simple calculation going on there, so why there's a massive performance hit happening here? Maybe the rand_from_seed() function? dk for sure

if (!COLLIDED) {
	float vel_mag = length(VELOCITY);
	float vel_infl = clamp(mix(turbulence_influence_min, turbulence_influence_max, rand_from_seed(alt_seed)) * turbulence_influence, 0.0, 1.0);
	VELOCITY = mix(VELOCITY, normalize(noise_direction) * vel_mag * (1.0 + (1.0 - vel_infl) * 0.2), vel_infl);
}

EDIT: I noticed the mix() function here, I read it min(), dk what it means. If someone can explain why this snippet causes these big differences in FPS, I would really appreciate it.

Godot detects when the COLLIDED keyword is used. If it's used anywhere in the shader then Godot will generate the screen space SDF for collisions. The performance cost is not from the shader itself, but from the cost of generating the SDF. If the SDF is used by another effect, then using COLLIDED wouldn't add additional cost

@k0T0z
Copy link
Contributor Author

k0T0z commented Oct 22, 2023

@clayjohn I noticed COLLIDED is only used in branches where the collision is enabled, yeah, it's hard to know about Godot detects any use of COLLIDED anywhere alone, I get it now thanks for clarifying 👌

@akien-mga
Copy link
Member

Do we need to modify this patch #76598?

WDYT @Calinou @RPicster?

@RPicster
Copy link
Contributor

I don't know if that was the root of the cause of the original reason for the performance problem. Turbulence will always come at an increased cost. So eventually rather be safe than sorry with the documentation?

@Calinou
Copy link
Member

Calinou commented Oct 23, 2023

I don't know if that was the root of the cause of the original reason for the performance problem. Turbulence will always come at an increased cost. So eventually rather be safe than sorry with the documentation?

Indeed, we still expect turbulence to take up a significant amount of GPU time on integrated graphics.

@akien-mga akien-mga merged commit bbade19 into godotengine:master Oct 24, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

Enabling particle turbulence causes massive performance hit
7 participants