Cell Fluid Dynamics trail effect #5996
Conversation
|
Looks good, but the effect is barely visible for large cells with nucleus. Is it possible to somehow scale the range of the ripples with the cell size? |
yes pal, you can play around with the parameters in the editor just look it up on the Membrane.tscn file ;) |
| shader_type spatial; | ||
| render_mode blend_mix, depth_prepass_alpha, cull_back, diffuse_lambert; | ||
|
|
||
| uniform vec4 water_color : source_color = vec4(0.0, 0.0, 0.0, 0.02); // Almost completely transparent background |
There was a problem hiding this comment.
Our shaders should also use our C# naming convention so this should be converted to use pascalCase naming convention rather than snake_case. Also the trailing comments should be changed to be comments on the line before what they are commenting on to match Thrive style.
| vec2 current_pos = vec2(0.0, 0.0); | ||
|
|
||
| // Simulate multiple ripple sources from object's recent past positions (longer trail) | ||
| for (int i = 0; i < 14; i++) { // VISCOSITY: More iterations = longer trail, less viscous appearance |
There was a problem hiding this comment.
I'm seeing quite a lot of loops and ifs so I would expect this shader to be pretty expensive, so we probably need to have a graphics option to turn this off.
| /*Controls how quickly ripples dissipate | ||
| higher values = less viscous appearance*/ |
There was a problem hiding this comment.
Should this be formatted as an XML summary comment?
| public float RadiusMultiplier = 2.5f; | ||
|
|
||
| [Export] | ||
| public int MaxMeshSubdivision = 100; // Reduced for better performane can be increased for a more detailed effect |
There was a problem hiding this comment.
Trailing comments shouldn't be used
| [Export] | ||
| public float VerticalOffset = -0.15f; // How far below the membrane the water plane should be | ||
|
|
||
| // Default radius if we can't determine it from the membrane |
There was a problem hiding this comment.
This definitely needs to be an XML summary comment.
| // Initialize the effect | ||
| SetProcess(true); | ||
| SetPhysicsProcess(true); |
There was a problem hiding this comment.
These should be automatic, so not required to be explicitly written like this.
| parentMembrane = GetParent<Membrane>(); | ||
| if (parentMembrane == null) | ||
| { | ||
| GD.PrintErr("MembraneWaterRipple must be a child of a Membrane"); | ||
| QueueFree(); | ||
| return; | ||
| } |
There was a problem hiding this comment.
This should be done the other way around, i.e. the membrane initializing the component inside it and not calling up to the parent. That's basically always a sign of bad Godot scene tree usage.
| // Initialize position tracking for movement calculation | ||
| lastPosition = parentMembrane.GlobalPosition; | ||
| previousPosition = lastPosition; | ||
| lastMovementTime = Time.GetTicksMsec() / 1000.0f; |
There was a problem hiding this comment.
Time shouldn't be done like this, only rely on the delta values given in _Process or _PhysicsProcess.
| UpdatePosition(); | ||
| } | ||
|
|
||
| // Called every frame for visual updates |
There was a problem hiding this comment.
This is like a default comment that should be deleted. Or if you really want to keep this, this must be reformatted as an XML comment.
| catch (Exception e) | ||
| { | ||
| GD.PrintErr("Error in water ripple process: " + e.Message); | ||
| } |
There was a problem hiding this comment.
There shouldn't be a catch like this in a component.
| // Update shader time with exact original timing | ||
| if (waterMaterial != null) | ||
| { | ||
| float currentTime = Time.GetTicksMsec() / 1000.0f; |
There was a problem hiding this comment.
Time passing should be taken from delta and not like this.
| } | ||
|
|
||
| // Called on physics process for movement and simulation updates | ||
| public override void _PhysicsProcess(double delta) |
There was a problem hiding this comment.
Is there an actual performance improvement from splitting the processing into two parts? I would guess that as you already check the time elapsed here, it would be more performance friendly to put all of the content from here into _Process and using a double variable to keep track when the extra checks need to run.
| // Dispose managed resources | ||
| if (parentMembrane is IDisposable disposableMembrane) | ||
| { | ||
| disposableMembrane.Dispose(); |
There was a problem hiding this comment.
This is absolutely incorrect. Godot nodes that are attached to the scene tree may never be disposed.
You need to suppress the compiler warning suggesting to do this like we do in basically every one of our Node-derived classes.
| waterPlane = new MeshInstance3D(); | ||
| AddChild(waterPlane); |
There was a problem hiding this comment.
This should be directly setup in the scene as this looks like static node data that doesn't need to be actually dynamically created.
| float currentRadius = GetMembraneRadius(); | ||
|
|
||
| // Only update if size changed significantly | ||
| // NOLINT |
There was a problem hiding this comment.
Why are there these NOLINT lines all over the place?
| PlaneMesh planeMesh = new PlaneMesh(); | ||
| planeMesh.Size = new Vector2(meshSize, meshSize); |
There was a problem hiding this comment.
I think this could also be directly setup in the tscn file but marked scene local so that modifying the plane mesh would be okay.
| catch (Exception e) | ||
| { | ||
| GD.PrintErr("Error creating water plane: " + e.Message); |
There was a problem hiding this comment.
This exception catching everywhere should just be deleted from this entire file. It's fine if you were developing with this on but from the finished version these need to be deleted. Godot 4 already has a top level catch and will prevent the game from crashing if a node _Process throws an exception.
| AddChild(waterPlane); | ||
|
|
||
| // Set the water plane's position to match the membrane's position | ||
| waterPlane.GlobalPosition = parentMembrane.GlobalPosition; |
There was a problem hiding this comment.
Isn't this position immediately overridden a few lines below? Meaning that this is a useless memory copy that goes all the way to Godot (C# -> C++ is an expensive call).
|
|
||
| // Get current position | ||
| Vector3 currentPos = parentMembrane.GlobalPosition; | ||
| float currentTime = Time.GetTicksMsec() / 1000.0f; |
There was a problem hiding this comment.
Incorrect time handling here as well.
| waterMaterial.SetShaderParameter("movement_direction", currentDirection); | ||
| waterMaterial.SetShaderParameter("movement_speed", currentSpeed); |
There was a problem hiding this comment.
Here should definitely need to use StringNames as these are called often.
| return DEFAULT_MEMBRANE_RADIUS; | ||
|
|
||
| // Tries to access radius directly if available | ||
| foreach (var propertyName in new[] { "Radius", "radius", "_radius" }) |
There was a problem hiding this comment.
??? This is the most questionable code.
Why is this using reflection? This is super bad for performance and very, very weird.
If this is about getting parent properties, it would be 100% better if the Membrane class handled giving its radius to this component that is a child of the membrane.
|
|
||
| return DEFAULT_MEMBRANE_RADIUS; | ||
| } | ||
| catch (Exception) |
There was a problem hiding this comment.
And this silently ignores errors in reading the data??
Pal you shouldn’t hold back you’re just giving me suggestions to improve the code and you as always are very polite so there’s nothing to worry about, originally the dispose part wasn’t in the original code I tried to experiment with it after I had to change the implementation the shader is really expensive as you said so the solution you proposed is probably the right way to do it |
|
Thanks for the encouragement. I do worry about coming across as too demanding in reviews, but that of course also depends on how the recipient feels about the review.
Yeah, I can see how that happened. Unless I'm misremembering the style guide should actually have the advice on how to deal with the Dispose stuff. |
|
One more thing I just realized, the new .uid files should be committed to git otherwise the game causes unknown uid warnings on everyone else's computer.
Should be committed to this branch. |
Brief Description of What This PR Does
Adds a transparent water ripple effect that creates dynamic fluid disturbances as cells move through the environment.
Related Issues
Closes #5702
Progress Checklist
Note: before starting this checklist the PR should be marked as non-draft.
break existing features:
https://wiki.revolutionarygamesstudio.com/wiki/Testing_Checklist
(this is important as to not waste the time of Thrive team
members reviewing this PR)
styleguide.
Before merging all CI jobs should finish on this PR without errors, if
there are automatically detected style issues they should be fixed by
the PR author. Merging must follow our
styleguide.