Style consistency pass on Spatial, Fog, CanvasItem, Sky, Particle shader pages by tetrapod00 · Pull Request #9746 · godotengine/godot-docs

tetrapod00 · 2024-08-14T04:53:53Z

Resolves #9745
Also looking at the text in #9691 as a guide for some of these changes.

Formalized style guidelines:

Use sentence case for built-in descriptions. Avoid random capitalizations and title case.
Shader built-ins should always be styled as code: VERTEX.
Built-in bool descriptions should start with "true if...", "true when...", etc.
Literal values should almost always be styled as code: 1.0, false, ENUM_VALUE. Ints are 1. Floats are styled as 1.0.
Processor functions should usually be styled as "the light() function" in the body and in built-in tables. Use of "the light function" or "a light function" are also okay, if it improves readability. Avoid "the light() function" (without code style).
Always capitalize "UV".
When referring to classes or properties, link to the class reference if possible.
Use "model space" instead of "local space" or "local coordinates". Exception: refer to "model/local space" at least a couple times to establish the connection. Exception: CanvasItem page consistently uses "local space" and I left it alone, since I'm not deeply familiar with 2D spaces.
Add a section about coordinate spaces to shader pages. #9757 may let us standardize completely on "model space" except for in the definition table for coordinate spaces.
In built-in descriptions, try to describe the space as "in world space". For example: NODE_POSITION_WORLD is "Node position, in world space", not "Node world space position".
Render modes should be stylized as "using the unshaded render mode". Avoid "render_mode" in the body of the page.

Other, opinionated, changes made by this PR:

In description of VERTEX, call it "vertex position" where possible. ctrl+f "position" should surface VERTEX.
Similarly, get the word "color" into ALBEDO's description somehow.
Link to Standard Material 3D from Spatial shaders. It's the best documentation for alpha modes among other things.
A few small style, clarity, or grammar changes

clayjohn · 2024-08-14T05:52:47Z

Looks great so far! I agree with your style guideline

tetrapod00 · 2024-08-14T07:07:34Z

Does Godot have a standard term for local/model space? Within the Spatial Vertex Built-ins table, I see both "local coordinates" and "model space".

I suppose if both are commonly used, it's better to keep references to both in the docs?

clayjohn · 2024-08-14T18:03:20Z

In the descriptions of MODEL_MATRIX and MODELVIEW_MATRIX, refer to "Model/local space". It's fine if "model space" and "local coordinates" are used interchangeably elsewhere if they are established as the same at least once or twice.

I would standardize on "model space" because it matches the naming of MODEL_MATRIX and MODELVIEW_MATRIX. Using the term local space was likely done without much thought

tetrapod00 · 2024-08-14T18:15:52Z

"Local space" or "local coordinates" does match the naming convention for local coordinates in CPU-side transforms. If we standardize on "model space", there should still be a few mentions of "model / local space" to make the connection.

At some point all the different spaces should be documented in one place, so that single explanation can be linked to.

Another complication: the CanvasItem page uses "local space" as the main description. Should a different standard be applied to CanvasItem shaders since they are more of an on-ramp for new shader users?

In any case, I'll put the maximal standardization of "model space" into the draft and we can see how it looks.

tetrapod00 · 2024-08-18T03:52:56Z

@@ -260,14 +263,14 @@ Below is an example of a light shader that takes a CanvasItem's normal map into
 +----------------------------------+------------------------------------------------------------------------------+
 | in float **LIGHT_ENERGY**        | Energy multiplier of Light.                                                  |


All the references to "Light" I left alone. I am not familiar with 2D shaders - do these refer to the LIGHT built-in, or a 2D light node?

Refers to a 2D light node, (technically a "light instance" I think)

Changed these into links to Light2D and it's properties.

tetrapod00 · 2024-08-18T03:54:49Z

 +=============================================+===============================================================+
 | in vec4 **FRAGCOORD**                       | Coordinate of pixel center. In screen space. ``xy`` specifies |
-|                                             | position in window. Origin is upper-left.                     |
+|                                             | position in window. Upper-left of the screen is ``(0.0,0.0)``.|


Unsure about this change. The CanvasItem page seems more beginner friendly than the Spatial page. "Origin" is a term that may be unfamiliar.

tetrapod00 · 2024-08-18T04:46:27Z

This one's ready for review.

I can also clean up the guidelines themselves if we think we'll be reusing them. I think it's a fine outcome if these pages get cleaned up once and we don't formalize the style guidelines anywhere.

huwpascoe · 2024-09-10T22:12:22Z

 +=============================================+===============================================================+
 | in vec4 **FRAGCOORD**                       | Coordinate of pixel center. In screen space. ``xy`` specifies |
-|                                             | position in window. Origin is upper-left.                     |
+|                                             | position in window. Upper-left of the screen is the origin,   |


position in window or in viewport?

I'm not that familiar with 2d coordinate spaces and shaders, I'll investigate this and clarify

It seems to be based on the viewport. Here's a quick fragcoord visualization shader, applied on both a full rect control and a subviewport. The subviewport's FRAGCOORD does not change as its position in the window changes.

Godot_v4.3-stable_win64_qmFkBMpqnF.mp4

I figured the original incorrectly stated window instead of viewport.

huwpascoe · 2024-09-10T22:14:52Z

 +==================================+==============================================================================+
 | in vec4 **FRAGCOORD**            | Coordinate of pixel center. In screen space. ``xy`` specifies                |
-|                                  | position in window. Origin is lower-left.                                    |
+|                                  | position in window. Lower-left of the screen is the origin, ``(0.0,0.0)``.   |


again, is that window or viewport?

