godot icon indicating copy to clipboard operation
godot copied to clipboard

Add "Inspect Native Shader Code" to shader inspector and shader editor

Open tetrapod00 opened this issue 1 year ago • 2 comments

Implements and closes https://github.com/godotengine/godot-proposals/issues/10278.

Adds the action "Inspect Native Shader Code", currently available on the Material resource inspector (implementation), to the Shader resource inspector and to the shader editor. LMPrAaIzQi SC05RjXnlO

godot windows editor dev x86_64_tU88Nvc2Ah godot windows editor dev x86_64_hcpELW4Tsh godot windows editor dev x86_64_OBXfqaaZUL

I added the action to the shader editor under the File menu, because it's a valid action for both text and visual shaders. I can see reasons to instead do one of the following:

  • Add the action to one of the other menus, like Edit, which only appear for text shaders. This action is for advanced users, currently takes multiple seconds to open on my machine, and may be confused with the "Show Generated Shader Code" action in visual shaders. On the other hand, it is a valid action for both types of shader.
  • Not include this action in the shader editor at all. Perhaps it will cause more confusion than its worth. With this PR, you can now do File -> Open File in Inspector and then Manage object properties -> Inspect Native Shader Code.

tetrapod00 avatar Sep 19 '24 20:09 tetrapod00

Maybe hold off on merging. I want to see if being exposed to scripting causes problems, and if there's a good way to avoid exposing at least the new implementation to scripting. Un-exposing the Material implementation probably breaks compat even if it's correct to do.

tetrapod00 avatar Sep 20 '24 20:09 tetrapod00

Made the small style changes.

I also looked into whether the method needs to be exposed to the class DB and scripting, and I believe it does. The resource inspector dock uses the class DB to add methods to the Manage Object Properties menu. If we were only adding this method to the shader editor, there would be no need to expose it to scripting.

See the inspector dock implementation: https://github.com/godotengine/godot/blob/842f98239713fd10cfd648cd6aa3781895f289eb/editor/inspector_dock.cpp#L566-L583

tetrapod00 avatar Oct 07 '24 23:10 tetrapod00

@tetrapod00 I'm a little lost in the last few comments. Is this PR ready for merging now?

clayjohn avatar Oct 16 '24 23:10 clayjohn

Yes, the PR is ready to merge.

The function Shader.inspect_native_shader_code() is exposed to scripting, which seemed unnecessary. I looked to see if there was a way to add this action to the Shader resource's inspector, without exposing it to scripting, but could not find one. The rest is all details.

tetrapod00 avatar Oct 16 '24 23:10 tetrapod00

Thanks!

Repiteo avatar Oct 21 '24 21:10 Repiteo