Spatial Shader with no render_mode set appears to default to "specular_disabled" rather than "specular_schlick_ggx"

[Arnklit]

Godot version:

3.2.4.rc4

OS/device including version:

NA

Issue description:

When no specular rendermode is specified, Spatial Shaders appear to default to specular_disabled. The manual says that specular_schlick_ggx is supposed to be the default. specular_schlick_ggx also is set both when converting a Spatial Material to a shader and making a visual shader, so it appears it should be the default everywhere?
https://docs.godotengine.org/en/stable/tutorials/shading/shading_reference/spatial_shader.html#render-modes

Steps to reproduce:
See screebshot above

Minimal reproduction project:

See screebshot above

[clayjohn]

The documentation needs to be updated to say that specular_disabled is the default

Edit: the default behaviour is actually a little odd. Internally, the light function runs through an if/elif branch to determine the specular mode. If no mode is set, no specular is added (same as the specular_disabled branch). This makes the de facto default "disabled". However, for vertex-lit materials, the logic is "use blinn unless disabled" so the default is to use Blinn if nothing is specified. I guess we need to clean up this behaviour and make it consistent. And the best way to do that is probably to change the if/elif branch to make GGX default e.g. change the final "elif" to an "else"

See here:

godot/drivers/gles3/shaders/scene.glsl

Lines 1134 to 1140 in 64a9e86

	#elif defined(SPECULAR_DISABLED)
	// none..
	#elif defined(SPECULAR_SCHLICK_GGX)
	// shlick+ggx as default

and here:

godot/drivers/gles2/shaders/scene.glsl

Lines 1357 to 1362 in 64a9e86

	#elif defined(SPECULAR_DISABLED)
	// none..
	#elif defined(SPECULAR_SCHLICK_GGX)
	// shlick+ggx as default

[AnaDenisa]

I would like to work on this issue. Could you assign me here, please?

[Calinou]

@AnaDenisa Thanks for your interest in contributing! No need to be assigned – you can start working on it directly 🙂

[JonatasOrsini]

Hi, to what branch a commit should be made?

[Calinou]

@JonatasOrsini The general rule is to open pull requests against the master branch. However, some pull requests need to be done separately for the 3.2 branch (such as this one), so you should open it against the 3.2 branch instead of master.

The master branch currently only has a Vulkan renderer whereas the 3.2 branch has GLES3 and GLES2 renderers.

[akien-mga]

I guess we need to clean up this behaviour and make it consistent. And the best way to do that is probably to change the if/elif branch to make GGX default e.g. change the final "elif" to an "else"

I'm not sure about this, looking at the shader there are several locations where #if defined(SPECULAR_SCHLICK_GGX) is checked, and changing only one of those to #else would likely only solve part of the problem.

But more importantly, SPECULAR_SCHLICK_GGX is the default value for Material::specular_mode, so I don't understand why the define doesn't end up in the final shader. The proper fix IMO would be to make sure that the generated shader includes the defines that match default values. Reading material.cpp::_update_shader() that's what it seems to do, but I guess I'm missing something? I put a breakpoint there and modifying the shader script in the editor doesn't seem to trigger an _update_shader() call.

[JonatasOrsini]

Please diregard my entry.