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 GPUParticles2D not randomizing seed when set_one_shot is called #101688

Conversation

TCROC
Copy link
Contributor

@TCROC TCROC commented Jan 17, 2025

gpu_particelse_2d needed fixing in this regard as well as pointed out by @clayjohn and @QbieShay: #101596 (comment)

The seed should be randomized by calls to set_one_shot. Although the behavior is redundant imo, @clayjohn made a solid point that we should still do it so that the behavior remains the same for gpu particles between 4.3 and 4.4.

@TCROC TCROC requested a review from a team as a code owner January 17, 2025 14:09
@TCROC
Copy link
Contributor Author

TCROC commented Jan 17, 2025

Why does it want me to update the docs? I presume this is a false flag? All I changed was the internals of the set_one_shot method.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 17, 2025

It's not a false positive, it changes the default value, as set_one_shot is called in the constructor

Did you intend this to change the default seed?

@TCROC
Copy link
Contributor Author

TCROC commented Jan 17, 2025

Ah fascinating... This is the same issue we ran into in gpu_particles_3d although it didn't flag the default value change... but it should still apply. Previously this was randomized but in a process method due to the side effect of a boolean. Now it is set directly in the method. This definitely isn't something we took into consideration... what are your thoughts on this @clayjohn and @QbieShay?

@AThousandShips here's the convo where we've been discussing in detail: https://chat.godotengine.org/channel/rendering/thread/Mgd4xD8bH3TZKrQND

@TCROC
Copy link
Contributor Author

TCROC commented Jan 17, 2025

@AThousandShips is Math::rand() deterministic? If so, I can update the docs to make this PR happy... otherwise its just gonna be luck of the draw on the PR

@AThousandShips
Copy link
Member

It is if you set the seed, but don't think we do that in the documentation generation, but I think the default seed should probably not be set randomly if it wasn't set to that before, but that needs to be discussed, I think the way this is done should be solved locally

(No need to tag me immediately again after tagging me)

@TCROC
Copy link
Contributor Author

TCROC commented Jan 17, 2025

My bad. Won't tag as much. Thanks for the heads up.

I think the way this is done should be solved locally

What do you mean by that?

@AThousandShips
Copy link
Member

As in it should probably ensure it doesn't reset the seed when constructing, I.e. only when calling set_one_shot manually, or we can reset the seed manually after set_one_shot(false) in the constructor

@TCROC
Copy link
Contributor Author

TCROC commented Jan 17, 2025

Ah I see. I like the 2nd approach. Is clean and simple. I will go do that

@TCROC
Copy link
Contributor Author

TCROC commented Jan 17, 2025

Why do you think this PR didn't have the same issue? #101596

@TCROC TCROC force-pushed the fix-gpu-particles-2d-set_one_shot-seed-randomization branch from 0c725d0 to fa4daea Compare January 17, 2025 14:34
@AThousandShips
Copy link
Member

I don't know I haven't studied the code closely, you'd have to go through the code and see if there are any differences in the initialization

@TCROC
Copy link
Contributor Author

TCROC commented Jan 17, 2025

Sounds good. I don't think either of the 2 PRs need to be held up on those questions though. A third PR to make 2d and 3d particles behave consistently may be better suited as we can discuss all those changes in one place. These 2 PRs should make the behavior of 2d and 3d particles the same going from 4.3 to 4.4. At least as far as clay and qbie noticed in the rocket chat

@TCROC TCROC force-pushed the fix-gpu-particles-2d-set_one_shot-seed-randomization branch from fa4daea to b461c6e Compare January 20, 2025 15:01
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.

Looks good to me.

@TCROC TCROC force-pushed the fix-gpu-particles-2d-set_one_shot-seed-randomization branch from b461c6e to e4ecdb7 Compare January 21, 2025 13:39
Copy link
Contributor

@QbieShay QbieShay left a comment

Choose a reason for hiding this comment

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

We spoke about this in the rendering meeting.

			if (!use_fixed_seed) {
				set_seed(Math::rand());
			}

should be removed both from gpuparticle2d and gpuparticle3d.

the function calls in the constructor (set seed and set use fixed seed) should be put both here and in gpuparticles3d

@TCROC
Copy link
Contributor Author

TCROC commented Jan 21, 2025

Sounds good. I will do that for this PR and open a new pr for 3d particles

@QbieShay
Copy link
Contributor

You can do all in the same PR, it's better for reverting just in case we need to.

@TCROC
Copy link
Contributor Author

TCROC commented Jan 21, 2025

Makes sense! Will do! :)

@TCROC TCROC force-pushed the fix-gpu-particles-2d-set_one_shot-seed-randomization branch from e4ecdb7 to 3973c0b Compare January 22, 2025 16:30
@TCROC TCROC requested a review from a team as a code owner January 22, 2025 16:30
@TCROC
Copy link
Contributor Author

TCROC commented Jan 22, 2025

We spoke about this in the rendering meeting.

			if (!use_fixed_seed) {
				set_seed(Math::rand());
			}

should be removed both from gpuparticle2d and gpuparticle3d.

the function calls in the constructor (set seed and set use fixed seed) should be put both here and in gpuparticles3d

@QbieShay Done 😎

@TCROC TCROC requested a review from QbieShay January 22, 2025 19:22
@Repiteo Repiteo merged commit b15b24b into godotengine:master Jan 24, 2025
19 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Jan 24, 2025

Thanks!

@akien-mga akien-mga changed the title Fix gpu_particles_2d not randomizing seed when set_one_shot is called Fix GPUParticles2D not randomizing seed when set_one_shot is called Jan 30, 2025
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