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

Tone-map HDR colours for ColorButton #1674

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

troughton
Copy link

When an HDR editing mode is being used and the colour value is high, the swatch colour appears white. Implement simple tonemapping (just component / (1 + component)) to bring the colours back into a usable range.

When an HDR editing mode is being used and the colour value is high, the swatch colour appears white. Implement simple tonemapping to bring the colours back into a usable range.
@ocornut
Copy link
Owner

ocornut commented Mar 20, 2018

Thank you Thomas, will look into this.

I was wondering if we should aim to use a gradient (with the tone mapped color on the edge and white around the center) to signify this.

Did you get into other issues with using unbounded color values?

Also linking to old notes in the color picker thread:
#346 (comment)

@troughton
Copy link
Author

Hi Omar,

Having some sort of 'HDR' signifier could be useful, but I'd worry that you'd make it more difficult to read the actual colour; for example, with a gradient to white it could desaturate the original colour.

It might be useful to allow the user to input both an input value and the tone-mapped representation of that value for display; that way people can input HDR values and have a preview of how they'll be tone-mapped in their use case.

And yes, there are other issues with unbounded colour values which the ideas in the other thread would help with. The main problem is we generally select a colour by choosing a hue/saturation and then dragging the value up, but if we want to change the hue/saturation again later we can't - I think that's the RGB-HSV round-trip issue.

By the way, thank you in general for ImGui - it's a great tool!

@pdoane
Copy link

pdoane commented Mar 21, 2018

With HDR displays becoming more common, ideally we could display the actual color somehow. I think the 2nd suggestion is a better intermediate solution though (passing two colors instead of one).

@xelatihy
Copy link

I too think we should pass two colors. This means that the user can choose which tone mapper to use.

@ocornut
Copy link
Owner

ocornut commented Sep 25, 2018

One minor issue is that the functions flow is generally read input > process edit > render so a tone mapped representation passed manually will lag by a frame compare to the non-tone-mapped value.

Not a big issue. Or we could provide an io callback for user specifying a tone-mapping function, which would be convenient but detrimental to software which may want to represent varied tone-mapping simultaneously.

I was going to say, the tricky part is to have to introduce new function signature - but since the ColorEdit, ColorPicker functions take float* we could enable the feature with an additional ImGuiColorEditFlags which will read 3 more floats in the stream.

It won't be super easy to use for quick inline call but not a problem for data-driven property serializer.

@troughton
Copy link
Author

Alternatively, if you wanted maximum flexibility, you could have the callback be passed in to a ColorEditToneMapped function along with a void* userData parameter – it does increase the API surface area but is probably the right solution for those use cases. A general tone mapping function might want to take context into account - e.g. current exposure - so having it only as a function pointer is probably limiting.

Manually passing a mapped value doesn't work well for the colour picker, since there you'd ideally want to display a range of tone-mapped values rather than just the currently selected colour.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants