Rewrite texture handling #1870

colinator27 · 2024-08-14T20:40:48Z

Description

Rewrites all of the core texture code to use a new immutable GMImage class, which can be one of four texture types: PNG, raw BGRA pixel data, QOI, or BZip2 + QOI. This class is used as a replacement to the existing byte[] containing compressed PNG data always.

The major benefit of this is that load/save times on games using non-PNG textures are now much faster (some games are at least 5 seconds faster on either end, on my machine). QOI and/or BZ2 textures are now decoded/decompressed on the fly, rather than during the (de)serialization process.

This also mostly replaces System.Drawing.Common calls with ones to ImageMagick instead. In UndertaleModLib, it's used only for PNG encoding/decoding, but I also rewrote a tiny bit of code on the UI side. This should mean the library code itself is cross-platform. Additionally, I performed some tests and found that exporting PNGs with the new UI code preserves colors with 0 alpha.

Caveats

This breaks pretty much every script that interacts with texture data, but that will be resolved in an upcoming PR.

There's also one further memory optimization that can be made with BZ2 + QOI textures, if we can decompress the first 8 bytes of the BZ2 data only. Essentially, it involves finding the size of the inner QOI data, which can be used to initialize the capacity of the buffer used to decompress the BZ2 data later (when called to do so, on the fly). The performance impact is probably not extreme, though - it would prevent a few reallocations at most. It's something maybe worth revisiting in the future.

Notes

Tested on a number of games, including ones with PNG textures, BZ2 + QOI textures, and pre/post GameMaker 2022.5. I haven't yet tested QOI-only games, but the code should theoretically work with them. Files have loaded/saved byte-for-byte, besides a bug in the FONT chunk that's fixed by the serialization cleanup PR (#1864).

I tested collision mask saving/loading, the room editor, the sprite editor, the embedded texture editor (saving/loading), and the texture page item editor (saving/loading).

On the UI front, instances of BitmapSource are cached for as long as the garbage collector doesn't get rid of them from memory, which can prevent unnecessary image decodes.

Initial work, most scripts involving textures will no longer work

github-actions · 2024-08-14T20:44:21Z

Download the artifacts for this pull request here:

GUI:

CLI:

UndertaleModCli/Program.cs

UndertaleModLib/Models/UndertaleEmbeddedTexture.cs

UndertaleModLib/Models/UndertaleTexturePageItem.cs

UndertaleModTool/Editors/UndertaleEmbeddedTextureEditor.xaml.cs

UndertaleModTool/MainWindow.xaml.cs

UndertaleModTool/UndertaleModTool.csproj

UndertaleModLib/Util/TextureWorker.cs

Miepee · 2024-08-16T18:58:21Z

UndertaleModLib/Models/UndertaleEmbeddedTexture.cs

-    private static object _textureLoadLock = new();
+    // 1x1 blank image
+    private static readonly TexData _placeholderTexture = new() { Image = new GMImage(1, 1) };
+    private static readonly object _textureLoadLock = new();


afaik MS recommended to use a System.Threading.Lock for locks. It's only available in c# 13+, cant remember what version utmt is in right now. Probably good idea to a make mental note for this.

Yeah I don't think it's new enough yet, that's a .NET 9 thing.

Miepee · 2024-08-16T18:59:20Z

UndertaleModLib/Models/UndertaleEmbeddedTexture.cs

-    };
-    private static object _textureLoadLock = new();
+    // 1x1 blank image
+    private static readonly TexData _placeholderTexture = new() { Image = new GMImage(1, 1) };


Any specific reason this has to be 1x1 instead of 0x0?

Not really, but this is just what shows in lieu of an unloaded (or failed to load) texture

Miepee · 2024-08-16T19:03:01Z

UndertaleModLib/Models/UndertaleEmbeddedTexture.cs

+        public bool FormatBZ2 => _image.Format is GMImage.ImageFormat.Bz2Qoi;
+
+        /// <summary>
+        /// If located within a data file, this is the maximum end position of the image data (or start of the next texture blob).


what exactly is "maximum end position" supposed to mean? That makes it sound like a minimum end position or an average end position exists.
Also if -1 means "this is an external texture" this should be noted in the documentation.

So the "maximum end position" in this context is actually an upper bound on the end position of the image data. Inside a data file, it's either set to the start address of the next texture blob, or the end of the TXTR chunk if none exists. When loading textures, this is then narrowed down to the actual end address (but this field isn't updated).

I think I'm going to actually make this a private field, and just make a setter method for this, since this isn't meant to be relied upon for anything other than deserialization.

I'm still confused by the name. If this is the end position of the image, why not just call it "endOfImagePosition"?

I'm going to update this with a newer comment in just a moment, hopefully it'll be more clear - but this is the upper bound on the image end position, not the actual image end position.

UndertaleModLib/Models/UndertaleEmbeddedTexture.cs

UndertaleModLib/Models/UndertaleTexturePageItem.cs

UndertaleModTool/Converters/UndertaleCachedImageLoader.cs

UndertaleModTool/Editors/UndertaleTexturePageItemEditor.xaml

UndertaleModTool/Editors/UndertaleEmbeddedTextureEditor.xaml

Miepee · 2024-08-17T09:33:03Z

UndertaleModTool/MainWindow.xaml.cs

@@ -101,6 +102,9 @@ public object Selected

        public List<Tab> ClosedTabsHistory { get; } = new();

+        private List<(GMImage, WeakReference<BitmapSource>)> _bitmapSourceLookup { get; } = new();
+        private object _bitmapSourceLookupLock = new();


Miepee · 2024-08-17T09:33:54Z

UndertaleModTool/MainWindow.xaml.cs

@@ -101,6 +102,9 @@ public object Selected

        public List<Tab> ClosedTabsHistory { get; } = new();

+        private List<(GMImage, WeakReference<BitmapSource>)> _bitmapSourceLookup { get; } = new();


is there a reason why this needs to be a list instead of a dictionary?

So this was originally a dictionary, but I changed it to a list because every lookup does a full sweep on the weak references anyway. Not sure if there's any better way to do it, though (we could possibly defer the sweep operation).

wdym with "every lookup does a full sweep on the references"?
The way I see it is that currently, it always iterates over the full list when you want to get just one item, cause it always invalidated everything.

With a dictionary, you would only check the image asked for, and only invalidate that.

Yeah, so in this case it's weird, because we also want to clear the "dictionary" of the weak references that are no longer valid (because the GC freed their memory). If you don't do this, it could gradually leak memory, especially if the images are getting replaced by new ones. If we move this clearing process somewhere else, we don't need to do a linear lookup here, but then we have to find another good place to do that "sweep" operation anyway. As it stands, though, I don't think we ever have more than a small handful of textures loaded at once.

I see, then probably no harm in keeping it as a list and keeping the linearity.

colinator27

Left some responses, but I'll get to updating the code soon to resolve most of the comments.

colinator27 · 2024-08-17T12:58:20Z

UndertaleModLib/Models/UndertaleEmbeddedTexture.cs

-    };
-    private static object _textureLoadLock = new();
+    // 1x1 blank image
+    private static readonly TexData _placeholderTexture = new() { Image = new GMImage(1, 1) };


Not really, but this is just what shows in lieu of an unloaded (or failed to load) texture

colinator27 · 2024-08-17T12:59:02Z

UndertaleModLib/Models/UndertaleEmbeddedTexture.cs

-    private static object _textureLoadLock = new();
+    // 1x1 blank image
+    private static readonly TexData _placeholderTexture = new() { Image = new GMImage(1, 1) };
+    private static readonly object _textureLoadLock = new();


Yeah I don't think it's new enough yet, that's a .NET 9 thing.

colinator27 · 2024-08-17T13:03:34Z

UndertaleModLib/Models/UndertaleEmbeddedTexture.cs

+        public bool FormatBZ2 => _image.Format is GMImage.ImageFormat.Bz2Qoi;
+
+        /// <summary>
+        /// If located within a data file, this is the maximum end position of the image data (or start of the next texture blob).


So the "maximum end position" in this context is actually an upper bound on the end position of the image data. Inside a data file, it's either set to the start address of the next texture blob, or the end of the TXTR chunk if none exists. When loading textures, this is then narrowed down to the actual end address (but this field isn't updated).

I think I'm going to actually make this a private field, and just make a setter method for this, since this isn't meant to be relied upon for anything other than deserialization.

UndertaleModLib/Models/UndertaleEmbeddedTexture.cs

UndertaleModLib/Models/UndertaleTexturePageItem.cs

UndertaleModLib/Util/TextureWorker.cs

colinator27 · 2024-08-17T13:38:23Z

UndertaleModLib/Util/TextureWorker.cs

-        // Grabbed from https://stackoverflow.com/questions/3801275/how-to-convert-image-to-byte-array/16576471#16576471
-        public static Bitmap GetImageFromByteArray(byte[] byteArray)
+        /// <summary>
+        /// Performs a resize of the given image, if required, using bilinear interpolation. Always returns a new image.


Not sure, but for now it's probably fine to leave this as-is. We can add optional parameters later to configure the interpolation, if we want.

UndertaleModLib/Util/TextureWorker.cs

UndertaleModTool/Converters/UndertaleCachedImageLoader.cs

colinator27 · 2024-08-17T13:51:22Z

UndertaleModTool/MainWindow.xaml.cs

@@ -101,6 +102,9 @@ public object Selected

        public List<Tab> ClosedTabsHistory { get; } = new();

+        private List<(GMImage, WeakReference<BitmapSource>)> _bitmapSourceLookup { get; } = new();


So this was originally a dictionary, but I changed it to a list because every lookup does a full sweep on the weak references anyway. Not sure if there's any better way to do it, though (we could possibly defer the sweep operation).

UndertaleModLib/UndertaleChunks.cs

UndertaleModLib/Util/GMImage.cs

UndertaleModLib/Util/TextureWorker.cs

UndertaleModTool/Editors/UndertaleTexturePageItemEditor.xaml.cs

UndertaleModLib/Util/GMImage.cs

Miepee

LGTM, I'll do a quick test on linux CLI soon:tm: to check whether it works and hten merge.

Miepee · 2024-08-22T21:37:12Z

CLI was able to dump all embedded textures, so i'll call this a success!
Gonna be merging this now, fixing scripts is next on the table then :)

addressed

colinator27 added 5 commits August 13, 2024 23:04

Rewrite texture handling, update to .NET 8

74d3ccf

Initial work, most scripts involving textures will no longer work

Go back to .NET 6 for now

0b4c1c9

Further optimize BZip2 textures

b269fa2

Add two missing comments

ddea157

Some cleanup

c74c24d

colinator27 requested review from Miepee and VladiStep August 14, 2024 20:40

VladiStep previously requested changes Aug 15, 2024

View reviewed changes

colinator27 added 3 commits August 15, 2024 11:11

Address reviews

70b6ab2

Fix spacing

ce94847

Undo Microsoft.Windows.Compatibility bump for now

ab6c198

colinator27 mentioned this pull request Aug 15, 2024

Refactor from System.Drawing.Common to Skiasharp. #1538

Closed

5 tasks

colinator27 added 3 commits August 15, 2024 15:45

Cleanup and memory leak fix

221f201

Fix mask import/export on 2024.6+, and cleanup

6838e69

Fix minor WPF error message

03dfdd4

VladiStep reviewed Aug 15, 2024

View reviewed changes

UndertaleModLib/Util/TextureWorker.cs Outdated Show resolved Hide resolved

colinator27 added 2 commits August 15, 2024 18:26

Fix typo

c020a94

Switch a remaining RGBA to BGRA

0644108

Miepee reviewed Aug 17, 2024

View reviewed changes

colinator27 commented Aug 17, 2024

View reviewed changes

Address reviews, and some small changes

90834d8

Miepee reviewed Aug 18, 2024

View reviewed changes

UndertaleModLib/Util/GMImage.cs Outdated Show resolved Hide resolved

UndertaleModLib/Util/GMImage.cs Outdated Show resolved Hide resolved

colinator27 added 2 commits August 18, 2024 10:27

Address reviews

e8e036f

Merge branch 'master' into texture-handling-rewrite

bcdaf41

Miepee approved these changes Aug 22, 2024

View reviewed changes

Miepee merged commit 405f10f into master Aug 22, 2024
5 checks passed

colinator27 mentioned this pull request Aug 26, 2024

Compatibility upgrade of scripts involving textures #1890

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewrite texture handling #1870

Rewrite texture handling #1870

colinator27 commented Aug 14, 2024

github-actions bot commented Aug 14, 2024 •

edited

Loading

Miepee Aug 16, 2024

colinator27 Aug 17, 2024

Miepee Aug 16, 2024

colinator27 Aug 17, 2024

Miepee Aug 16, 2024

colinator27 Aug 17, 2024

Miepee Aug 18, 2024

colinator27 Aug 18, 2024

Miepee Aug 17, 2024

Miepee Aug 17, 2024

colinator27 Aug 17, 2024

Miepee Aug 18, 2024

colinator27 Aug 18, 2024

Miepee Aug 22, 2024

colinator27 left a comment

colinator27 Aug 17, 2024

colinator27 Aug 17, 2024

colinator27 Aug 17, 2024

colinator27 Aug 17, 2024

colinator27 Aug 17, 2024

Miepee left a comment

Miepee commented Aug 22, 2024

		@@ -101,6 +102,9 @@ public object Selected

		public List<Tab> ClosedTabsHistory { get; } = new();

		private List<(GMImage, WeakReference<BitmapSource>)> _bitmapSourceLookup { get; } = new();

Rewrite texture handling #1870

Rewrite texture handling #1870

Conversation

colinator27 commented Aug 14, 2024

Description

Caveats

Notes

github-actions bot commented Aug 14, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

colinator27 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miepee left a comment

Choose a reason for hiding this comment

Miepee commented Aug 22, 2024

github-actions bot commented Aug 14, 2024 •

edited

Loading