-
-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
Improve shader function descriptions #9338
base: master
Are you sure you want to change the base?
Conversation
Pull request for issue #9310 |
CC @clayjohn |
That was unbelievably fast. I'm going to need a few days to review this. But I am very excited! |
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 looks good, gonna go through it for style again but this was some that stuck out for now
Co-authored-by: Yuri Rubinsky <[email protected]> Co-authored-by: A Thousand Ships <[email protected]>
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.
Amazing work! This looks really good so far.
I think I just have two general comments:
- I agree with Chaosus' comment earlier that, in most cases, functions with the same name should be grouped instead of duplicated. Especially for functions with really long descriptions.
- I like how you kept the short description in the table. But I think it needs to be truly a short description. Some of the entries have the entire description copied in the table and that ends up making the table bloated in hard to navigate.
In these cases, the table actually has more information than the long description below.
Awesome! I'm glad you like it. I'm pushing hard to finish a project milestone this week, but I should be able to work on an update next week. |
Co-authored-by: tetrapod <[email protected]>
Co-authored-by: tetrapod <[email protected]>
@tetrapod00 Any thoughts on the conversation @clayjohn and I were having about functions working on arrays piecewise? |
Latest commit addresses concerns about component-wise functions and operators. It also improves some formatting and fixes some oddities with the navigation tree. Here's a bulleted list of what I changed:
|
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.
Otherwise I think this looks good
I see a lot of places where style can be improved in a minor way, like styling literals as code and with a decimal marker for floats (e.g. |
I started on a nitpicky style review but I think it would be better as a followup PR - making all the required changes is annoying as suggestions. IMO the style inconsistency is not bad enough to prevent merging as is. |
Should have stated functions are based on GLSL ES 3.0 (GLSL != GLSL ES)
Ok, fixed the couple things you already mentioned since I needed to fix my GLSL 4.0 mistake. I'm more than happy to let you fix the rest of the tedious formatting issues in another PR. What does the merge process look like from here? We wait for ClayJohn or some such to come give a final approval and pull it in? |
Yeah, now you're waiting on another review from clayjohn or someone else with shader-specific knowledge to do a high-level review including whatever other style nitpicks come up in that review, then it should be merge-able. IMO this PR is already so much better than the current state of the function docs that this PR is merge-able as-is. (Also I believe in most cases we would address the style consistency in this PR, not a followup, but it is really tedious work on a large PR, it's not blocking, and I'm volunteering to do it) |
I keep meaning to do a deep review of this. But this is a huge chunk of work and I never have the time to do it in one sitting. At this point I trust that the current state is way better than it was before. I also trust that the reviews from tetrapod and ATS would catch any glaring issues. Further Ashby has shown that they are working hard to make this as good as possible and I appreciate and respect his efforts. I suggest we go ahead with merging this without me doing a super detailed review as I don't want to be the blocker for this any longer and I trust all the others involved in the process. Great work everyone! |
Standardized style. Fix a few factual and copy paste errors. Fix some formatting. Consistently use code blocks for literals. Use sentence case for all parameters. Avoid math symbols, prefer code-style >=.
I did an in-depth review. Overall this looks great. I found a few factual and copy-paste errors, and made a lot of minor style changes. The changes are in a PR to the branch at AshbyGeek#1, which seemed to be the best way to make changes this big. Summary of changes:
|
Shader functions style improvements
Wowee, you weren't kidding about a lot of fiddly changes. You have a much better eye for detail and consistency in technical documentation than I do. My hat is off to you sir! I have already merged that pull request. My only real question from looking at your pull request is really just a code style question: You changed a bunch of instances of
to the much more verbose
As far as I can tell the rendered HTML is the same. Am I mistaken? Note, as I said above I've already merged your pull request. If you want to change these back, you can send me another pull request I guess, lol. |
The rendered HTML is not the same (tested on Windows 10, Firefox, 100% browser zoom). Before the changes, the function names are gray, but after the changes the function names are white, like the functions which appear on a single line with no overloads: After the changes, in 8972c45: Compare to any function without overloads: The version with white function names is correct. (In 220fb2b, there were also a few unrelated formatting errors like the following, but those are not related to this difference.) |
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.
With the style changes applied, this is good and mergable.
There are still many minor improvements that could be made, and I fully expect to find a factual error or two even after this is merged. But this is already much better than the current state of the shader function documentation. It's a good base for future improvements, which can be done in smaller chunks.
Why does yours render dark? Godot docs online use a light theme, which is also what I see on my computer. Here's the code from commit 220fb2b:
when compiled with Sphinx produces the following HTML on my computer:
which looks like this in Chrome v129.0.6668.101 (on Windows 10) Here's code from commit 897nc452 (the merge commit)
which produces the following html
and looks like this in Chrome v129.0.6668.101 (on Windows 10) I would guess from the inclusion of the "|" in your last image and the fact that you have a dark theme, that there's some configuration difference between your sphinx compiler and mine or maybe you've set a custom theme for your Godot docs? 🤷 As I've said, the code change here really isn't a show stopper for me, I just find my previous syntax easier to read. |
The docs was dark mode due to my Firefox configuration. I switched Firefox to light mode. Now this reStructuredText source from commit 220fb2b: .. _shader_func_textureProjGrad:
.. rst-class:: classref-method
| |gvec4_type| **textureProjGrad**\ (\ |gsampler2D| s, vec3 p, vec2 dPdx, vec2 dPdy)
| |gvec4_type| **textureProjGrad**\ (\ |gsampler2D| s, vec4 p, vec2 dPdx, vec2 dPdy)
| |gvec4_type| **textureProjGrad**\ (\ |gsampler3D| s, vec4 p, vec3 dPdx, vec3 dPdy) Produces this HTML: <div class="classref-method line-block" id="shader-func-textureprojgrad">
<div class="line"><abbr title="Any of: vec4, ivec4, uvec4">gvec4_type</abbr> <strong>textureProjGrad</strong>(<abbr title="Any of: sampler2D, isampler2D, uSampler2D">gsampler2D</abbr> s, vec3 p, vec2 dPdx, vec2 dPdy)</div>
<div class="line"><abbr title="Any of: vec4, ivec4, uvec4">gvec4_type</abbr> <strong>textureProjGrad</strong>(<abbr title="Any of: sampler2D, isampler2D, uSampler2D">gsampler2D</abbr> s, vec4 p, vec2 dPdx, vec2 dPdy)</div>
<div class="line"><abbr title="Any of: vec4, ivec4, uvec4">gvec4_type</abbr> <strong>textureProjGrad</strong>(<abbr title="Any of: sampler3D, isampler3D, uSampler3D">gsampler3D</abbr> s, vec4 p, vec3 dPdx, vec3 dPdy)</div>
</div> Formatted similar to yours, it is identical to the one you posted. There is no difference in the compiled HTML: <div class="classref-method line-block" id="shader-func-textureprojgrad">
<div class="line">
<abbr title="Any of: vec4, ivec4, uvec4">gvec4_type</abbr>
<strong>textureProjGrad</strong>
(<abbr title="Any of: sampler2D, isampler2D, uSampler2D">gsampler2D</abbr>
s, vec3 p, vec2 dPdx, vec2 dPdy)
</div>
<div class="line">
<abbr title="Any of: vec4, ivec4, uvec4">gvec4_type</abbr>
<strong>textureProjGrad</strong>
(<abbr title="Any of: sampler2D, isampler2D, uSampler2D">gsampler2D</abbr>
s, vec4 p, vec2 dPdx, vec2 dPdy)
</div>
<div class="line">
<abbr title="Any of: vec4, ivec4, uvec4">gvec4_type</abbr>
<strong>textureProjGrad</strong>
(<abbr title="Any of: sampler3D, isampler3D, uSampler3D">gsampler3D</abbr>
s, vec4 p, vec3 dPdx, vec3 dPdy)
</div>
</div> That HTML looks like this in Now it looks fine but some style is still not applied, still. Compared to another function, the compact style still doesn't produce the gray text for the rest of the function (the difference is fairly subtle). On On Since the HTML is identical, this is certainly some difference in configuration between your computer and mine. The more compact style (from 220fb2b) is probably the better choice in the abstract. The more verbose style (from 8972c45) does seem to render more consistently, with this tiny amount of testing. Note that if we switch back to the compact style, there are some unrelated formatting errors that were in 220fb2b. The stray With just the two of us testing, I'm inclined to keep the current verbose style (that is, the current state of this PR) and investigate again after this is merged. |
Sounds like a plan to me. And honestly, don't even feel like additional investigation is really necessary. More consistent/better results should be a higher priority than code style. |
Adds much detail to the documentation on built-in shader functions, copied from (with a few modifications) and linked to official GLSL documentation.
That documentation was moved into a separate file due to its verbosity, and organized into smaller sections following the organizational comments in https://github.com/godotengine/godot/blob/master/servers/rendering/shader_language.cpp.
Style tries to generally follow the styling used in class documentation, but differences between .gdshader and .gdscript meant some changes had to be made.