The origin for FRAGCOORD in light() might be the upper left, too? This shader which outputs the light() FRAGCOORD seems to increase down and to the left, just like the FRAGCOORD in fragment().

The setup for testing a subviewport is more complicated for Node2D, still working on that.

Conclusion: the light() FRAGCOORD is also the coordinate in the viewport, not the window. And it's origin is the upper-left, like the fragment() FRAGCOORD.

Godot_v4.3-stable_win64_ZS2gYGzZ4w.mp4

Updated the description for both instances of FRAGCOORD to match observed behavior. This still might be technically a little incorrect, though?

huwpascoe · 2024-09-10T22:44:12Z

@@ -260,14 +263,14 @@ Below is an example of a light shader that takes a CanvasItem's normal map into
 +----------------------------------+------------------------------------------------------------------------------+
 | in float **LIGHT_ENERGY**        | Energy multiplier of Light.                                                  |


Refers to a 2D light node, (technically a "light instance" I think)

clayjohn

Seems great to me. There are a few outstanding comments from huwpascoe's review. Should be good to merge once those are resolved.

tetrapod00 · 2024-09-14T22:37:19Z

I made another round of changes for all of the outstanding comments. Would appreciate another correctness check on the changes!

tetrapod00 · 2024-09-20T19:26:56Z

Besides the resolved comments, I also changed how ranges were described in most cases in the spatial shader built-in tables.
Before, a range between 0 and 1 was described as "(0..1)". Now, it's "range of [0.0, 1.0]".

Use code styling for the literals.
Use square brackets to describe range, to avoid confusion with vector literals.
Explicitly mention "range of". It's wordy, but increases clarity.

tetrapod00 · 2024-09-20T19:38:29Z

+-----------------------------------+------------------------------------------------------------------------+
+| in vec3 **LIGHT**                 | Light vector, in view space.                                           |
+-----------------------------------+------------------------------------------------------------------------+
+| in vec3 **LIGHT_COLOR**           | :ref:`Light color<class_Light3D_property_light_color>` multiplied by   |


These two were awkward to write because you can't have links in a code block. I think I found a decent compromise.

skyace65 · 2024-09-26T02:32:38Z

Thanks!

tetrapod00 force-pushed the spatial-style-pass branch 2 times, most recently from a001c9d to 43f478b Compare August 14, 2024 05:45

tetrapod00 force-pushed the spatial-style-pass branch from 43f478b to ca6fb9d Compare August 14, 2024 06:36

tetrapod00 force-pushed the spatial-style-pass branch 4 times, most recently from a5727a9 to 3359dd0 Compare August 14, 2024 09:03

AThousandShips added enhancement area:manual Issues and PRs related to the Manual/Tutorials section of the documentation topic:shaders labels Aug 14, 2024

tetrapod00 commented Aug 14, 2024

View reviewed changes

Comment thread tutorials/shaders/shader_reference/spatial_shader.rst

tetrapod00 force-pushed the spatial-style-pass branch 4 times, most recently from 67dbbd7 to 46c4464 Compare August 18, 2024 03:44

tetrapod00 commented Aug 18, 2024

View reviewed changes

tetrapod00 force-pushed the spatial-style-pass branch 2 times, most recently from 17edb70 to 0723c00 Compare August 18, 2024 03:58

tetrapod00 commented Aug 18, 2024

View reviewed changes

Comment thread tutorials/shaders/shader_reference/spatial_shader.rst

tetrapod00 commented Aug 18, 2024

View reviewed changes

Comment thread tutorials/shaders/shader_reference/spatial_shader.rst

tetrapod00 marked this pull request as ready for review August 18, 2024 04:01

huwpascoe reviewed Sep 10, 2024

View reviewed changes

clayjohn approved these changes Sep 11, 2024

View reviewed changes

tetrapod00 force-pushed the spatial-style-pass branch from 0723c00 to c980f10 Compare September 14, 2024 22:21

tetrapod00 force-pushed the spatial-style-pass branch 2 times, most recently from bb2704e to a363655 Compare September 14, 2024 22:31

AThousandShips reviewed Sep 15, 2024

View reviewed changes

AThousandShips reviewed Sep 18, 2024

View reviewed changes

tetrapod00 force-pushed the spatial-style-pass branch from a363655 to 1cdc9de Compare September 20, 2024 19:23

This comment was marked as resolved.

Sign in to view

Standardize style within spatial_shader.rst, etc

4e02ee3

tetrapod00 force-pushed the spatial-style-pass branch from 1cdc9de to 4e02ee3 Compare September 20, 2024 19:36

tetrapod00 commented Sep 20, 2024

View reviewed changes

skyace65 merged commit 41bb40b into godotengine:master Sep 26, 2024

		@@ -260,14 +263,14 @@ Below is an example of a light shader that takes a CanvasItem's normal map into
		+----------------------------------+------------------------------------------------------------------------------+
		\| in float LIGHT_ENERGY \| Energy multiplier of Light. \|

Uh oh!

Uh oh!

Conversation

tetrapod00 commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

clayjohn commented Aug 14, 2024

Uh oh!

tetrapod00 commented Aug 14, 2024

Uh oh!

clayjohn commented Aug 14, 2024

Uh oh!

tetrapod00 commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tetrapod00 commented Aug 18, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clayjohn left a comment

Choose a reason for hiding this comment

Uh oh!

tetrapod00 commented Sep 14, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tetrapod00 commented Sep 20, 2024

Uh oh!

This comment was marked as resolved.

Choose a reason for hiding this comment

Uh oh!

skyace65 commented Sep 26, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

tetrapod00 commented Aug 14, 2024 •

edited

Loading

tetrapod00 commented Aug 14, 2024 •

edited

Loading