-
-
Notifications
You must be signed in to change notification settings - Fork 23.1k
Add missing features to VisualShaderNodeParticleOutput #73037
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
base: master
Are you sure you want to change the base?
Conversation
Hey @hmans ! It's awesome you put your changes up, thank you for taking the time! The only thing i'm not a fan of, is to write particle lifetime in CUSTOM.x. The default particle shader writes lifetime in |
That is a good point. I've updated the code to follow |
@fracteed There is nothing to approve, CI has been run already (perhaps someone pressed the button earlier). Artifacts should be available. |
@hmans thanks for working on this, really promising already!
I have attached a project file with 3 demo particle systems, to test out the new functionality. The first demo is quite simple, but shows the particle progress driving color, alpha, velocity, and scale. The curves and the 2d texture ramp all work as expected. Good to see that color over life works, though not sure if I set it up correctly but using life to drive uv's does seem to work. rising_bubbles0000-0399.mp4Second demo adds in rotation, with random rotation speed. When they hit a certain age, the color changes and particles rise by adding to position, and they spin faster. cube_evolution0000-0399.mp4Third demo is more complex and doesn't use any velocity with the position set directly in process. Combining age driven and distance based effects to get a motion graphics effect. grid_ripples0000-0299.mp4The first demo node graph: Some general notes:
I will be interested to see how this patch progresses and will keep playing with it! |
368b90e
to
de4f549
Compare
Sorry, I accidentally submitted this PR for review. This was a misclick. I've returned it to Draft status. |
rather than align y to velocity, id prefer having a look_at node of some sort that can be used for that |
@QbieShay Oh yeah, definitely need something more flexible than just align y to velocity. I would love to see an "align rotation to vector" node like Blender geometry nodes has. That way you could align rotation to velocity or have it look at a position, etc on any axis. https://docs.blender.org/manual/en/2.93/modeling/geometry_nodes/point/align_rotation_to_vector.html |
I second what @fracteed said, i think we should follow VisualShader's conventions for inputs and have them in a dropdown Also, please don't add any more features to this PR. Smaller PRs are way easier to review (and have a way higher change to be merged) than larger PRs. |
I'd be happy to move this forward into something that's actually mergeable, and I'd love to limit the scope to a minimum that makes sense to merge while still being useful. To me, that would currently be:
What do you think? The one part of all this that I'm not happy with is rotation, because there's currently no clean way to mutate rotation across frames (but maybe there is a way to extract axis and angle from |
Sorry, I didn't fully understand. This PR does the following things:
is that what you want to move forward with? |
I was asking you, because you mentioned keeping the scope small :) If the above makes sense w.r.t. scope, the only thing that's missing is the new node. The experimental nodes I've built for this only live in my own project, and they're implemented in GDScript. I'm assuming this node will need to be implemented in C++ if it is to be part of this PR. Scary, but I think I can do it. Also, as mentioned before, it'd be great to find a way to improve the handling of rotation, but this can probably go into a separate PR. |
Oh. I completely misunderstood what this PR does. I thought it contained the lifetime input nodes as well? |
de4f549
to
5e0f99f
Compare
I would appreciate some design input please. Should the new particle inputs (age, progress, position, etc.) live in a new node (as discussed in this PR previously), or should they instead be housed within I'm asking because while looking through the codebase, I found this: // PARTICLES-FOR-ALL
add_options.push_back(AddOption("Active", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "active", "ACTIVE"), { "active" }, VisualShaderNode::PORT_TYPE_BOOLEAN, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("AttractorForce", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "attractor_force", "ATTRACTOR_FORCE"), { "attractor_force" }, VisualShaderNode::PORT_TYPE_VECTOR_3D, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("Color", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "color", "COLOR"), { "color" }, VisualShaderNode::PORT_TYPE_VECTOR_4D, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("Custom", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "custom", "CUSTOM"), { "custom" }, VisualShaderNode::PORT_TYPE_VECTOR_4D, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("Delta", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "delta", "DELTA"), { "delta" }, VisualShaderNode::PORT_TYPE_SCALAR, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("EmissionTransform", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "emission_transform", "EMISSION_TRANSFORM"), { "emission_transform" }, VisualShaderNode::PORT_TYPE_TRANSFORM, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("Index", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "index", "INDEX"), { "index" }, VisualShaderNode::PORT_TYPE_SCALAR_UINT, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("LifeTime", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "lifetime", "LIFETIME"), { "lifetime" }, VisualShaderNode::PORT_TYPE_SCALAR, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("Number", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "number", "NUMBER"), { "number" }, VisualShaderNode::PORT_TYPE_SCALAR_UINT, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("RandomSeed", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "random_seed", "RANDOM_SEED"), { "random_seed" }, VisualShaderNode::PORT_TYPE_SCALAR_UINT, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("Restart", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "restart", "RESTART"), { "restart" }, VisualShaderNode::PORT_TYPE_BOOLEAN, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("Time", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "time", "TIME"), { "time" }, VisualShaderNode::PORT_TYPE_SCALAR, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("Transform", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "transform", "TRANSFORM"), { "transform" }, VisualShaderNode::PORT_TYPE_TRANSFORM, -1, Shader::MODE_PARTICLES));
add_options.push_back(AddOption("Velocity", "Input/All", "VisualShaderNodeInput", vformat(input_param_shader_modes, "velocity", "VELOCITY"), { "velocity" }, VisualShaderNode::PORT_TYPE_VECTOR_3D, -1, Shader::MODE_PARTICLES)); ...which gives me the feeling that it should be the latter. I'd be happy to just add these to the existing input node. Is this the way forward? |
Thanks for the feedback, @fracteed! I'm happy to see that these changes are useful — it's motivating me to flesh these out now.
I may be misunderstanding you, but is this not the intended behavior? I've understood "world mode" to simulate particles in world space, meaning that moving the emitter will merely move the location of newly spawned particles, without affecting previously spawned ones.
I'll figure something out for the Start node.
We should be able to do this with the (upgraded) nodes already. The
Please see my comment above — I'm unsure if the new inputs should also go into the |
I've pushed a few changes and would appreciate feedback. If anyone wants to give this build a try, I've attached a simple project that uses this PR's features: EXP-Godot-Visual-Shader-Nodes.zip Here's a screenshot of the example Visual Shader from the project: Stuff that is going on here:
The new node outputs are, as of now, all housed within the Stuff that's missing and/or needs to be decided:
|
What is "lifetime factor"? Is it what is normally assigned in CUSTOM.w? |
@hmans nice work, it is all looking very promising!
Could one of the maintainers please approve the workflow, so I can try an artifact auto build :) |
I think it's possible to make a lower bound random, but definitely not an upper bound. So yuo'd have a lifetime of (say, 4) in the properties, and a random min lifetime in the particle system shader. It's not ideal but I understand why the randomization is done like that and frankly i can't imagine better solutions that don't involve significant refactors. |
In addition to @QbieShay's comment above, I think you can still exact quite a lot of control over these values by setting the particle system's global lifetime to something high (essentially the upper bound of lifetimes you want to support; this can be a very high value if you want "potentially unlimited"), and figuring out the lifetime ratio by some multiplication math: if The value you're setting here is per particle (!).
Ah, I read your original comment exactly the other way. :-) I'll check this, thanks!
It probably won't, because there won't be any rotation as long as the angle input remains at |
I just checked this and I think the behavior of the already spawned particles works as intended; if the particle system is set to "Local Coords", particles will move together with the particle system's scene node; if it's not set to "Local Coords", particles will be simulated in world space. However, I noticed that the existing emitter nodes (like I think semantically this is sound, but setting this up of course isn't ideal because a) you can't quickly grab the emission position from the inputs, just the complete transform, and b) you can only extract its position component to combine with To me, for now, this is a "let's improve this in another PR" type deal. Update: I've found the |
I think we can address that in a later PR by adding configuration to these nodes? |
@QbieShay thanks, that makes sense as this all has to fit within the confines of the existing particle system. As @hmans explains, you can get any value you want based on the ratio maximum life, so it is quite workable the way it is. @hmans thanks for all the explanations, they certainly helped! I was able to use the transform mult to get moving emitters, no problem. I created some more tests and didn't really have any issues, so it all seems quite solid. The rotation issues may be better tackled in another PR, as that is a tricky one.
Here are 3 more tests...
rising_cubes20000-0299.mp4
attraction0000-0299.mp4
bounce0000-0299.mp4project file: |
Thanks for pointing this out! This seems to be a regression introduced with this PR; I've created a simple 2D scene with a Update: fixed! |
196366e
to
ff3e9b1
Compare
I think this PR is getting ready to submit for review soon — I've implemented the Collide graph (and used the opportunity to fix it so that within a frame where a particle is colliding, the normal Process graph is still run in addition to the Collide graph — this matches the behavior of ParticleProcessMaterial.) There are some parts of If you have an opinion on whether I should do this in this PR, in a separate PR, or maybe not at all, please let me know. Other than that, it'd be great if we could put the changes in this PR through their paces a little. They've worked fine in what testing I've performed for them, but who knows. |
Should we look into finishing this for 4.2? If so, it would be great to resolve all concerns and do a rebase in the next few weeks. |
Yep - I have contacted OP just today and notified them that someone else will pick up this PR. Can be me, or someone else that's motivated. |
Prelude
This is a Draft PR that I'm putting up for discussion, both about finding out if and how this may be able to get merged, but also about what other tweaks and improvements I could make while I'm at it.
If this should be a full-on proposal, please let me know and I will post one.
Summary
Visual Shaders are great, but the current implementation for Particle Shaders as found in
master
, the recent 4.0 betas and RCs, is slightly lacking:TRANSFORM
#72798scale
input in the Process graph only supporting uniform scale (float), not a vectorCUSTOM
, from scratch for every effect that needs them; also, it would be good to achieve parity with howParticleProcessMaterial
utilizesCUSTOM
for lifetime bookkeeping)TRANSFORM
"manually".This PR aims at improving these with as little changes as possible:
scale
input from a float to a vec3position
input that allows the user to overwrite the particle's positionParticlesProcessMaterial
and writes per-particle time information intoCUSTOM
:CUSTOM.y
being the progress over the default lifetime configured for the particle effectCUSTOM.w
being the randomness ratio (currently hardcoded to 1.0, but we can now expose per-particle lifetime as an input on the Start graph)USERDATA1
so they can be mutated in the Process/Collide graphsInput
node in particle mode:age
: the particle's age in secondsprogress
: the progress (0.0-1.0) across the particle's lifetimeposition
: the particle's current position (vec3)scale
: the particle's current scale (vec3)rotation_axis
: the particle's rotation axis (vec3)rotation_angle
: the particle's rotation angle around the above axis (float)Example Visual Shader
These changes make visual particle shaders like this possible — please see this comment for details and an example project: