Skip to content
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 convert function srgba to linear in example #433

Closed
wants to merge 8 commits into from

Conversation

TWITTM
Copy link
Contributor

@TWITTM TWITTM commented Jun 12, 2020

Ospray expects color to be in linear space, the ColorPicker passes the colors as srga.
So this just converts the color spaces.
I also changed the documentation so that it clarify that colors are expected in linear space.

See behavior before change :
ColorPicker uses (50%,50%,50%) => gimp(on the right side) ( 74%,74%,74%)
osp_ray_background

…ing to ospray

clearify that colors are expected in linear colorspace.
@TWITTM TWITTM changed the base branch from master to devel June 15, 2020 20:50
TWITTM and others added 3 commits July 2, 2020 10:56
…ich doe the handles the srgb_to_linear conversion the simelear to Texture2D and FrameBuffer
remove whitespaces
@johguenther
Copy link
Contributor

Right, color parameters of type float are in linear space. We may extend that in future to accept also uint32 colors (which then could be in sRGBA or in linear RGBA space). The question is, what color space should the color picker widget represent. We can probably agree that the color displayed in the color picker for the background color should then also match the color of the background. However, this can also be achieved by displaying the color widget in sRGB space (instead of converting the color when passing to OSPRay).

@TWITTM
Copy link
Contributor Author

TWITTM commented Oct 26, 2020

The main motivation, of this pull request is that beginners are often not aware about color spaces.
So hiding the conversation in the color-picker, might not solve this concern.
However if you plan to introduce another method to accept sRGA values, you can close this request.
You might think about changing the documentation to clearify that parameters are expected in linear space.
Which is a huge trap, and not easy to solve, when someone already stepped in.

@johguenther
Copy link
Contributor

Until we have a clear idea how to support setting sRGB color on OSPRay side (which may involve merging FB color channel and texture format enums into OSPDataType) I like to focus changes to the app side (i.e. ospExamples). There is already some discussion of sRGB and linear color space in the documentation (in the textures and the material section) and because color parameters are in float (vec3f) I'd hoped it is clear that this is in linear space.

Nevertheless, I agree that if the color checker displays "byte" values (integers in 0-255) the expectation by users is that those values should be consistent to e.g. Gimp's color checker, which is similar to sRGB textures values. Which means color from the color checker should be converted to linear. See also ocornut/imgui#578.

@TWITTM
Copy link
Contributor Author

TWITTM commented Dec 18, 2020

"float (vec3f) I'd hoped it is clear that this is in linear space."

As many formats define the option for per vertex colors, which are also often defined as floats, but in many cases has to be interpreted as sRGB (for example GLTF2 ).

I already had a hard time to figure out in which file-fomat which color format is defined. But even this knowledge didn't help,
as the most common viewers do interpret this formats wrong. And so the user expectation did not fit.

In my opinion you, changing the documentation would be a first step.

@johguenther
Copy link
Contributor

I changed documentation based on your suggestion and added an sRGB conversion in ospExamples to arrive at

ospExample_sRGB

Maybe better handling can be achieved once ocornut/imgui#2943 is merged.

@johguenther
Copy link
Contributor

see 4d32a46

@johguenther johguenther closed this Feb 8, 2021
@TWITTM
Copy link
Contributor Author

TWITTM commented Feb 8, 2021

@johguenther Perfect, thanx.

@TWITTM TWITTM deleted the srgba_to_linear branch February 8, 2021 19:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants