-
-
Notifications
You must be signed in to change notification settings - Fork 21.6k
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
Implement documentation comments and tooltips for shader uniform in the inspector. #90161
Conversation
Thanks for opening a pull request 🙂 Note that the proposal had a different syntax in mind, as opposed to using multi-line comments directly: /// Like this (notice the 3 slashes, not just 2).
/// Example continuation line.
uniform int something = 2; The reason we don't want to use standard multi-line comments is that you should be able to have a regular (non-doc) multiline comment above a uniform, and have it not be picked up by the editor inspector for public display. This multiline docblock syntax could also be supported, but it's much more verbose especially when written within a shader. I probably wouldn't bother for now, but I'll leave it here for posterity: /**
* Like this (notice the two asterisks above).
* Example continuation line.
*/
uniform int something = 2; |
It has now been changed to |
b418fd4
to
d241021
Compare
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.
Tested locally, it mostly works as expected. This is a great start!
Some issues I noticed:
- Descriptions don't update after you save the shader more than once per editor session, even if you close and reopen the inspector on the ShaderMaterial. You need to reload the project entirely.
- Syntax highlighting for documentation comments should use a different color to distinguish it from regular comments. See how GDScript does it:
This uses the text_editor/theme/highlighting/doc_comment_color
color defined in the Editor Settings.
- Leading asterisks (
*
) should be stripped from the description on each line, otherwise the following shader code:
/**
* Example description 1.
* Example continuation line 1.
*/
uniform int foo = 1;
will result in:
(The asterisks should not appear in the description here.)
Note that for those wanting a more compact syntax, the following generates a valid tooltip:
/** Example description 4.
* Example continuation line 4. */
uniform int baz2 = 4;
We should make sure to define a guideline for the Godot shaders style guide in the future.
Edit: Pull request opened: godotengine/godot-docs#9300
done |
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.
Tested locally, it works as expected. All issues I mentioned above are now resolved. Great work 🙂
Code looks good to me.
using regular expressions
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.
Seems fine to me. I am not super familiar with editor code, but this is short and self-contained.
I have no preference for the style of the comment block. The short discussion in the proposal seemed to favour the one suggested by Calinou. So I think that is fine as well.
Thanks! |
#ifdef TOOLS_ENABLED | ||
if (Engine::get_singleton()->is_editor_hint()) { | ||
#ifdef MODULE_REGEX_ENABLED | ||
const RegEx pattern("/\\*\\*([^*]|[\\r\\n]|(\\*+([^*/]|[\\r\\n])))*\\*+/\\s*uniform\\s+\\w+\\s+" + pi.name + "(?=[\\s:;=])"); |
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.
Is this taking into account that we can have instance uniform
?
There is also global uniform
, though this one would not show docs I guess?
But instance uniform
does appear in the inspector, right? I think it should show docs.
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.
Yes, IMHO the problem is that instance uniform belongs to per instance, as it's _get_property_list
is in visual_instance_3d
, but I think the impl can be transferred there, but it will need some extra work.
Initially, the implementation was done by modifying
shader_language.cpp
,look at.magian1127@49fe127
However, some issues were discovered, so it was decided to switch to using regular expressions and modify
shader.cpp
to achieve the desired outcome.Using
/** */
on top of uniform will do the trick.一开始是修改 shader_language.cpp 来实现的, 代码看这里.
magian1127@49fe127
但是发现有些问题, 还是改用正则表达式, 以及修改 shader.cpp 来实现.
在 uniform 上面使用
/** */
就可以实现.