-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
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
[Superseded by #77154] Rework Particle Turbulence #64606
[Superseded by #77154] Rework Particle Turbulence #64606
Conversation
e33c4e1
to
5bebf7c
Compare
I'm in vacation for two weeks and won't have the time to properly review this PR. I saw that there are a lot of changes and if it's ok I would like to review it in practice before big changes like this are getting merged. There are a couple of things I found on the first look where I have some "feelings". But I like to properly review it and test it before saying anything. One thing that I can tell already is, that the changes to the emission shapes should go into a separate PR. And, please also keep in mind the necessary license changes in this PR. Where are the functions from? What is necessary for Godot and Godot users to respect the license? |
5bebf7c
to
6e9f30d
Compare
No worries. Thanks for taking the time to give the initial comment! I went ahead and removed the 2D-specific code, as well as the changes to emission shape. I'll set up a second emission shape PR at some point. All the code is either MIT or my own. To break it down:
The new vertex gradient system between the hash and the noise code itself is self-authored. The 4D noise in the repository seemed to produce some diagonal patterns that changed when I rendered slices of the noise with different coordinate permutations, meaning the gradient directions were not well-distributed. The replacement here first distributes the points on the surface of a tesseract, then uses a two-stages of polynomial approximation to get something like the result of the 4D version of this. The 'magic numbers' come from integral regression solving in Desmos. I also realized a chance to re-tune the Feel free to let me know of any more questions / concerns as they come up. |
In general the changes look good to me. I have two primary concerns:
|
I am very sorry about that close/reopen. It was not intentional! |
6e9f30d
to
bd750e6
Compare
All good! Figured it was that the moment I saw the cut-off comment. Re: noise code complexity, I see where you're coming from, and definitely agree there's a learning curve to understanding the internals of this type of noise. What I do see as helping its cause in that regard is that it's fairly self-contained. While it takes space as a block of code that needs study to completely understand, in general you don't need to fully grasp it to write good code around it. I wouldn't consider it worth settling for a quality downgrade for a more-immediately-intuitive function here -- or really in most cases when paired with decent encapsulation principles, but I would certainly agree the code's readability as it is can be improved. I've gone ahead and revised the noise code with clearer variable names and more verbose comments. |
3cc5915
to
d874d10
Compare
@clayjohn: Noise, in my experience, is the sort of thing you implement once and don't care afterwards. It's better that it's more performant even at the cost of readability. If you really want people to understand how noise is done, you could link in a comment to a more readable, unoptimized implementation "if you want to understand noise look it up at blah blah" |
By the way, can you provide real life comparisons of particle systems with a performance gain that can give a good reason to change the simple functions with the more complex ones? You pointed out that my original approach uses more calls, but how do the two approaches compare in terms of real world performance? It would be very interesting to see what differences we're talking about here. |
Unsure if you're asking me or clayjohn, as I haven't used the turbulence (yet) but I plan on using it to make my tire smoke less uniform and performance will definitely be a concern. From my experience testing/looking up shaders on Shadertoy, the number of noise lookups can make or break a shader. For instance, if you look up 'gas giant' or 'planet', many of those have very many lookups even to the point of freezing/crashing webgl. When implementing my own gas giant shader, I went for less 'realistic' but less lookups because I fully expect star systems with multiple giants and I don't want my performance to suffer (it's already suffering because in 3.5 there are no instance uniforms/parameters, so to have different individual planets I need to make them unique/local to scene so no instancing) |
I just want to avoid changing things when there is no problem ( https://docs.godotengine.org/en/stable/community/contributing/best_practices_for_engine_contributors.html ). Me and several other people tested the turbulence with millions of particles and never had any performance problems - so it would be great to see the real benefit before changing things "just because". |
Performance isn't a huge concern, I agree. I ran similar tests with the largest value suggested by the slider and found that rendering was far more of a bottleneck than the particle updates were. There was no perceptible difference in framerate in either system when zoomed out far enough. The main importance here would be the 4D noise and the evolution speed parameter. Scrolled 3D noise limits the space of parameters that produce patterns with more desirable properties and fewer undesirable ones -- namely a visible scrolling direction, which won't always be part of the artist's vision. The problem with the current solution is that it forces artists to consider a scrolling direction, when they shouldn't have to given the PCG tools we have to address it -- i.e. 4D noise. Scrolling same-dimensional noise can make sense where, for example, you're limited to textures and don't have the headroom to compute noise in real time. But here that isn't a constraint, and doing so only limits artists. Pattern evolution, if you accept these points, really should use 4D noise. From there we'd need to debate the specifics of the implementation. We could simply upgrade the existing noise function to 4D, add the evolution parameter, and make as few changes to the code otherwise as possible. That would work, but I wouldn't say the rest of the changes in this PR are without their own rhymes and reasons. The existing curl formula is more complex and less readable than it needs to be to achieve results of quality at least as good. From what I can gather, it effectively takes the gradients of three noise maps, rotates them each 90 degrees in different directions, projects them each to one of the three cardinal planes, and adds the results together. The rotations and cardinal plane projections seem arbitrary if there aren't any underlying principles I'm overlooking, and I'm not sure this merged solution is necessarily better for the purpose than the solution I propose. This cross-product formula here looks good, feels more intuitive, and has a solid basis in mathematics. I also find that it's more flexible for creating a variety of different patterns. It creates stronger flow-lines at high influence values, and patterns more similar to the current system at lower influence values. There is a subtle difference in character, I will grant, but my experience tells me that it's more technically sound and will offer artists more freedom. Re: the noise algorithm conversation, the existing function could be extended to 4D. But if we do proceed with 4D noise, I would hold a Simplex noise solution to be the way to go for the reasons covered thus far. It's easier to compute a derivative for, it's more directionally-even to the eye, and has widely-used MIT implementations available that can be molded into exactly what we need. The code itself is more complex, but as @Zireael07 notes we can link an external resource -- for example Simplex Noise Demystified. I agree that it's not sustainable to accept arbitrary changes. But the changes here do solve problems that users of this system will encounter through normal use -- especially pattern evolution. And it's better, the way I see it, to address these before release than after. |
Thanks for the great answer! I understand it much better now! I will test this PR extensively from the artist point of view when I return from my vacation! 👍 |
76f32fa
to
2589af5
Compare
Just rebased on upstream/master, and addressed a Secondly, I have created an alternative solution in this branch here that uses FastNoiseLite-style domain warp simplex noise to create the vector field instead. It creates a more ribbon-like appearance in the flows. If you choose to test both and prefer this other solution, just give me the word and I'll rebase it into the branch for this PR. |
2589af5
to
85e9d2a
Compare
Any 3D texture can be used, but dedicated tools for generating those are rare. You can import a 3D texture from any 2D image in Godot using layers (each row/column image is a different layer). |
a415e65
to
a05670c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright! So I gave the PR a lot of testing. Went in pretty deep with the two versions side by side.
I have to say that the PR is a definitive improvement, especially the coherence when the turbulence is animated!
@KdotJPG great work ♥
Sorry that it took that long 🙈 But I really wanted to make sure this is an improvement and that people / artists can create anything they can imagine. I think it's very powerful now.
By the way - in the current state of the PR I think it's safe to remove the breaks-compat
tag.
979415a
to
22715b4
Compare
@RPicster Sounds great! Thanks for your in-depth review! I've gone ahead and fixed the lingering spacing issues + done a final method naming tweak. Should we keep And regarding the removal of |
4.0 is at release candidate stage and fully feature frozen, so no, this wouldn't be merged right now. But we can merge soon after the release to get it well tested for 4.1. |
As long as no names are changed, this PR will not break compat, so I think we should keep names as is. |
Does someone have a testing project with particle turbulence setups, so we can compare performance between |
Testing project: test_particle_turbulence.zip I've tested this PR rebased against BenchmarkOS: Fedora 37 1152×648
3840×2160
Regardless of this PR, it's interesting to see the mspf difference between turbulence disabled and enabled becomes much greater at higher resolutions. I thought particle shaders' performance weren't influenced by viewport resolution, since they run once for every particle and not for each pixel. Besides, in the testing project, each particle is only a few pixels tall at 3840×2160. |
This is good to go for me , just needs a rebase on latest master |
Superseded by #77154. Thanks for your contribution! |
After encountering the latest Alpha snapshot and discovering @RPicster's great efforts on the Particle Turbulence PR, I found some areas for improvement and rework, to hopefully start this feature off on the best footing possible for the upcoming 4.0 release. (Updated description here following discussions further down)
The current implementation merged in #55387 uses directionally-scrolling 3D noise to create an evolving pattern in 3D. This is problematic because it forces the consideration of a scrolling direction, which creates unavoidable and often unfitting bias in resulting patterns. The included speed-random parameter is poised as a means to conceal these effects, but it's only a moderately-effective patch on top of a problem that doesn't need to exist in the first place. This PR replaces the scrolling 3D noise with 4D noise which varies in place over time.
Further, the included formula for curl noise is highly complex and is based on axis-biased arithmetic of uncertain mathematical origin.
This PR replaces it with a more concise solution that uses fewer noise calls while also producing more natural effects.These changes do break turbulence configs made using the existing dev snapshots![NEW] Full change set:
turbulence_noise_speed_random
so that it controls the noise evolution instead (pending feedback on desired implementation).[OLD] Full change set:
turbulence_noise_speed
(vec3) withturbulence_noise_evolution_speed
(float).2D particles use evolving 3D noise.turbulence_noise_speed
(vec3) if it is decided that the scrolling is useful -- perhaps asturbulence_noise_directional_trend
to be more explicit about its purpose and effect.cross(noiseGradientA, noiseGradientB)
which produces a proper divergence-free flow field with many fewer steps.-
Added a separate 2D case which uses an even simplerrotate90(noiseGradient)
formula (paraphrased code).turbulence_noise_scale
as it's passed to the shader, to better reflect the description in the documentation.shader_turbulence_noise_scale = 1.0 / turbulence_noise_scale;
turbulence_noise_speed_random
since it no longer has its necessary use case.RemovedDetermined that it does produce an effect and re-added corresponding functionality though aturbulence_noise_strength
since it had no effect on the particle update step, and its effect during the initialization step could be accomplished just the same by increasingturbulence_initial_displacement_min
andturbulence_initial_displacement_max
.turbulence_sharpness
parameter.alsopreviously used in the definition of `adj_contrast, which no longer plays a role now that the partial derivative vector is computed analytically.turbulence_influence
(from values sampled according toturbulence_influence_min
andturbulence_influence_max
), which biases the result towards more influence, to do so more cleanly. old, new https://www.desmos.com/calculator/hmsaly3f4bI also adjusted the way the normalization happens so that the new vector is always the same length as the old.Fixed the distance-from-center selection in the existingEMISSION_SHAPE_SPHERE
code -- it needs to be passed through a cube-root for a proper uniform distribution that doesn't cluster at the center.This technically does change existing patterns, but I wouldn't generally expect it to break any. If I should revert it or implement it differently, just give me the word.Motion to also remove Initial DisplacementInitial Displacement seems well-intentioned, but I haven't found any behavior in it that I couldn't replicate with the Emission Shape features. If there is consensus, I will update this PR with its removal. (@RPicster feel free to fill me in if I'm missing something.)All code added is
either MIT-licensed orself-authored.