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

Fix Image.convert() overwriting custom mipmaps #74238

Merged
merged 1 commit into from
Jul 12, 2023

Conversation

bitsawer
Copy link
Member

@bitsawer bitsawer commented Mar 2, 2023

Fixes #66848, but this probably should not be merged without some discussion and maybe some more testing.

Currently, using custom Image mipmaps doesn't work reliably. You can write custom mipmaps in the Image data, but some renderer code paths with certain image formats can cause a call to Image.convert(). The way image format conversion is currently implemented converts only the largest main image mipmap and then generates downsampled mipmaps from that by calling Image.generate_mipmaps(). Naturally, this overwrites any custom mipmaps the image had before.

The fix that this PR implements is to simply convert all mipmap levels individually, this way conversion preserves all custom mipmaps.

This PR breaks compatibility to some degree as calling Image.convert() will no longer forcefully update all image mipmap levels which some code might have (incorrectly in my opinion) relied on, so this might cause some new bugs to surface. The image results can also differ by a very small amount, as the previous convert() averaged the main image down to smaller mipmap levels.

I also ran some benchmarks. Release editor build, clang+mingw on Windows 10, Intel i7-4790 @4.0GHz. The new convert() is usually from x1.5 to x2.6 faster when processing large non-byte images, especially floating point images where the memory bandwidth and CPU processing required by generate_mipmaps() pixel averaging is significant. When converting small images or fast byte formats the speed differences between versions are very small.

Old Image.convert() that uses generate_mipmaps():

benchmark_convert_slow_format (129x129): 5 passes, average 0.4564 ms (total=2.282, min=0.449, max=0.473)
benchmark_convert_slow_format (515x515): 5 passes, average 11.0614 ms (total=55.307, min=9.766, max=11.704)
benchmark_convert_slow_format (1024x1024): 5 passes, average 62.882 ms (total=314.41, min=60.522, max=65.581)
benchmark_convert_slow_format (2047x2047): 5 passes, average 299.9408 ms (total=1499.704, min=295.363, max=304.546)
benchmark_convert_slow_format (4096x4096): 5 passes, average 2012.0606 ms (total=10060.303, min=2003.167, max=2029.163)

benchmark_convert_fast_format (129x129): 5 passes, average 0.0298 ms (total=0.149, min=0.028, max=0.037)
benchmark_convert_fast_format (515x515): 5 passes, average 0.971 ms (total=4.855, min=0.884, max=1.1)
benchmark_convert_fast_format (1024x1024): 5 passes, average 3.6168 ms (total=18.084, min=3.53, max=3.736)
benchmark_convert_fast_format (2047x2047): 5 passes, average 15.7818 ms (total=78.909, min=15.687, max=15.911)
benchmark_convert_fast_format (4096x4096): 5 passes, average 62.6828 ms (total=313.414, min=58.137, max=68.799)

This PR, which converts each mip level individually:

benchmark_convert_slow_format (129x129): 5 passes, average 0.5898 ms (total=2.949, min=0.526, max=0.75)
benchmark_convert_slow_format (515x515): 5 passes, average 11.1374 ms (total=55.687, min=10.78, max=11.568)
benchmark_convert_slow_format (1024x1024): 5 passes, average 46.716 ms (total=233.58, min=45.05, max=49.615)
benchmark_convert_slow_format (2047x2047): 5 passes, average 187.4274 ms (total=937.137, min=180.513, max=192.586)
benchmark_convert_slow_format (4096x4096): 5 passes, average 772.2734 ms (total=3861.367, min=761.192, max=794.856)

benchmark_convert_fast_format (129x129): 5 passes, average 0.0304 ms (total=0.152, min=0.027, max=0.041)
benchmark_convert_fast_format (515x515): 5 passes, average 0.9296 ms (total=4.648, min=0.884, max=0.965)
benchmark_convert_fast_format (1024x1024): 5 passes, average 3.533 ms (total=17.665, min=3.404, max=3.576)
benchmark_convert_fast_format (2047x2047): 5 passes, average 14.4464 ms (total=72.232, min=14.388, max=14.556)
benchmark_convert_fast_format (4096x4096): 5 passes, average 63.7272 ms (total=318.636, min=57.488, max=69.749)

@bitsawer bitsawer requested review from a team as code owners March 2, 2023 15:09
@akien-mga akien-mga added this to the 4.1 milestone Mar 2, 2023
@akien-mga akien-mga modified the milestones: 4.1, 4.2 Jun 19, 2023
@akien-mga akien-mga requested a review from a team June 19, 2023 14:14
@akien-mga akien-mga changed the title Fix Image.convert() overwriting custom mipmaps Fix Image.convert() overwriting custom mipmaps Jun 19, 2023
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally with the updated MRP from the issue (rebased on top of master 7641936), it works.

Code looks good to me. Thanks for adding unit tests too 🙂

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

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

TIWAGOS, but makes sense.

@YuriSizov YuriSizov merged commit e88934c into godotengine:master Jul 12, 2023
@YuriSizov
Copy link
Contributor

Thanks!

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.

Textures created with ImageTexture.create_from_image are ignoring custom provided mipmaps
5 participants