Skip to content
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

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

Merged
merged 1 commit into from
Oct 21, 2024

Conversation

tetrapod00
Copy link
Contributor

@tetrapod00 tetrapod00 commented Sep 19, 2024

Implements and closes godotengine/godot-proposals#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 tetrapod00 requested review from a team as code owners September 19, 2024 20:56
@tetrapod00 tetrapod00 changed the title Add "Inspect Native Shader Code" to shader resource and shader editor Add "Inspect Native Shader Code" to shader inspector and shader editor Sep 19, 2024
@AThousandShips AThousandShips added this to the 4.x milestone Sep 20, 2024
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally on both text and visual shaders, it works as expected.

Copy link
Member

@clayjohn clayjohn left a 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!

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Sep 20, 2024
@tetrapod00
Copy link
Contributor Author

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.

editor/plugins/shader_editor_plugin.cpp Outdated Show resolved Hide resolved
scene/resources/shader.cpp Outdated Show resolved Hide resolved
@tetrapod00 tetrapod00 requested a review from a team as a code owner October 7, 2024 23:37
@tetrapod00
Copy link
Contributor Author

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:

List<MethodInfo> methods;
p_object->get_method_list(&methods);
if (!methods.is_empty()) {
bool found = false;
List<MethodInfo>::Element *I = methods.front();
int i = 0;
while (I) {
if (I->get().flags & METHOD_FLAG_EDITOR) {
if (!found) {
p->add_separator();
found = true;
}
p->add_item(I->get().name.capitalize(), OBJECT_METHOD_BASE + i);
}
i++;
I = I->next();
}

@clayjohn
Copy link
Member

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

@tetrapod00
Copy link
Contributor Author

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.

@Repiteo Repiteo merged commit 7a438dc into godotengine:master Oct 21, 2024
19 checks passed
@tetrapod00 tetrapod00 deleted the inspect-native-shader-code branch October 21, 2024 21:49
@Repiteo
Copy link
Contributor

Repiteo commented Oct 21, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make "inspect native shader code" available also when selecting shader resources
5 participants