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 often used built-ins (camera-pos, object-pos, camera-eye etc.) to spatial shaders #63597

Merged
merged 1 commit into from
Aug 2, 2022

Conversation

paddy-exe
Copy link
Contributor

@paddy-exe paddy-exe commented Jul 28, 2022

Implements this proposal: godotengine/godot-proposals#4957

(Updated) Syntax of implementation for written shaders

vec3 node_pos_world = NODE_POSITION_WORLD;
vec3 camera_pos_world = CAMERA_POSITION_WORLD;
vec3 camera_dir_world = CAMERA_DIRECTION_WORLD;
vec3 node_pos_view = NODE_POSITION_VIEW;

(Updated) Showcase in visual shaders:

image

(Old syntax) Showcase in written shaders:

Node3D.tscn.-.Shader-Spatial-Built-Ins-Test.-.Godot.Engine.2022-07-29.13-54-16.mp4

@Calinou Calinou added this to the 4.0 milestone Jul 28, 2022
@paddy-exe paddy-exe force-pushed the spatial-shader-built-ins branch 2 times, most recently from 50ea0cf to 9a02a7f Compare July 29, 2022 16:58
@paddy-exe paddy-exe marked this pull request as ready for review July 29, 2022 17:10
@paddy-exe paddy-exe requested review from a team as code owners July 29, 2022 17:10
@QbieShay
Copy link
Contributor

Hey @paddy-exe , thank you so much for this!

@paddy-exe
Copy link
Contributor Author

Hey @paddy-exe , thank you so much for this!

Sure! If you have any suggestion for other built-ins I can implement, let me know

@QbieShay
Copy link
Contributor

Because the fragment is in view space, I think it could be useful to have those variables as well in view space

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Jul 29, 2022

Hmm yeah I agree but we have to be careful that we don't overload the input node with too many built-ins since the list is already quite long:
image

I will probably first wait for feedback from @clayjohn and @Chaosus since this is their field

@Chaosus
Copy link
Member

Chaosus commented Jul 30, 2022

Seems useful, especially for visual shaders.

@paddy-exe
Copy link
Contributor Author

I updated the syntax naming to be in line with each other. I updated the syntax in the first comment as well
WORLD_CAMERA_EYE is now CAMERA_WORLD_EYE

@paddy-exe
Copy link
Contributor Author

@QbieShay I added the ObjectViewPosition as a new input:

Node3D.tscn.-.Shader-Spatial-Built-Ins-Test.-.Godot.Engine.2022-07-30.17-06-42.mp4

@paddy-exe
Copy link
Contributor Author

image

Autocompletion in the visual shader add-node dialog works now too

@paddy-exe paddy-exe force-pushed the spatial-shader-built-ins branch 2 times, most recently from 410e98f to 0c6e07f Compare July 30, 2022 15:16
@William-Godwin
Copy link

Is it possible to add the same variables to the particle shader? This would be very handy for some effects. I think...

@paddy-exe
Copy link
Contributor Author

After chatting on discord with @William-Godwin we decided it would be best to create a new proposal for his needs in particle shaders since they would require more in-depth changes and would complicate the status of this PR

@QbieShay
Copy link
Contributor

Proposition to rename "ObjectViewPosition" and "ObjectWorldPosition" to "NodePositionView" and "NodePositionWorld"

Of note, all other engines work in either world or object space, we are the only one working in view space.

@clayjohn @Calinou @reduz opinions on the names?

@clayjohn
Copy link
Member

clayjohn commented Aug 1, 2022

Proposition to rename "ObjectViewPosition" and "ObjectWorldPosition" to "NodePositionView" and "NodePositionWorld"

Of note, all other engines work in either world or object space, we are the only one working in view space.

@clayjohn @Calinou @reduz opinions on the names?

I have no strong opinion on the naming. Object to me sounds more natural, but Node fits better with the naming we use outside of shaders so it is likely more approachable for users. I would go with Qbie's suggested names for now.

I am a little confused about CAMERA_WORLD_EYE The eye vector usually refers to the direction between a fragment/vertex and the camera. I expected this to return the direction between the object center and the Camera in world space, but of course that is not what this does. Can you clarify what it is CAMERA_WORLD_EYE is supposed to represent so we can figure out a more intuitive name?

@paddy-exe
Copy link
Contributor Author

paddy-exe commented Aug 1, 2022

Okay so I updated the syntax according to @QbieShay 's feedback:

vec3 object_world_pos = NODE_POSITION_WORLD; // returns the object position in world space
vec3 camera_pos = CAMERA_POSITION_WORLD; // returns the camera position in world space
vec3 camera_world_eye = CAMERA_EYE_WORLD; // returns the camera eye vector in world space direction of the camera
vec3 object_view_pos = NODE_POSITION_VIEW; // returns the object position in view space

@clayjohn I actually just wanted to implement the same that I documented here for this godot-docs PR.
I would describe it as the camera view direction vector in world space
godotengine/godot-docs@e5f23ee#diff-3df75bc2fdd44e719dd72a78099844ccb88e1190d2fc3d432b75030590b0ae98R239

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Approved in PR review meeting.

Last change requested would be to rename CAMERA_EYE_WORLD to CAMERA_DIRECTION_WORLD (proposed by @JFonS and approved by @QbieShay and @reduz).

@paddy-exe
Copy link
Contributor Author

@akien-mga Thanks for the quick feedback! Rebased everything and updated to your feedback.
Also updated the PR document (in the beginning) for anyone reading this later on.

@akien-mga akien-mga merged commit 5155528 into godotengine:master Aug 2, 2022
@akien-mga
Copy link
Member

akien-mga commented Aug 2, 2022

Thanks! And congrats for your first merged Godot contribution 🎉

@paddy-exe paddy-exe deleted the spatial-shader-built-ins branch August 2, 2022 17:32
paddy-exe added a commit to paddy-exe/godot that referenced this pull request Aug 5, 2022
Backport of this PR: godotengine#63597
This adds these as new Built-Ins to Spatial Shaders
* Object's Position in World Space
* Camera Position in World Space
* Camera Direction in World Space
* Object's Position in View Space
Riordan-DC pushed a commit to Riordan-DC/godot that referenced this pull request Jan 23, 2023
Backport of this PR: godotengine#63597
This adds these as new Built-Ins to Spatial Shaders
* Object's Position in World Space
* Camera Position in World Space
* Camera Direction in World Space
* Object's Position in View Space
Riordan-DC pushed a commit to Riordan-DC/godot that referenced this pull request Jan 24, 2023
Backport of this PR: godotengine#63597
This adds these as new Built-Ins to Spatial Shaders
* Object's Position in World Space
* Camera Position in World Space
* Camera Direction in World Space
* Object's Position in View Space
@celyk
Copy link

celyk commented Mar 17, 2023

Shouldn't that
actions.renames["CAMERA_DIRECTION_WORLD"] = "scene_data.view_matrix[3].xyz";
be using -scene_data.inv_view_matrix[2].xyz, as in the camera forward vector?
So actions.renames["CAMERA_DIRECTION_WORLD"] = "-scene_data.inv_view_matrix[2].xyz";

Also, I think this
actions.renames["NODE_POSITION_VIEW"] = "(model_matrix * scene_data.view_matrix)[3].xyz";
Should be this
actions.renames["NODE_POSITION_VIEW"] = "(scene_data.view_matrix * model_matrix[3]).xyz";

rsubtil pushed a commit to rsubtil/godot-docs that referenced this pull request Apr 11, 2023
Following up on this PR: godotengine/godot#63597
Documents the new shader built-ins introduced by this PR.
```
NODE_POSITION_WORLD = Node world space position
NODE_POSITION_VIEW = Node view position
CAMERA_POSITION_WORLD = Camera world space position
CAMERA_DIRECTION_WORLD = Camera world space direction
```
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.

None yet

8 participants