Add support for saving HDR screenshots to Image.save_exr functions.#117800
Conversation
Isn't this going to break compatibility with uses such as saving floating point heightmaps? (i.e not colors). Those could contain legitimate negative values. |
Yep. Good to know about that, thanks for pointing out this use case! Since I'll probably be looking into issues regarding use of wide gamut EXR files for skies/GI, I'm curious if you know of other times people may be saving and loading EXR with negative values. My suspicion is that EXR import of wide gamut colour images might play poorly with Godot lighting/rendering. Presuming people have been using
Option 2 sounds pretty dumb. With option 1, I could update the documentation to say that clip_negative should always be true when saving a colour image. |
|
People using openexr for data would need to rescale to avoid losing data when -1 is clipped probably. I would expect some sort of non-default override for this. Edited: Adding a new function that does new things seems like the Reduz house-style. |
Image.save_exr functions.Image.save_exr functions.
|
Thanks again for the comment, @Zylann! Sometimes the urge to go back in time is too strong, but maintaining compatibility is paramount. I've reworked this PR to maintain existing behaviour and it is now ready (again) for review. |
fire
left a comment
There was a problem hiding this comment.
I have not done a technical review, but the amendment of allowing backwards compatibily with exr saving makes sense to me.
The default being the same behaviour as the current stable is correct, but I haven't checked it.
BlueCube3310
left a comment
There was a problem hiding this comment.
Code looks good to me
|
Thanks! |
What problem(s) does this PR solve?
save_exrdo not match HDR output #117795What is the rationale for the approach used in this PR?
Image.save_exrfunctions have been modified to add two new parameters:color_image: boolcolor_imagetotruewhen saving a color image, such as a screenshot. Negative values will be included whencolor_imageisfalse, which may be useful for saving raw floating point data such as a lightmap that includes negative light information.true, negative values are clipped to0.0to give correct colour rendering for screenshots.max_linear_valuemax_linear_valueifmax_linear_valueis not negative.Window.get_output_max_linear_valueformax_linear_value.Clipping negative values (
color_image == true)Internal to Godot, negative color values represent negative radiant energy for the purposes of lighting simulation. But for the purposes of CIE colorimetry, which is used for HDR image formats such as EXR, negative RGB values represent colors that are outside of the current RGB gamut and are positive RGB values in other color gamuts. For this reason, negative values should never be saved to EXR files that are intended to represent a color image such as a screenshot. This PR adds parameters to the existing
save_exrandsave_exr_to_bufferfunctions to allow saving of HDR screenshots.New
max_linear_valueparameterThe new
max_linear_valueparameter matchesWindow.get_output_max_linear_value(). This means that producing an EXR image that is identical to the rendering presented by Godot is as simple as:With these changes, it is now possible to save an image that is identical to the image presented by HDR output:
In the above renderings: the red, green, and blue rectangles shown on the bottom left corner contain negative values, such as those that may be created by negative lights. The colour sweep that comprises the majority of the render exceeds values of
1.0on the right side.Was generative AI (LLM AI) used to create a portion of this PR?
No.
Are there any parts of this PR that you are uncertain of or require special attention from reviewers?
SRC_FLOAT. I don't know how to do this, but if it's possible to test this code path it should be done before merging. Anyone know how to?