Skip to content

Add Image.load_exr_from_buffer#101255

Merged
Repiteo merged 1 commit into
godotengine:masterfrom
metamuffin:load-exr-image
Nov 21, 2025
Merged

Add Image.load_exr_from_buffer#101255
Repiteo merged 1 commit into
godotengine:masterfrom
metamuffin:load-exr-image

Conversation

@metamuffin

@metamuffin metamuffin commented Jan 8, 2025

Copy link
Copy Markdown
Contributor

This closes godotengine/godot-proposals#676.

A tinyexr_always build option was added to enable TinyEXR even in template builds.
This PR was created in collaboration with @tpart120.

@metamuffin metamuffin requested review from a team as code owners January 8, 2025 00:58

@fire fire left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The change looks ok.

  1. The listed issue/pr doesn't seem to match this one
  2. github actions integration testing needs to pass
  3. Do you have a project / screenshots or a video of this working?

@fire

fire commented Jan 8, 2025

Copy link
Copy Markdown
Member

There is a style issue you can fix. See https://docs.godotengine.org/en/stable/contributing/workflow/pr_workflow.html

Run:

pip3 install pre-commit
# Go to Godot Engine folder
pre-commit run -a

Not sure where the proper directions are for this.

@metamuffin

Copy link
Copy Markdown
Contributor Author

I could provide a screenshot of it working, although there is not much to see. We tested this with the unit test and a scene where we load an EXR file and set it in a TextureRect for display.

Comment thread modules/tinyexr/config.py Outdated
Comment thread tests/core/io/test_image.h Outdated
@fire

fire commented Jan 8, 2025

Copy link
Copy Markdown
Member

Documentation changes required.

Try godot --doctool

diff --git a/doc/classes/Image.xml b/doc/classes/Image.xml
index 97c4bba..138abe0 100644
--- a/doc/classes/Image.xml
+++ b/doc/classes/Image.xml
@@ -346,13 +346,6 @@
 				[b]Note:[/b] This method is only available in engine builds with the BMP module enabled. By default, the BMP module is enabled, but it can be disabled at build-time using the [code]module_bmp_enabled=no[/code] SCons option.
 			</description>
 		</method>
-		<method name="load_from_file" qualifiers="static">
-			<return type="Image" />
-			<param index="0" name="path" type="String" />
-			<description>
-				Creates a new [Image] and loads data from the specified file.
-			</description>
-		</method>
 		<method name="load_exr_from_buffer">
 			<return type="int" enum="Error" />
 			<param index="0" name="buffer" type="PackedByteArray" />
@@ -361,6 +354,13 @@
 				Note: The TinyEXR module is disabled in non-editor builds, which means load_exr_from_buffer will return an error in exported builds unless it is explicitly enabled at build-time using [code]tinyexr_always=yes[/code] SCons option.
 			</description>
 		</method>
+		<method name="load_from_file" qualifiers="static">
+			<return type="Image" />
+			<param index="0" name="path" type="String" />
+			<description>
+				Creates a new [Image] and loads data from the specified file.
+			</description>
+		</method>
 		<method name="load_jpg_from_buffer">
 			<return type="int" enum="Error" />
 			<param index="0" name="buffer" type="PackedByteArray" />

@metamuffin

Copy link
Copy Markdown
Contributor Author

Will those commits be squashed on merge?

@AThousandShips

Copy link
Copy Markdown
Member

No, once this is approved you'll need to squash your commits, but it's not necessary now

@metamuffin metamuffin requested a review from fire January 10, 2025 14:47
@Calinou

Calinou commented Jan 10, 2025

Copy link
Copy Markdown
Member

A tinyexr_always build option was added to enable TinyEXR even in template builds.

This is also implemented by #73003, although it uses the tinyexr_export_templates=yes SCons option instead (which I think is more explicit). Your implementation of the option seems cleaner still, so I'm in favor of this PR over #73003.

I think we should rename the option in this PR to tinyexr_export_templates=yes, or just make use of module_tinyexr_enabled=yes being explicitly defined like #94965 did for runtime lightmap baking.

Comment thread tests/core/io/test_image.h Outdated
Comment thread doc/classes/Image.xml Outdated
@akien-mga

Copy link
Copy Markdown
Member

What's the size impact on optimized export templates to enable tinyexr at runtime? We need to evaluate the cost to see if we have to bother adding a SCons option and disabling this by default (which means hardly anybody will use it).

@metamuffin

metamuffin commented Jan 10, 2025

Copy link
Copy Markdown
Contributor Author

The other PR (#73003) measured a size increase of 100KB for export templates. I assume that value did not change drasticly since then.

@fire

fire commented Jan 10, 2025

Copy link
Copy Markdown
Member

I encourage enabling tinyexr by default.

@akien-mga akien-mga changed the title Add Image.load_exr_from_buffer Add Image.load_exr_from_buffer Jan 12, 2025
@metamuffin

Copy link
Copy Markdown
Contributor Author

Should I commit changes to enable tinyexr by default within this PR or create another one?

@metamuffin metamuffin requested review from a team as code owners January 15, 2025 16:23
@AThousandShips AThousandShips requested review from a team and removed request for a team January 15, 2025 16:29
@metamuffin

Copy link
Copy Markdown
Contributor Author

It's been quite some time... I just rebased my changes to work with version 4.5.
What is left to do for the PR to be accepted? Is it only the missing review from AThousandShips?

Comment thread doc/classes/Image.xml Outdated
<return type="int" enum="Error" />
<param index="0" name="buffer" type="PackedByteArray" />
<description>
Loads an image from the binary contents of a OpenEXR file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Loads an image from the binary contents of a OpenEXR file.
Loads an image from the binary contents of an OpenEXR file.

@tpart120

tpart120 commented Nov 8, 2025

Copy link
Copy Markdown

The requested change was implemented. Is there something else missing for this to be merged?

@Repiteo

Repiteo commented Nov 20, 2025

Copy link
Copy Markdown
Contributor

Could you squash your commits? See our pull request guidelines for more information

@metamuffin

metamuffin commented Nov 20, 2025

Copy link
Copy Markdown
Contributor Author

Rebased and squashed.

@Repiteo Repiteo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job! Sorry to keep you here a bit longer, but AThousandShip's suggestion appears to have been lost in the squash/rebase process. Re-apply that, and this'll be good to go

@metamuffin

Copy link
Copy Markdown
Contributor Author

Oops. Good you noticed that. Its fixed now.

@Repiteo

Repiteo commented Nov 21, 2025

Copy link
Copy Markdown
Contributor

Thanks! Congratulations on your first merged contribution! 🎉

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.

Expose Image.load_{format}_from_buffer() for OpenEXR

8 participants