Disallow renderables with missing required attributes

[bejado]

All lit materials implicitly require the tangents attribute. Not including tangents for lit renderables causes undefined behavior in OpenGL and a crash in Metal.

RenderableManager permits renderables to be created with missing required attributes; however, missing attributes are logged in two places:

• RenderableManager::Builder::build
• FRenderableManager::setMaterialInstanceAt

A better solution would be to somehow disallow renderables with insufficient attributes. Ideally, renderables should be immutable. This way, the checks at build and setMaterialInstanceAt could be promoted to errors.

[prideout]

Related PR is #1290

[prideout]

Some questions arise when thinking about this:

• Why do our renderables allow users to change out the VertexBuffer? The VertexBuffer defines which attributes are present so this makes validation complex. If Sceneform does not need this ability then we should consider removing it. Note that renderables also allow users to swap out materials.
• Do all our backends have the ability to supply dummy attributes? In OpenGL you can provide a default value that is spread across all vertices using glVertexAttrib4f. This feature seems unavailable with Vulkan / Metal, although in theory you can set the stride to 0.
• Which attributes should trigger actual errors when missing? The presence of tangents should perhaps be a hard requirement for lit variants, but our skinning+morphing variant uses 10 attributes (8 for morphing, 2 for skinning) and we should definitely not require users to supply all of these.

[bejado]

On point 2, the Metal backend can create a small zero-ed out "dummy" buffer for missing vertex attributes and use the MTLVertexStepFunctionConstant step function.

In order to do that, the backend needs to know which attributes are "expected" for a given program. I propose we add an AttributeBitset field to Program that gets set up by Filament. The backend can use it to determine which attributes are read from in the shader, but aren't provided by the renderable.

[prideout]

See also #1663

[prideout]

Let me add yet another bullet to my list:

• Why do our renderables allow users to change the MaterialInstance? This causes the same validation issue that swapping out the VertexBuffer has. For example, Filament lets you switch from an unlit material to a lit material.

[romainguy]

It can be extremely useful if you want to change the visual state of an object. You could create a different renderable but this is much easier.

[prideout]

Creating a new renderable would be super easy if we exposed a clone method. Also, users can always call setParameter on the material instance, the problem arises only when they want a drastic change like the shading model or set of attributes.

[pixelflinger]

I agree with @prideout my original intent was for all these things to be immutable.

[prideout]

@pixelflinger I complained about mutability in 2019 but unfortunately the ship has already sailed. We now have several users who swap out materials nowadays. In fact we made VertexBuffer more mutable with the advent of BufferObject, in order to satisfy glTF morphing requirements.

Anyway: I feel this bug is obsolete and that we can close it.