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

[WIP] Move visual shaders to a module #64596

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Aug 19, 2022

Do not merge yet! I will make other PRs that better abstract this code, so that it doesn't have to be as hacky. This one needs to be merged first: #65276

This PR moves visual shaders to a module. I have three reasons for doing this:

  • Organization. It's big enough that it makes sense to put visual shader stuff in a separate place.
  • Allowing disabling it. In my testing, disabling visual shaders shrinks the engine by about 3% in a release build. This may seem small but when disabling a lot of optional features the savings can add up.
  • I discussed this with visual shader contributors, and @QbieShay had the idea of later on making something that compiles visual shaders to text shaders. This means that we could save about 3% of the executable size on exported games even for games that do use visual shaders. This PR does not implement this, it just moves things to a module, and allows disabling it, which is a prerequisite to the text shader compilation idea being useful.

@aaronfranke aaronfranke added this to the 4.0 milestone Aug 19, 2022
@aaronfranke aaronfranke changed the title [WIP] Move visual shaders to a module Move visual shaders to a module Aug 21, 2022
@aaronfranke aaronfranke marked this pull request as ready for review August 21, 2022 20:53
@aaronfranke aaronfranke requested review from a team as code owners August 21, 2022 20:53
@QbieShay
Copy link
Contributor

Thank you @aaronfranke !
I am not proficient enough to do a code review, but i wanted to ask you, what happens if you have a visual shader (maybe in an addon or something) and you export the build without VS? will it crash? will it show an error message?

@aaronfranke
Copy link
Member Author

aaronfranke commented Aug 22, 2022

@QbieShay It will not crash, the class will simply be unavailable. In the console it will print ERROR: Cannot get class 'VisualShader'.. Any ShaderMaterials with a VisualShader assigned will have null assigned instead. When opening in the editor, the data will be preserved with the new MissingResource system:

Screen Shot 2022-08-22 at 6 56 50 PM

editor/editor_node.cpp Outdated Show resolved Hide resolved
Comment on lines +1291 to 151
#ifdef MODULE_VISUAL_SHADER_ENABLED
Ref<VisualShader> vs = es.shader;
if (vs.is_valid()) {
es.visual_shader_editor = memnew(VisualShaderEditor);
shader_tabs->add_child(es.visual_shader_editor);
es.visual_shader_editor->edit(vs.ptr());
Copy link
Member

Choose a reason for hiding this comment

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

This should likely be refactored further so that such hacks are not needed. A module should be modular and thus self-contained (sometimes it's not possible and we do allow hacks like this occasionally, but this needs to be assessed).

Copy link
Member

Choose a reason for hiding this comment

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

Basically, if we want VisualShader to be a module, it should have the same relationship to the ShaderEditor and (script) Shader as VisualScript has to the ScriptEditor and GDScript.

There are no uses of MODULE_VISUAL_SCRIPT_ENABLED anywhere in the editor code, everything is self-contained. There are a few references by String though which are allowed:

$ rg '"VisualScript"'
editor_node.cpp
2306:                   if (main_plugin->get_name() == "Script" && !current_obj->is_class("VisualScript") && current_res && !current_res->is_built_in() && (bool(EditorSettings::get_singleton()->get("text_editor/external/use_external_editor")) || overrides_external_editor(current_obj))) {

editor_asset_installer.cpp
122:            extension_guess["vs"] = tree->get_theme_icon(SNAME("VisualScript"), SNAME("EditorIcons"));

translations/de.po
17338:msgstr "VisualScript"

plugins/script_editor_plugin.cpp
2270:                   !p_resource->is_class("VisualScript")) {
2369:   if (!p_resource->is_class("VisualScript")) {

Comment on lines +47 to +49
#ifdef MODULE_VISUAL_SHADER_ENABLED
SHADER_TYPE_VISUAL,
#endif // MODULE_VISUAL_SHADER_ENABLED
Copy link
Member

Choose a reason for hiding this comment

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

This is problematic as it may break compatibility. Does this ShaderType get saved to some scene/resource files?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's only used in the shader create dialog.

Copy link
Member

Choose a reason for hiding this comment

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

But doesn't it remember the selected value for future runs? If so, the selected value might change (similarly to what happens when using C# in the script create dialog and then switching to a non-Mono build). It may not be a big deal though, unless you have SHADER_TYPE_INC selected in a build with VisualShader, and then when you open in a build without VisualShader this becomes SHADER_TYPE_MAX and throws an error.

@aaronfranke aaronfranke changed the title Move visual shaders to a module [WIP] Move visual shaders to a module Sep 15, 2022
@aaronfranke aaronfranke modified the milestones: 4.0, 4.x Sep 16, 2022
@QbieShay QbieShay removed this from the 4.x milestone Feb 9, 2023
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.

None yet

3 participants