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

Add Sphere Mask and Particles Turbulence Nodes and Posterize ColorFunc #87173

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

paddy-exe
Copy link
Contributor

@paddy-exe paddy-exe commented Jan 14, 2024

Adds Posterize ColorFunc Mode to CanvasItem and Spatial Visual Shader Nodes
Adds Particle Turbulence to Particle Visual Shader Nodes

Closes godotengine/godot-proposals#5106
Part of godotengine/godot-proposals#7666

Showcase

Posterize node

grafik

ParticleTurbulence

Turbulence.Node.-.Godot.Engine.mp4

Production edit: closes godotengine/godot-roadmap#7

@paddy-exe paddy-exe added this to the 4.3 milestone Jan 14, 2024
@paddy-exe paddy-exe marked this pull request as ready for review January 14, 2024 13:20
@paddy-exe paddy-exe requested review from a team as code owners January 14, 2024 13:20
@paddy-exe paddy-exe force-pushed the more-visual-shader-nodes branch 3 times, most recently from b216e47 to 78af6d0 Compare January 14, 2024 14:52
@paddy-exe paddy-exe force-pushed the more-visual-shader-nodes branch 2 times, most recently from b2566b5 to 1cd7b60 Compare January 14, 2024 19:58
Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

Posterize needs its own AddOption (visual_shader_editor_plugin.cpp ~line 6210) so that you can add it directly like the other color functions.

I played a bit with every node and it looks like everything is working fine.

Ideally this PR would be split up since (I think) these nodes are unrelated to each other, but nonetheless great work!

scene/resources/visual_shader_nodes.h Outdated Show resolved Hide resolved
doc/classes/VisualShaderNodeParticleTurbulence.xml Outdated Show resolved Hide resolved
doc/classes/VisualShaderNodeParticleTurbulence.xml Outdated Show resolved Hide resolved
doc/classes/VisualShaderNodeSphereMask.xml Outdated Show resolved Hide resolved
@paddy-exe paddy-exe force-pushed the more-visual-shader-nodes branch 3 times, most recently from e1edb1b to b3986eb Compare March 30, 2024 13:10
@paddy-exe
Copy link
Contributor Author

@Geometror Thanks for the detailed feedback! I have amended the changes you suggested

Copy link
Contributor

@Ansraer Ansraer left a comment

Choose a reason for hiding this comment

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

The sphere mask node sounds like a useful node to have (I know that I created something similar it manually multiple times in the past) but I really think the name should be changed.
"Mask" kind of implies for me that this can only be used to mask an existing texture, which is only one usecase of it. In addition the node itself doesn't mask anything, needing an additional Multiplication OP node for that purpose.

IMO "SphereShape" would be a far better name that better communicates what this node actually does.

@QbieShay QbieShay self-assigned this Apr 2, 2024
Comment on lines +1356 to +1362
set_input_port_default_value(2, 1.0); // noise strength
set_input_port_default_value(3, 9.0); // noise scale
set_input_port_default_value(4, Vector3(0.0, 0.0, 0.0)); // noise speed
set_input_port_default_value(5, 0.2); // noise speed random
set_input_port_default_value(6, 0.1); // influence min
set_input_port_default_value(7, 0.1); // influence max
set_input_port_default_value(8, 1.0); // influence over life
Copy link
Member

Choose a reason for hiding this comment

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

I know this is the way we currently do it for almost all nodes, but we should really start to use enums to describe the ports/slots. (not a blocker, just for reference)

Copy link
Member

@Geometror Geometror left a comment

Choose a reason for hiding this comment

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

@Ansraer I have to disagree here, in my opinion "mask" is a better term to describe what it does (and is likely the most common usecase, not just to mask textures, but whole parts of shaders). However, I'm fine with "shape" too.
But I think it should be called CircleMask since it's 2D.

@Geometror
Copy link
Member

Geometror commented Apr 6, 2024

Besides that, I think it would be of value to implement it as a general Shape/Mask node (which allows selecting a shape using a dropdown menu). But in that case it should really be split from this PR, so that we could merge the other stuff sooner.
Example from material maker:
grafik
Well, I just noticed it's called "Shape" there, one point for Ansraer :)

@paddy-exe
Copy link
Contributor Author

Thanks for the feedback @Geometror @Ansraer I suppose it does make sense then to instead remove the shape node (as it would make sense to make it extensible with other shapes when introducing it) for now and go forward with the other changes

Adds Posterize ColorFunc Mode to CanvasItem and Spatial Visual Shader Nodes
Adds Particle Turbulence to Particle Visual Shader Nodes
@paddy-exe
Copy link
Contributor Author

Updated the PR and removed the SphereMask node for now. I have the changes locally still.

@paddy-exe paddy-exe requested a review from QbieShay April 14, 2024 18:51
@clayjohn clayjohn modified the milestones: 4.3, 4.4 Apr 30, 2024
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.

[TRACKER] Visual Shader nodes wishlist
6 participants