Define shader language project settings before creation of TextShaderEditor object.

[smosages]

Fix #73336

Preamble: this is my first contribution. From the description and the comment from @Calinou , I have moved the GLOBAL_DEF for shader_language settings into the ProjectSettings contstructor.

[clayjohn]

Looks like you are getting a few errors connected to ShaderWarning not being available. Note that ShaderWarning is only defined when DEBUG_ENABLED is defined (i.e. in editor builds). So any code referencing it outside of editor classes needs to be wrapped in an #ifdef DEBUG_ENABLED conditional.

Further, core classes like ProjectSettings should not rely on non-core code like shader_warnings.cpp. The definitions, should likely be moved to rendering_server.cpp

godot/servers/rendering_server.cpp

Line 2858 in 29f670b

void RenderingServer::init() {

[smosages]

thanks. I've moved these definitions into rendering_servers. Why is this the right place for them? It's necessary to wrap ShaderWarning in an #ifdef in rendering_server.cpp? I don't see any other definitions in rendering_server.cpp using #ifdef DEBUG_ENABLED. Why is that?

[smosages]

@clayjohn , this? or wrap all GLOBAL_DEF inside DEBUG_ENABLED?

void RenderingServer::init() {
        ...
        ...
	GLOBAL_DEF("debug/shader_language/warnings/enable", true);
	GLOBAL_DEF("debug/shader_language/warnings/treat_warnings_as_errors", false);

#ifdef DEBUG_ENABLED
	for (int i = 0; i < (int)ShaderWarning::WARNING_MAX; i++) {
		GLOBAL_DEF("debug/shader_language/warnings/" + ShaderWarning::get_name_from_code((ShaderWarning::Code)i).to_lower(), true);
	}
#endif
}

[clayjohn]

thanks. I've moved these definitions into rendering_servers. Why is this the right place for them?

Its a class that is always initialized and it allows you to keep rendering server features within the rendering_server. We have a chain of allowed dependencies that goes like this Core < Servers < Scene < Editor. You can include stuff from the left but not the right. So anything can depend on Core, but core shouldn't depend on servers, scene, or editor.

The ShaderWarnings class is in Servers, so it shouldn't be referenced in Core, but it is okay to be referenced in rendering_server.

It's necessary to wrap ShaderWarning in an #ifdef in rendering_server.cpp? I don't see any other definitions in rendering_server.cpp using #ifdef DEBUG_ENABLED. Why is that?

None of the other definitions rely on a class that is conditionally compiled. The issue here is that ShaderWarning is only compiled when DEBUG_ENABLED is defined. before it was okay without the #ifdef because the code was in the editor (which is always wrapped in #ifdef DEBUG_ENABLED)

@clayjohn , this? or wrap all GLOBAL_DEF inside DEBUG_ENABLED?

void RenderingServer::init() {
        ...
        ...
	GLOBAL_DEF("debug/shader_language/warnings/enable", true);
	GLOBAL_DEF("debug/shader_language/warnings/treat_warnings_as_errors", false);

#ifdef DEBUG_ENABLED
	for (int i = 0; i < (int)ShaderWarning::WARNING_MAX; i++) {
		GLOBAL_DEF("debug/shader_language/warnings/" + ShaderWarning::get_name_from_code((ShaderWarning::Code)i).to_lower(), true);
	}
#endif
}

This looks fine. You just need it around the sections referencing ShaderWarning

[clayjohn]

Just some comments on formatting changes

[clayjohn]

Looks great! The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

[brno32]

@clayjohn the commits are squashed. Can this be merged?

[clayjohn]

Looks great now! Sorry for the slow re-review

[akien-mga]

Thanks! And congrats for your first merged Godot contribution 🎉

[AThousandShips]

This appears to break CI

[AThousandShips]

Making a fix