Fix missing shader params in headless export

[cgsdev0]

Fixes #66842

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).

I'm not confident this change is 100% correct; I did some testing with my project where I first observed the behavior, and the change did resolve the issue.

This is the function we used to call previously:

godot/scene/resources/shader.cpp

Lines 117 to 141 in a7276f1

	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);
	for (PropertyInfo &pi : local) {
	bool is_group = pi.usage == PROPERTY_USAGE_GROUP || pi.usage == PROPERTY_USAGE_SUBGROUP;
	if (!p_get_groups && is_group) {
	continue;
	}
	if (!is_group) {
	if (default_textures.has(pi.name)) { //do not show default textures
	continue;
	}
	}
	if (p_params) {
	//small little hack
	if (pi.type == Variant::RID) {
	pi.type = Variant::OBJECT;
	}
	p_params->push_back(pi);
	}
	}
	}

I don't really understand what this group logic is doing, and I think my change potentially loses that information. Definitely open to feedback, this is my first time exploring the Godot codebase 😄

(This change isn't needed for 3.x)

[cridenour]

Applying this on top of 4.0.3 solved my shader params missing from headless exports. I am going to do some wider testing to see if there are any other material side effects from this change.

Thank you @cgsdev0

[bitsawer]

This PR kind of works, but it will only export uniforms that previously have their values set and saved in the inspector ui. After applying this PR, adding new uniforms in the shader code no longer appear in the inspector UI, because shader->get_shader_uniform_list() is responsible for fetching these new (and old) uniforms from the RenderingServer. They are not exported, either.

The --headless export problem is a pretty tricky as in the headless mode many Godot servers are not initialized and instead use dummy versions. For example to fix some mesh --headless export issues people have been patching the dummy MeshStorage to keep enough mesh data so that export works. Same kind of issues plague shader uniforms, multimesh etc.

I feel it would required some major refactoring to make --headless export reliable, currently it's pretty much a combination of ad-hoc hacks to fix any errors that people encounter when exporting their specific projects using --headless.

[cridenour]

Thanks for the additional testing @bitsawer - I'll probably roll this back in our build and just avoid headless for the time being.

[cgsdev0]

adding new uniforms in the shader code no longer appear in the inspector UI, because shader->get_shader_uniform_list() is responsible for fetching these new (and old) uniforms from the RenderingServer. They are not exported, either.

thanks for this comment - I feel like this is the piece that I was lacking understanding of

[QbieShay]

Hey, what's the state of this?

[clayjohn]

Hey, what's the state of this?

See bitsawer's comment above

[cgsdev0]

in the spirit of the existing 'ad-hoc hacks', we could gate this change behind a check to see if we're in headless mode. It's not really a long-term solution, but it should at least fix this issue without regressions in the editor.

I've pushed an updated commit with this change.

[bitsawer]

I think that can be a reasonable solution with some changes. It's a hack, but considering what other hacks we have been doing for headless export it's a pretty minor one. A small, nice local hack that should work pretty well. Still, other maintainers are free to offer their opinions if they find a solution like this acceptable.

I suggest some changes, for example window_can_draw() is not a good fit for this as it can also return false if the window is minimized. I would maybe add a new, local, static helper function and call it to determine if we are doing a headless export. Better be explicit. Note that I didn't test or even compile this code, so beware.

static bool is_headless_exporting() {
#ifdef TOOLS_ENABLED
    if (OS::get_singleton()->get_cmdline_args().find("--headless")) {
        for (const String &arg: OS::get_singleton()->get_cmdline_args()) {
            if (arg.begins_with("--export")) {
                return true;
            }
        }
    }
#endif
    return false;
}

And then maybe use it like this, with static const bool as a small performance optimization and to make sure it can't change during runtime.

static const bool is_exporting = is_headless_exporting();
if (is_exporting) {
    // Hacky solution to make --headless exports include shader parameters.
    for (const KeyValue<StringName, Variant> &P : param_cache) {
        list.push_back(PropertyInfo(P.value.get_type(), P.key));
    }
} else {
    shader->get_shader_uniform_list(&list, true);
}

I haven't tested or even compiled these, so please try this out first if you decide to adopt this solution.

[QbieShay]

I think considering the criticality of this issue, we can be okay with a hack for 4.1.1 and then find better solutions that require significant refactors later. Losing uniform on export is pretty critical imo.

Reminder for people reading, clay's opinion and bitsawer's one definitely override mine, it's their expertise way more than mine, but from the point of view of a professional, i think this is a really critical issue that if it can be addressed quickly, even with a hack, it's better than being unable to export the game on CI until a refactor (which takes way more effort and time) is done.

[cgsdev0]

@bitsawer that change seems reasonable to me. Let me know if you think there's a better central place to move the is_headless_exporting function to; I wasn't really sure, so I just left it local for now

I'll do some further testing of this change once the CI builds are done

[RevoluPowered]

good find! could be a little cleaner with this, as I don't think it needs to check if it's an export context, there could be other things like unit tests etc that rely on the shader having the correct parameters:

		const bool headless = DisplayServer::get_singleton()->get_name() == "headless";
		if (headless) {
			// we have to use the cached parameters as we can't query it from the display server
			for (const KeyValue<StringName, Variant> &P : param_cache) {
				list.push_back(PropertyInfo(P.value.get_type(), P.key));
			}
		} else {
			shader->get_shader_uniform_list(&list, true);
		}

Some shorthand could be added to display server like:

 static bool DisplayServer::is_headless() const {
    return DisplayServer::get_singleton()->get_name() == "headless"
 }
// or this 
 bool DisplayServer::is_headless() const {
    return get_name() == "headless"
 }

[tcoxon]

The proposed fix doesn't seem to preserve texture parameters on materials. MRP: 66842.zip

It seems like more info might need to be added to the PropertyInfo to preserve them, but I couldn't figure out what exactly.

[clayjohn]

Just an update on this PR. I think we should aim to solve it properly. The ideal solution is that we store enough data on the DummyRenderingServer to allow the call to get_shader_parameter_list() to return the correct information. The problem here is that shaders/materials rely on passing data to the RenderingServer while loading, and then retrieving that data back before saving. The Dummy server currently just tosses out the data it receives on load. So when the exporter retrieves the data back to save it, it gets default values instead.

[clayjohn]

The proposed fix doesn't seem to preserve texture parameters on materials. MRP: 66842.zip

It seems like more info might need to be added to the PropertyInfo to preserve them, but I couldn't figure out what exactly.

Fixed this in #87392

Headless export from 4.2.1

Headless export from #87392 (I never opened the project in the editor)

[cgsdev0]

@clayjohn i dont think i have bandwidth for this any time soon

feel free to take over the branch and/or close the PR

[clayjohn]

@clayjohn i dont think i have bandwidth for this any time soon

feel free to take over the branch and/or close the PR

No problem! Thank you very much for your initial work and ensuring that this got the attention it needed.

Superseded by #87392