-
-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
Fully update first 3D Shader page #10554
Conversation
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.
I followed along with the tutorial. The images in the page now match the results of following along in 4.4beta1.
I approve the image changes. However, this page still needs a more significant refresh to fix a few things:
- Nitpicks around the way we stylize things like setting properties in the inspector
- Some confusion around the purpose and method of setting the normal values
- A few other inaccuracies and nitpicks I noticed
I would recommend merging this as-is, and I can try a followup PR to make the more extensive changes.
@@ -63,7 +63,7 @@ Setting up | |||
|
|||
Add a new :ref:`MeshInstance3D <class_MeshInstance3D>` node to your scene. | |||
|
|||
In the inspector tab beside "Mesh" click "[empty]" and select "New PlaneMesh". | |||
In the inspector tab beside "Mesh" click "<empty>" and select "New PlaneMesh". |
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.
I'm not a fan of how this page spells out how to create new resources by clicking on "<empty>"
each time. I feel like a shader tutorial should expect a little more familiarity with the inspector UI than that?
I'd prefer something more like the following:
In the inspector tab beside "Mesh" click "<empty>" and select "New PlaneMesh". | |
In the inspector tab, set the **Mesh** property to a new PlaneMesh. |
Subject to tweaks, and perhaps we do want to spell it out the first time it comes up.
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.
This is the only page I can think of that actually describes clicking on ""
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.
Search the page for "empty" and it comes up at least four times
Edit: sorry, misread, thought you were disputing the number of times it appears in the page
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.
It comes up a couple other times in other pages. I think it's fine and justified in the Getting Started tutorials, since it makes it extremely clear what to do. My guess is that it's a stylistic convention that was being followed at the time these pages were written.
I think we should move towards describing cases like this as "setting the Whatever property to a new Thing". If necessary we could also add in a general tutorial somewhere in the docs (either in Getting Started or maybe in the Inspector page) about which buttons to click to set properties, so this is explained in one place rather than piecemeal wherever it comes up. Something for another PR.
``NORMAL_MAP`` in the ``fragment()`` function. This way Godot will handle the | ||
wrapping of texture around the mesh automatically. |
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.
The section on setting the normals is a little broken, as indicated in #10501. I think that PR suggests the wrong fix, though.
I believe the intention with the tutorial is that the plane mesh normals are to be corrected by reading from a normal map. But since the normal map texture has high resolution, and the height is changed in vertex()
but the normals are changed in fragment()
, it not only corrects for the height, but also adds a bunch of extra normal detail. I think the right solution here might be to set the NORMAL value in vertex()
rather than setting a normal map here. I'm not 100% sure, though.
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.
Approving the changes here, I'll leave it to you two whether you're merging this as is and do a followup or do the further fixes in this PR. :)
Good work!
@skyace65 If you want to do any of the rewording away from Other than that I would recommend merging this one as-is and leaving the rest for a followup PR, since the changes are a combination of nitpicky style stuff, also may require a proper review from shaders/rendering if we end up changing the instructions around setting the normals. This is already a great improvement as is. |
ab9894d
to
33a91df
Compare
I've made AThousandShips suggested change. @tetrapod00 just merge this as is, I'd rather leave rewording the "click on empty" stuff for another PR. |
Thank you, merged! I'll get started on the more extensive changes when I get a chance. |
Looks like we went through the page at some point and updated the editor screenshots, but not the screenshots of the shader in the editor.
"[empty]"
in the text since it's now"<empty>"
vertex()
is now generated by defaultfragment()
function.