Zylann/godot_heightmap_plugin

Using `group_uniforms` in a custom shader, creates superfluous subgroups in the hterrain inspector for custom shader.

nan0m opened this issue · 4 comments

nan0m commented

When using group_uniforms in a shader, the group/subgroup shader params is re-added. I think this is more easily explained by a screenshot.
image
I reckon it is related to line 377 in hterrain.gd

To reproduce:
make a custom shader in the plugin
add group_uniforms Test; to it.

Godot 4.1.1
plugin version: 16c0465

Zylann commented

Ew, I had no idea this was even a feature in Godot... no idea how it works either regarding properties returned by RenderingServer.get_shader_parameter_list, there is no information in the docs.
Shader.get_shader_uniform_list has a boolean though, but the doc still doesn't say what the "grouping" is supposed to look like.

Unfortunately, I don't know how this is supposed to work here...
Shader parameters are supposed to be all nested inside a shader_params/ prefix, which Godot interprets as a folder in the inspector.
Godot's Group feature does the same thing but is only an inspector thing and does not modify the way properties are accessed.

But when both things are used, you get this kind of mess...
Currently, the plugin also prefixes shader_params/ to the group names, which ends up in what you see.
But if I make it not prefix groups, I still get a mess:
image
image

One way to "fix this" is to remove groups and add extra prefixes to properties, but it's very bad, because it means any change in groups will actually act as renaming properties and you will loose their values in existing scenes using the previous names.

Looking at how ShaderMaterial handles this mess, I find this happens:
image
No idea yet how it does that (a first glimpse hints that it does A LOT of shit just to handle those groups :( ), but if I do the same thing, shader parameters will end up not shown under the "shader parameters" folder of the terrain inspector. Which is not really desired, but I don't see how else it would work since there already seems to be some incompleteness in the result obtained.

Zylann commented

It seems this works?

		if p.usage == PROPERTY_USAGE_GROUP:
			cp.hint_string = "shader_params/"
		else:
			cp.name = str("shader_params/", p.name)

Still not sure what it is that ShaderMaterial tries to handle with all its code

Zylann commented

Should be fixed in 826518b, you can give it a try

nan0m commented

I will try it ASAP, thank you so much!