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

Shader parameters specified in the inspector are not included when exporting a project in headless mode #66842

Open
GullikX opened this issue Oct 3, 2022 · 24 comments · Fixed by #87392

Comments

@GullikX
Copy link

GullikX commented Oct 3, 2022

Godot version

4.0.beta2.official.f8745f2f7

System information

Void linux

Issue description

When exporting a project using --headless --export-pack, any shader parameters/uniforms specified in the inspector are not included in the exported project. All shader parameters are reverted to their default values (or null).

Steps to reproduce

Project setup:

  1. Create a cube mesh.
  2. Add a shader material to the cube with the following simple shader:
shader_type spatial;
render_mode unshaded;

uniform vec4 color: source_color = vec4(0.0, 1.0, 0.0, 0.0); // Green

void fragment() {
        ALBEDO = color.rgb;
}
  1. In the inspector, set the 'color' shader parameter/uniform to be a red color.
  2. When running the project from the editor, the cube is now red.

Export without --headless option:

rm -rf .godot
./Godot_v4.0-beta2_linux.x86_64 --editor --export-pack Linux/X11 export.zip
./Godot_v4.0-beta2_linux.x86_64 --main-pack export.zip
# Cube is red (correct behavior)

Export with --headless option:

rm -rf .godot
./Godot_v4.0-beta2_linux.x86_64 --editor --headless --export-pack Linux/X11 export_headless.zip
./Godot_v4.0-beta2_linux.x86_64 --main-pack export_headless.zip
# Cube is green (incorrect behavior)

The behavior can be reproduced on Godot 4.0 alpha 16 through Godot 4.0 beta 2.

Minimal reproduction project

ShaderParameterHeadless.zip

@clayjohn clayjohn added this to the 4.0 milestone Oct 3, 2022
@clayjohn clayjohn added the bug label Oct 3, 2022
@GullikX
Copy link
Author

GullikX commented Oct 5, 2022

Git bisect indicates that this bug was introduced in commit ef17c46.

@akien-mga akien-mga moved this to To Assess in 4.x Priority Issues Oct 5, 2022
@akien-mga akien-mga moved this from To Assess to Todo in 4.x Priority Issues Oct 5, 2022
@GullikX
Copy link
Author

GullikX commented Oct 8, 2022

It seems like shader parameters get lost when scene files are converted to binary during export. A workaround is to disable 'convert text resources to binary' in the project settings.

@clayjohn clayjohn modified the milestones: 4.0, 4.x Jan 14, 2023
@jayypluss
Copy link

jayypluss commented Mar 17, 2023

I've had the same problem, I changed the default params of the shader to reflect what I wanted directly in the shader file.

It seems like shader parameters get lost when scene files are converted to binary during export. A workaround is to disable 'convert text resources to binary' in the project settings.

For future reference, this option is under Editor -> Export, you need to activate the Advanced Settings switch.

@cgsdev0
Copy link

cgsdev0 commented Apr 14, 2023

I spent a lot of time investigating this tonight

It seems like this code is problematic in headless mode:

if (type == "PackedScene") { // Its a scene.
Ref<PackedScene> ps = ResourceLoader::load(p_path, "PackedScene", ResourceFormatLoader::CACHE_MODE_IGNORE);
ERR_FAIL_COND_V(ps.is_null(), p_path);
Node *node = ps->instantiate(PackedScene::GEN_EDIT_STATE_INSTANCE); // Make sure the child scene root gets the correct inheritance chain.
ERR_FAIL_COND_V(node == nullptr, p_path);
if (!customize_scenes_plugins.is_empty()) {
for (Ref<EditorExportPlugin> &plugin : customize_scenes_plugins) {
Node *customized = plugin->_customize_scene(node, p_path);
if (customized != nullptr) {
node = customized;
modified = true;
}
}
}
if (!customize_resources_plugins.is_empty()) {
if (_export_customize_scene_resources(node, node, customize_resources_plugins)) {
modified = true;
}
}
if (modified || p_force_save) {
// If modified, save it again. This is also used for TSCN -> SCN conversion on export.
String base_file = p_path.get_file().get_basename() + ".scn"; // use SCN for saving (binary) and repack (If conversting, TSCN PackedScene representation is inefficient, so repacking is also desired).
save_path = export_base_path.path_join("export-" + p_path.md5_text() + "-" + base_file);
Ref<PackedScene> s;
s.instantiate();
s->pack(node);
Error err = ResourceSaver::save(s, save_path);
ERR_FAIL_COND_V_MSG(err != OK, p_path, "Unable to save export scene file to: " + save_path);
}

My working theory (as a total godot codebase noob) is that when we instantiate the node in headless mode, certain things (i.e. shader parameters) are ignored since they're unnecessary. This makes sense when we're running the game, but not when we're exporting.

I plan to continue working on this issue, but I figured I'd leave some notes here for myself and anyone interested.

@cgsdev0
Copy link

cgsdev0 commented Apr 16, 2023

I think I've narrowed down the problem, but I'm not really sure what a good impl for a fix looks like here.

void Shader::get_shader_uniform_list(List<PropertyInfo> *p_params, bool p_get_groups) const {
_update_shader();
List<PropertyInfo> local;
RenderingServer::get_singleton()->get_shader_parameter_list(shader, &local);

This function gets called in the path of exporting now, which, in headless mode, ends up calling this function:

virtual void get_shader_parameter_list(RID p_shader, List<PropertyInfo> *p_param_list) const override {}

Since the headless rendering server doesn't keep track of shader parameters, they get lost when we instantiate the tree.

@davidsmith1307
Copy link

Can confirm that the issue is still present in 4.0.3 and the workaround still works.

@Tichau
Copy link

Tichau commented Aug 22, 2024

I think it needs to be reopen.
This issue is still present with v4.3.stable.mono.official [77dcf97] (the workaround still solves it).

@clayjohn clayjohn reopened this Sep 2, 2024
@clayjohn clayjohn self-assigned this Sep 2, 2024
@akien-mga akien-mga modified the milestones: 4.3, 4.4 Sep 11, 2024
@jupiterbjy
Copy link

jupiterbjy commented Oct 23, 2024

Reproducible in 4.4 dev 3

so basically this makes CI/CD build kinda broken for my environment, any updates on this?

@clayjohn
Copy link
Member

@jupiterbjy Do you have a new MRP for testing? (or @Tichau?)

I can't reproduce the issue with 4.3 using the MRP from the OP.

@jupiterbjy
Copy link

jupiterbjy commented Oct 24, 2024

@clayjohn Not mre yet but have proof here which I'm making as part of company work:

image

So ground tile there is simple shader that has instance parameters for grid cell size, and as you see one with --headless this is changed back to default value(which makes it larger as result here).

so TL;DR instance param still seems to be broken. I'll try making MRE in free time, sorry I can't directly show you code of this haha

@jonjondev
Copy link

Our team is currently experiencing this issue as of 4.3 binary release: CapsCollective/sunflower#45

Setting convert_text_resources_to_binary provides a temporary workaround. Waiting for proper fix, however.

@github-project-automation github-project-automation bot moved this to For team assessment in Rendering Issue Triage Nov 3, 2024
@clayjohn clayjohn moved this from For team assessment to Up for grabs in Rendering Issue Triage Nov 3, 2024
@clayjohn
Copy link
Member

clayjohn commented Nov 3, 2024

So it looks like both instance uniforms and global uniforms are missed when doing a headless export. It should not be too much work to add them to the dummy rendering server as well

@clayjohn clayjohn moved this from Unassessed to Very Bad in 4.x Release Blockers Nov 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment