Support CUSTOM shader attributes in 2D

[Giwayume]

History: #76276

Closes: godotengine/godot-proposals#6703

A general summary of changes:

• The vertex shader attributes CUSTOM0, CUSTOM1 are now supported for canvas item shaders for all renderers
• The vertex shader attributes CUSTOM2, CUSTOM3 are not supported in canvas item shaders in order to save attribute slots for future use

CUSTOM0 and CUSTOM1 vertex attributes are placed in the same attribute slot as their 3D counterparts. This simplifies the changes, so we don't have to modify things like the bit shift mask variables, or remap them.

In OpenGL ES 3, attribute layout slots 14 & 15 should exist under the minimum GL specification, so all of the slots are shifted down to make room for slots 6 & 7 to be used for custom attributes. You can read more about this in the proposal linked above.

Sample project:
CanvasShaderTest.zip

[Giwayume]

@Calinou Please elaborate what you're looking for in a testing project beyond the sample project attached.

[AThousandShips]

Now solved 🙂

[Giwayume]

I added the CUSTOM0-3 properties to 2D visual shader. Missed that initially.

The code is still working for forward/mobile renderer, but stopped working for compatibility after the merge.

Looks like something changed where the input_mask passed into mesh_storage->mesh_instance_surface_get_vertex_arrays_and_format is always static for 2D meshes.

So, I first dived into trying to add vertex_input_mask to CanvasShaderData, just like how SceneShaderData works, manipulating CanvasShaderData::set_code and all, but I've come to the conclusion that it may not be worth it. All the extra code to read the materials just to ultimately prevent setting default values for a couple of vertex attributes is probably not worth the cost. What do you think?

I just added the customs to the end of the input mask for every 2D mesh in GLES3:
uint64_t input_mask = RS::ARRAY_FORMAT_VERTEX | RS::ARRAY_FORMAT_COLOR | RS::ARRAY_FORMAT_TEX_UV | RS::ARRAY_FORMAT_CUSTOM0 | RS::ARRAY_FORMAT_CUSTOM1;

I also updated the sample project with more test cases.
CanvasShaderTest.zip

[Giwayume]

If this PR hangs for another couple of months with no input like the last one, I'm giving up.

[fire]

I called a few people to review.

[clayjohn]

This is worth some discussion with others in the rendering team as I am unsure what to do with this.

An important design goal for us is to make basic rendering features compatible across all backends. Users should be able to rely on basic features continuing to work even when switching to the compatibility renderer. We try to limit the number of things that aren't supported in all backends. For the most part, things that aren't supported by all backends are optional features that can be turned on/off without significantly breaking the visuals (i.e. SDFGI, volumetric fog, etc.).

This feature is a bit different. If we move forward with it, users will be able to create assets that work only in the RD-based backends. If they convert their project to the compatibility backend, their meshes will stop working entirely and they will have to re-design their assets to continue working.

I don't think that is a situation I am comfortable introducing into the engine. Accordingly, in my mind, the only acceptable options are:

1. Only support CUSTOM0 and CUSTOM1 in the canvas item shader
2. Only support CUSTOM0 in the canvas item shader
3. Don't support any CUSTOM attributes in the canvas item shader

Seeing as the original proposal is only asking for one custom channel. It may make sense to just expose one custom value for now, and then expose another if there is demand. The trouble with exposing as many as possible is that we can't go backwards (i.e. we can always add more custom attributes, but we can never remove custom attributes).

This should be discussed with other rendering contributors in our rendering meetings

[Giwayume]

1. Only support CUSTOM0 and CUSTOM1 in the canvas item shader

2. Only support CUSTOM0 in the canvas item shader

3. Don't support any CUSTOM attributes in the canvas item shader

What would be the reason to pick 2 or 3 over 1? Saving vertex attribute slots for future unknown use?

[clayjohn]

1. Only support CUSTOM0 and CUSTOM1 in the canvas item shader

2. Only support CUSTOM0 in the canvas item shader

3. Don't support any CUSTOM attributes in the canvas item shader

What would be the reason to pick 2 or 3 over 1? Saving vertex attribute slots for future unknown use?

That's right. If users only need one CUSTOM slot, then there isn't much point in exposing 2, especially if it might block us from adding a different feature down the road

[QbieShay]

I read this when fire pinged us and I was unsure myself. How does this interact with potentially exposing instance customs for canvas items? Are they sharing this resource?

Regarding the compatibly between Rd and compat renderer, I do think that having assets that work in Rd and not in compat is a big problem, especially since our default renderer is Rd and this is about 2D. I think users will stumble on this way too much: they decide to make a 2D game that has a couple of effects, start in Rd, then decide to publish on low end, switch to compat, everything breaks. For me 2d is more critical than 3D for compatibility.

[Giwayume]

How does this interact with potentially exposing instance customs for canvas items? Are they sharing this resource?

In the case of Mesh, location 5 (instance_color_custom_data) is unused and in the case of both Mesh and MultiMesh, location 6 CUSTOM0 could be used.

I don't believe it makes sense to share this, because the user would run into an issue where the code allows them to define both for a MultiMesh, but one would override the other, causing confusion.

The offset for CUSTOM0 needs to be at 6 either way, in order for the bit shift masks to work correctly across all renderers.

[fire]

Another possibility to do the the complex tasks behind reorganizing these attributes in the code so that we get all CUSTOM variables for compatibility renderer and the forward renderer, but it would good to estimate and get support from the rendering team about merging before doing the work.

[Giwayume]

Another possibility to do the the complex tasks behind reorganizing these attributes in the code so that we get all CUSTOM variables for compatibility renderer and the forward renderer, but it would good to estimate and get support from the rendering team about merging before doing the work.

GLES3 is nearly at the limit for canvas item attributes even with a reorg, if there is a concern about blocking new functionality that would still apply. If all CUSTOM0-3 were supported, that leaves 1 attribute slot left over.

[clayjohn]

We discussed this yesterday in our weekly rendering meeting. We agreed that exposing CUSTOM0 and CUSTOM1 would be a good first step. In looking at the code closer we realized that we can free up one more slot if we need to by combining VERTEX and UV into one attribute. Therefore, we can safely expose CUSTOM0 and CUSTOM1 while having room for the future (which I think we will need quite soon actually).

In theory we can expose more slots if we create a fallback shader that doesn't use attributes for batching properties. However, doing so will be a good amount of work and will make the code much more complex. So we prefer to only do that if we absolutely have to. Since the current users aren't asking for more than 1 CUSTOM vector, exposing 2 should be more than enough.

In summary, for this PR to be merged, it just needs to be updated to only expose CUSTOM0 and CUSTOM1 for both RD renderers and the compatibility renderer.

[Giwayume]

I removed CUSTOM2 and CUSTOM3, updated demo project with MultiMesh and visual shader sample. Updated GLES3 material storage to build vertex_input_mask instead of it being hard-coded.

CanvasShaderTest.zip

[lyuma]

Just wanted to leave a note here as well in case people do need more than 8 channels of custom data in 2D...

Because CUSTOM0-3 can support 4-component full 32-bit floats, there is nothing stopping a developer from bitpacking attribute data from individual 16-bit half floats or even 8-bit integers (which is sufficient for colors), in which case you could possibly support 24-32 custom 8-bit channels (depending on how floatBitsToUint is used in your shader and how careful you are to avoid NaN)

I can't find any examples of people using this technique in practice, I suspect because people usually don't reach attribute limits... but anyway just want to put it out there. (and as always, be careful about perforamnce of vertex operations on old mobile hardware, especially with texture coords)

[Giwayume]

Is anyone scheduled to review this?

[clayjohn]

Is anyone scheduled to review this?

Not scheduled, no. But I started reviewing it and got pulled away to other tasks. I aim to finish the review shortly

[clayjohn]

Looks great so far!

I tested locally and can confirm that it works. I left some comments on cleaning up the code a bit and optimizing out some unnecessary work. Please let me know if you have any questions or if any of my suggestions don't make sense.

[clayjohn]

Suggested change

	shader_modes[RS::SHADER_CANVAS_ITEM].functions["vertex"].built_ins["CUSTOM0"] = ShaderLanguage::TYPE_VEC4;
	shader_modes[RS::SHADER_CANVAS_ITEM].functions["vertex"].built_ins["CUSTOM1"] = ShaderLanguage::TYPE_VEC4;
	shader_modes[RS::SHADER_CANVAS_ITEM].functions["vertex"].built_ins["CUSTOM0"] = constt(ShaderLanguage::TYPE_VEC4);
	shader_modes[RS::SHADER_CANVAS_ITEM].functions["vertex"].built_ins["CUSTOM1"] = constt(ShaderLanguage::TYPE_VEC4);

These should likely be constant as I don't see a reason why they would be written to. Weirdly enough the CUSTOM values in the spatial shader aren't marked as being constant. But since they aren't available in the fragment shader its kind of pointless to write to them

[Giwayume]

Is this wrong on the Spatial side too? That is what I based it from.

[clayjohn]

In my opinion, yes its wrong. However, to make the variables constant would technically break compatibility as users could do the following:

void vertex() {
	CUSTOM0 = some_vec4;
	...
	some_other_variable = CUSTOM0;
}

Its kind of a crappy situation as we either break compatibility or we have inconsistent behavior between the shaders

[Giwayume]

@clayjohn your feedback is implemented. I believe I cleaned up all the unnecessary variables now.

[clayjohn]

Looks great to me! Good work!

Thanks for sticking through multiple discussions and suggested changes. I think we settled on a good compromise that should make everyone happy.

[akien-mga]

Thanks!