-
Notifications
You must be signed in to change notification settings - Fork 42
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add option to assign SRGB format to diffuse and emissive textures #176
Conversation
This is more desirable than converting sRGB textures to linear in a fragment shader because the hardware performs filtering correctly.
@@ -296,6 +296,26 @@ SamplerData SceneConverter::convertTexture(const aiMaterial& material, aiTexture | |||
} | |||
} | |||
|
|||
bool sRGBTextures = false; | |||
options->getValue("sRGBTextures", sRGBTextures); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In incluce/vsg/core/Value.h there is a convenience template function vsg::value<>(..), this is used in several places in assimp.cpp to streamline the getting of values from objects with support for using a default when it's not available.
Are the formats of the images themselves not able to provide hints of sRGB? Or might assimp provide such a hint? I'm just wondering if we can avoid having to passing in options to enable use of sRGB. |
The colors are whatever are provided by the model, and in practice this is always sRGB for diffuse and and emissive colors, except for some very specialized cases. The option is for the benefit of the VSG. The current shaders expect sRGB values stored in UNORM texture formats and convert the colors from sRGB to liinear and back again. vsgCs, on the other hand, uploads its color textures in sRGB format and sets up an sRGB swap target, so its shaders don't do any conversion. I haven't found anything about whether or not the sRGB encoding of a texture is exposed through assimp. JPEG is always sRGB, PNG can be linear or sRGB depending on the gamma value in the header. I don't think that stbi does anything with it. Instead of this PR, we could always store color textures with sRGB format and change the default VSG shaders, but I thought was a bit radical. With an option, applications that want to do everything in linear space can use sRGB textures. |
src/assimp/assimp.cpp
Outdated
@@ -387,7 +416,7 @@ void SceneConverter::convert(const aiMaterial* material, vsg::DescriptorConfigur | |||
convertedMaterial.assignTexture("normalMap", samplerImage.data, samplerImage.sampler); | |||
} | |||
|
|||
if (samplerImage = convertTexture(*material, aiTextureType_UNKNOWN); samplerImage.data.valid()) | |||
if (samplerImage = convertTexture(*material, aiTextureType_METALNESS); samplerImage.data.valid()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have just looked into the aiTextureType_UNKNOWN oddity and it looks like its widely used for combined metal roughness texture.
I have tested GLTF models and found both the aiTextureType_UNKNOWN and aiTextureType_METALNESS return the same texture?!
Kinda feels like the assimp PBR model handling has been in flux and struggles to map all the different combinations that might happen with different file formats. I don't know if it's safe to just replace aiTextureType_UNKNOWN with aiTextureType_METALNESS, perhaps some models/formats only work with UNKNOWN and others aiTextureType_METALNESS, thought perhaps both are a bit broken.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'ts a bit hacky but have decided to add a check for aiTextureType_METALNESS but leave aiTextureType_UNKNOWN as a fallback:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this has evolved over time; around assimp 5.0 there was no METALNESS type, but that texture would be returned as UNKNOWN. That behavior has been left for backward compatibility.
I have merged this PR as a branch: https://github.com/vsg-dev/vsgXchange/tree/timoore-srgb-textures And standardized the setting of the options so we can now enable the sRGBTextures option on the command line with the commit: I have also looked at the standard_pbr.frag shader with:
When running: vsgviewer FlightHelmet/glTF/FlightHelmet.gltf --sRGBTextures --sRGB It looks OK when I leave the LINEARtoSRGB conversion in the frag shader, but it's too dark when I remove it. From what I've read it looked like a sRGB colour buffer would lead to an automatic conversion. So... I'm left wondering if I've misunderstood something along the line. |
The branch is now merged with vsgXchange master. |
The swapchain surface format needs to be set up with
I don't think you've changed vsgviewer to provide that option? |
Duh, you have in fact added that option. I'll try out your new vsgXchange code. |
I added support to toggle on the sRGB format to vsgviewer via a --sRGB command line option. This is checked in vsgExamples master and the 1.1.0 release I made this morning so it it can handle --sRGB --sRGBTextures and it'll be able to load .gltf etc. models with vsgXchange::assimp setting the image. The background colour is also automatically converted to sRGB by Window.cpp in VSG master/1.1.0. With these two options I'm finding that the models roughly look that same, but this is without any changes to the shaders which I'm surprised by. |
This is more desirable than converting sRGB textures to linear in a fragment shader because the hardware performs filtering correctly. It pairs nicely with setting up an sRGB swap target, so shaders operate only in linear shading space.