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 turbulence_noise_scale for Particle Turbulence #77631

Merged

Conversation

KdotJPG
Copy link
Contributor

@KdotJPG KdotJPG commented May 29, 2023

Fixes #77491 / my own oversight from #64606/#77154: turbulence_noise_scale needs to have 1 subtracted from it for parity with 4.0.

Curl evaluation in 4.0 here is
curl_3d(noise_pos + noise_time - diff, ...)

= (pos * turbulence_noise_scale - emission_pos) + noise_time - (pos - emission_pos)
= pos * turbulence_noise_scale - emission_pos + noise_time - pos + emission_pos
= pos * turbulence_noise_scale + noise_time - pos
= pos * (turbulence_noise_scale - 1) + noise_time

The position is being effectively multipled by turbulence_noise_scale - 1, but in 4.1 it's only multiplied by turbulence_noise_scale.

The 4.0 formula in set_turbulence_noise_scale is (pow(p_turbulence_noise_scale, 0.25) * 5.6234 / 10.0) * 4.0 - 3.0. This PR changes it to (pow(p_turbulence_noise_scale, 0.25) * 5.6234 / 10.0) * 4.0 - 4.0 to compensate, then simplifies the successive multiplications and divisions to (pow(p_turbulence_noise_scale, 0.25) * 2.24936) - 4.0.

Curve: https://www.desmos.com/calculator/xyzbgqaggw (correctly matches slider range of 0 to 10 again)

image
image
image

@KdotJPG KdotJPG changed the title #77491 Fix turbulence_noise_scale for Particule Turbulence #77491 Fix turbulence_noise_scale for Particle Turbulence May 29, 2023
@akien-mga akien-mga changed the title #77491 Fix turbulence_noise_scale for Particle Turbulence Fix turbulence_noise_scale for Particle Turbulence May 29, 2023
Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Thanks for working on fixing that regression! I haven't run the calculations myself but the explanations make sense, so I'm assuming they're correct :)

One suggestion: Should we make these magic numbers more explicit with a comment or a link to a reference where they can be derived from easily? Currently that scaling factor looks pretty arbitrary. (I'm not familiar with that code though so I might be expected here.)

Another workflow related note: We prefer commit titles not to include a direct link to a GitHub URL, but only a plain English description of the bug being fixed. The link to the GH issue should go to the body of the commit message, using one of GitHub's closing keywords (Fixes #77491). E.g.:

Fix particles `turbulence_noise_scale` regression

Fixes #77491.

@KdotJPG
Copy link
Contributor Author

KdotJPG commented May 30, 2023

One suggestion: Should we make these magic numbers more explicit with a comment or a link to a reference where they can be derived from easily? Currently that scaling factor looks pretty arbitrary. (I'm not familiar with that code though so I might be expected here.)

Good idea. We could do something like this:

void ParticleProcessMaterial::set_turbulence_noise_scale(float p_turbulence_noise_scale) {
	turbulence_noise_scale = p_turbulence_noise_scale;
	const float noise_frequency_when_slider_is_zero = 4.0;
	const float max_slider_value = 10.0;
	const float curve_exponent = 0.25;
	const float curve_rescale = noise_frequency_when_slider_is_zero / pow(max_slider_value, curve_exponent);
	float shader_turbulence_noise_scale = pow(p_turbulence_noise_scale, curve_exponent) * curve_rescale - noise_frequency_when_slider_is_zero;
	RenderingServer::get_singleton()->material_set_param(_get_material(), shader_names->turbulence_noise_scale, shader_turbulence_noise_scale);
}

@KdotJPG KdotJPG force-pushed the issue-77491-fix-turbulence-noise-scale branch from dcd2e0b to e5d3364 Compare May 30, 2023 21:53
@KdotJPG
Copy link
Contributor Author

KdotJPG commented May 30, 2023

Pushed the above and adjusted commit message.

@KdotJPG KdotJPG force-pushed the issue-77491-fix-turbulence-noise-scale branch from e5d3364 to 9332a09 Compare May 31, 2023 03:58
@YuriSizov YuriSizov merged commit 943e5b9 into godotengine:master May 31, 2023
@YuriSizov
Copy link
Contributor

Thanks a lot!

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.

Particle turbulence behaves very differently in 4.1.dev3
3 participants