Closed AnyOldName3 closed 1 month ago
I'm happy to see us migrate to sRGB frame buffers, but some of these changes have left me scratching my head - I have added comments to the code changes that will hopefully highlight why I'm confused by the changes.
I think it would be useful to have a test example in vsgExamples to test the conversions between linear and sRGB.
Do we need to start putting the colour space info into the scene graph so when we save and then reload the application can decide whether the vertex and texture data needs converting?
The VSG doesn't presently record colour space in the scene graph, so existing .vsgb/.vsgt will implicitly be linearRGB. Does the vsg::Data::Properties struct now need a colour space hint? Adding to Data::Properties has to be carefully as we don't want to bump over a word boundary and have it's size increase.
I have merged this PR as branch:
https://github.com/vsg-dev/VulkanSceneGraph/tree/AnyOldName3-gamma-correctness-phase-one
I have also merged the vsgXchange and vsgExamples PR's as branches too. Then ran make build_ShaderSets to make sure all the built in shaders are updated too.
Do we need to start putting the colour space info into the scene graph so when we save and then reload the application can decide whether the vertex and texture data needs converting?
The VSG doesn't presently record colour space in the scene graph, so existing .vsgb/.vsgt will implicitly be linearRGB. Does the vsg::Data::Properties struct now need a colour space hint? Adding to Data::Properties has to be carefully as we don't want to bump over a word boundary and have it's size increase.
Maybe. The other potentially sensible option would be to assume everything that stores colours as floats in .vsgt
/.vsgb
is in linear space and make sure that apps that are knowingly doing something else have the power to explicitly say so by making the serialisers and deserialisers for those formats respect the new members of vsg::Options
.
It's pretty common for files that have a field to declare the colour space don't have the right value for sRGB and linear. For example, most DDS files in the wild with S3TC data within have the format declared as the UNORM/SNORM variant rather than the sRGB one, despite obviously holding an sRGB image, so applications typically ignore it. You can usually trust other colour spaces, though, as people typically don't go out of their way to mark something as Adobe RGB or Dolby Vision 8.4 if they don't know what colour space they're working in.
We're probably going to end up needing to guess what colour space data uses at some point, whether it's only when loading old files and when users specify colours in code, or every time we load anything. If we're sufficiently good at guessing (whether it's because other people make mistakes, or because we successfully minimise use of all but one approach except by people who know to provide hints), then we'd get better results by always guessing.
As discussed here https://github.com/vsg-dev/VulkanSceneGraph/discussions/1291, there are quite a few gamma-correctness issues and annoying-to-work-around obstacles in the VSG. Between this, and related PRs https://github.com/vsg-dev/vsgXchange/pull/204 and https://github.com/vsg-dev/vsgExamples/pull/323, things should be more manageable.
This PR:
UNORM
one to anSRGB
one, enabling automatic linear-to-sRGB conversions when data is written to the framebuffer. We're already telling the driver that we're writing sRGB data to the surface as we're giving it an sRGB colour space. This should avoid things like alpha blending issues caused by doing the sRGB conversion first. The vsgExamples PR includes an update to the PBR shader to remove the now-redundant conversion it was doing at the end.color.h
as they did the opposite of what they said. The call site I found seemed to be calling the one with the right implementation but wrong name, so I switched it over.vsg::Options
to control when vsgXchange converts colour spaces in loaded data.Things with a potential to break existing applications can be opted out of - you can still explicitly ask for a
UNORM
surface format and set the source and destination colour spaces viavsg::Options
.