Refactor backend getShaderLanguage functions. #9204
Conversation
| /* | ||
| * Sets a preferred shader language for Filament to use. | ||
| * | ||
| * The Metal backend supports two shader languages: MSL (Metal Shading Language) and | ||
| * METAL_LIBRARY (precompiled .metallib). This option controls which shader language is | ||
| * used when materials contain both. | ||
| * | ||
| * By default, when preferredShaderLanguage is unset, Filament will prefer METAL_LIBRARY | ||
| * shaders if present within a material, falling back to MSL. Setting | ||
| * preferredShaderLanguage to ShaderLanguage::MSL will instead instruct Filament to check | ||
| * for the presence of MSL in a material first, falling back to METAL_LIBRARY if MSL is not | ||
| * present. | ||
| * | ||
| * When using a non-Metal backend, setting this has no effect. | ||
| */ |
There was a problem hiding this comment.
I think this comment should still go into Engine::Config
There was a problem hiding this comment.
can we still include this from the Driver.h files? I think that's the part I wasnt sure about or will it introduce some weird dependency?
There was a problem hiding this comment.
There are other enums in Engine::Config like this one. So it should be ok.
There was a problem hiding this comment.
I followed mathias suggestion and just did a conversion between both types
| enum class PreferredShaderLanguage { | ||
| DEFAULT = 0, | ||
| MSL = 1, | ||
| METAL_LIBRARY = 2, | ||
| }; | ||
|
|
There was a problem hiding this comment.
I don't think we need this, just use ShaderLanguage.
There was a problem hiding this comment.
then should I add a convert function from the Config::ShaderLanguage to the backend one?
| // interface with MSL. | ||
| virtual ShaderLanguage getShaderLanguage() const noexcept = 0; | ||
| virtual utils::FixedCapacityVector<ShaderLanguage> getShaderLanguage( | ||
| PreferredShaderLanguage preferredLanguage) const noexcept = 0; |
There was a problem hiding this comment.
use ShaderLanguage here, and document that's only hint. If the backend supports the asked prefered language, it must appear first in the list.
| ShaderLanguage::ESSL1, | ||
| ShaderLanguage::ESSL3, | ||
| ShaderLanguage::SPIRV, | ||
| ShaderLanguage::MSL, | ||
| ShaderLanguage::METAL_LIBRARY, | ||
| ShaderLanguage::WGSL, |
There was a problem hiding this comment.
I think ESSL3 should appear first to keep the old behavior.
| size_t getMaxShadowMapCount() const noexcept; | ||
|
|
||
| // Return a vector of shader languages, in order of preference. | ||
| utils::FixedCapacityVector<backend::ShaderLanguage> getShaderLanguage() const noexcept { |
There was a problem hiding this comment.
Since getShaderLanguage returns a vector, I think we should rename it and the new Driver method to getShaderLanguages (plural).
| /* | ||
| * Sets a preferred shader language for Filament to use. | ||
| * | ||
| * The Metal backend supports two shader languages: MSL (Metal Shading Language) and | ||
| * METAL_LIBRARY (precompiled .metallib). This option controls which shader language is | ||
| * used when materials contain both. | ||
| * | ||
| * By default, when preferredShaderLanguage is unset, Filament will prefer METAL_LIBRARY | ||
| * shaders if present within a material, falling back to MSL. Setting | ||
| * preferredShaderLanguage to ShaderLanguage::MSL will instead instruct Filament to check | ||
| * for the presence of MSL in a material first, falling back to METAL_LIBRARY if MSL is not | ||
| * present. | ||
| * | ||
| * When using a non-Metal backend, setting this has no effect. | ||
| */ | ||
| enum class ShaderLanguage { | ||
| DEFAULT = 0, | ||
| MSL = 1, | ||
| METAL_LIBRARY = 2, | ||
| }; |
There was a problem hiding this comment.
why do we need this at all? why not just use backend::ShaderLanguage ?
There was a problem hiding this comment.
This is what we currently have right now, I reverted back what I changed. This is comparings against my other commits of this PR
There was a problem hiding this comment.
are we allowed to change it since its a public api?
rafadevai
force-pushed
the
shader_languages
branch
from
acb8505 to
14d78d6
Compare
rafadevai
force-pushed
the
shader_languages
branch
2 times, most recently
from
6eb62bf to
4369356
Compare
Allow returning a list of supported shader languages instead of only one. Notify the backend which is the preferred language. In the case of metal, the preferred language will be the first element in the list. This change will allow a backend to support multiple languages as needed, in the case of the noop driver, it will support a material with any shader languages.
Address comment on keeping the Config::ShaderLanguage and passing down a backend::ShaderLanguage as a parameter.
rafadevai
force-pushed
the
shader_languages
branch
from
4369356 to
f2702c2
Compare
Allow returning a list of supported shader languages instead of only one.
Notify the backend which is the preferred language. In the case of metal, the preferred language will be the first element in the list.
This change will allow a backend to support multiple languages as needed, in the case of the noop driver, it will support a material with any shader languages.