Yeah, this struct could likely be moved to a more global location. The thinking of having it local to the shader class was that this contains a subset of global state which feeds PSO creation (which is shader specific), and is then used as a key in the hash map of previously compiled PSOs.

In this case, PSO backend compilation combines the intermediate representation of the compiled shader library with the render pipeline state to compile the final shader program which will execute on the GPU.

Currently, this struct has data copied in prior to PSO creation to ensure everything is up to date based on the current pipeline state. This mechanism could still happen in the same way, to avoid double-population of state.

There is also a separation between this struct and the MTLGlobalPipelineState/MTLRenderPassState, as this one contains only the hashable subset which feeds the MTLRenderPipelineDescriptor for shader-specific PSO creation.

The reason this is also a member struct rather than a local one is such that MTLBatch and MTLImmediate can directly populate the Vertex descriptor in this struct. However, this could most likely be replaced with a global vertex descriptor in a global copy of this struct.

Yeah, you're right, this appears to be stored unnecessarily, and if it ever did need to be fetched, it could just be found from the corresponding key in the pso_cache_ lookup.

This is a reference to a specialised variant of the vertex/fragment function for the current shader. Storing it here is primarily just a means of keeping a reference to the resource. Such that it can be retained while in use in a PSO, and released when the shader object (and pso_cache_) is destroyed.

When the backend was first implemented, interface sharing, and external interface creation was supported, though since everything now goes through the GLSL pipeline, rather than using hand-written MSL shaders, this is now no longer needed.

It was kept as a convenience to avoid repeated casting. Though I imagine the compiler probably negates any overhead with this. I can remove this.

Compute support is still something needing attention. It should not be too complicated and should be more straight forward than the existing shader translation. Though I can remove this as soon as that support is in place.

Would it be okay to keep this as a stop-gap for the time being, to enable testing while compute features are worked on?

Ok to keep that for the moment then.

Yeah, we can remove this and just always use separate files. This did exist from a time where internal metal vertex/fragment shaders shared a single source.

Yeah this is true, though the shader libraries and function entry point names are required prior to PSO (backend) compilation which happens upon a draw, based on the current rendering state.

A shader will potentially have several PSO permutations based on the pipeline state at the time it is used. This compilation is deferred until draw time, as this state is generally unknown up-front.

We do not however require any of the original string-based sources however, so I'll place these inside an MTLShaderBuilder struct and free these after a shader is finalized.

Will also refactor debugging to use GPULogParser.

This is also required for PSO creation (backend shader compilation). When layered rendering is used, primitive type needs to be explicitly specified.

Usually, specifying primitive type explicitly means that each time a different primitive type is used with a given shader, a PSO permutation needs to be compiled. We can instead use "unspecified" which allows this to be any type for most cases, reducing the number of backend compilations.

This was perhaps over-engineering for cases where push constants (general shader uniforms) may be stored in several different UBOs, to allow optimisations such as only updating/re-binding ones which had been modified.

Left this in as UBO's had the Frequency field, which seemed like it could extend to uniforms in future. I.e. if certain classes of uniforms only get updated per scene, then having split UBOs could have the advantage of less data being encoded in the command buffer between similar draw calls.

However, as it stands, this code does not have an active use-case. So it can be removed and just replaced with a single data store. As all uniforms (push_constants) currently get written to the first data block only.
If the data size exceeds the limit of 4KB (which none do currently, but path added for robustness), then the push constant data is bound as a buffer, using a temporary scratch buffer allocation, rather than using Metal's setBytes call, which allows data to be encoded in the command stream.

Just as a note, the Frequency is only for binding frequency. It is not about update frequency which in almost all cases should be once per frame. If there is an update frequency flag it should be on UBO creation. But we haven't one for the moment.

So I agree that you can remove it.