Add FlxGraphic.freeImageBuffer (dump)

[ACrazyTown]

Adds the feature described in #3034. I think it might also be possible to add undumping, but I'd have to play around and confirm.

One concern I have is about the naming, imo I feel as if dump isn't very descriptive and there's also deprecated canBeDumped and undump() fields that aren't related to this.

Also, the original issue mentions:

Depending on the type of graphic I think this should probably even be the default in many cases, requiring the graphic to be explicitly "undumped" to be drawn on.

Maybe there could be a compile flag (FLX_GRAPHIC_DEFAULT_DUMP?) or a variable that decides whether the graphics should be automatically dumped

[Geokureli]

If you're not already aware, the previous dump/undump fields were just removed in flixel 6.0.0, here: #3059

I still don't fully understand the implications of graphic dumping, but the idea was to remove the previous fields that did nothing, and then add them back with actual functionality later on, so people know to remove them from their project to avoid unintended behavior (not that anyone should have been using them)

So this is gonna be a post-6.0.0 feature, but pointing it at release6 is the right move

[ACrazyTown]

If you're not already aware, the previous dump/undump fields were just removed in flixel 6.0.0, here: #3059

I was waiting for that to get merged to avoid any potential conflicts

I still don't fully understand the implications of graphic dumping, but the idea was to remove the previous fields that did nothing, and then add them back with actual functionality later on, so people know to remove them from their project to avoid unintended behavior (not that anyone should have been using them)

In short, OpenFL BitmapDatas hold references to both an image buffer in RAM and a texture on the GPU. When rendering, the texture is used. When drawing on the bitmap or modifying its pixels, the image buffer is used and the rendering texture is updated. If you don't need to modify the pixels (most cases) you can get rid of the image buffer from RAM and just keep the GPU texture. Since decompressed images in RAM can be quite large in size (especially if dealing with bigger spritesheets, like FNF does), this lets us free a lot of memory and the only drawback is losing the ability to modify the bitmap's pixels.

I think a lot of fuss could be avoided simply by using names other than dump() and undump()

[Geokureli]

this all makes sense to me, thanks for the info

[ACrazyTown]

Was this meant to be closed?

[Geokureli]

This was closed automatically when I merged and deleted the release 6 branch. I didn't know that would happen, try pointing it at dev

[ACrazyTown]

FYI #2909 and #2891 were also closed for the same reason it seems. I'll try to fix the messed up commits

[Geokureli]

random thought: rather than loading a graphic, and disposing it. what if we just had sprite.loadVRamGraphic that did this for you, and disallowed the unique arg and any access to the underlying bitmap somehow?

[ACrazyTown]

random thought: rather than loading a graphic, and disposing it. what if we just had sprite.loadVRamGraphic that did this for you

I think that'd be nice to have more fine grain control over what gets dumped and what doesn't. Though while this would work with sprites, what about frame collections for example? They manage graphics by themselves and that's often where you would even come across a graphic big enough to decrease a significant amount of memory.

and disallowed the unique arg and any access to the underlying bitmap somehow?

Not sure why you'd hide the bitmap, some functions like BitmapData.draw() can still work even if it's a texture only bitmap.

[ACrazyTown]

Although I will note that I'm not sure if I'm too happy with how the FLX_FREE_IMAGE_BUFFER flag works. From my brief testing I noticed it breaks transitions, so likely that and any other flixel class that requires the buffer would have to call FlxGraphic.refresh(). I feel like that would be tedious to manage

Maybe adding a new optional arg to FlxG.bitmap.add() or whatever... would that be a breaking change?

[Geokureli]

I think that'd be nice to have more fine grain control over what gets dumped and what doesn't. Though while this would work with sprites, what about frame collections for example? They manage graphics by themselves and that's often where you would even come across a graphic big enough to decrease a significant amount of memory.

Atlases and frame collections are a good example of places where it makes sense that bitmap editing features should be "opt-in"

Not sure why you'd hide the bitmap, some functions like BitmapData.draw() can still work even if it's a texture only bitmap.

Didn't think about that, we could put an abstract over Bitmap and disable certain methods, but that could get pretty hairy

Although I will note that I'm not sure if I'm too happy with how the FLX_FREE_IMAGE_BUFFER flag works. From my brief testing I noticed it breaks transitions, so likely that and any other flixel class that requires the buffer would have to call FlxGraphic.refresh(). I feel like that would be tedious to manage

Not a fan of that, tbh

Maybe adding a new optional arg to FlxG.bitmap.add() or whatever... would that be a breaking change?

In cases where someone passed the method around, yes, but I'm guilty of doing this in minor versions. the safest way to to add another method

[ACrazyTown]

Didn't think about that, we could put an abstract over Bitmap and disable certain methods, but that could get pretty hairy

I still don't understand why you'd hide it in the first place. OpenFL will already prevent you from using drawing methods on hardware bitmaps by just returning early and doing nothing.

Maybe it'd be best for the scope of this PR to just be reduced to adding the freeImageBuffer() method to FlxGraphic until we work out the details on how to make Flixel take advantage of it in a seperate issue

[Vortex2Oblivion]

Any updates on this?

[ACrazyTown]

Any updates on this?

I'm still not quite sure on how to make Flixel play nice with texture only bitmaps. We could add additional methods to BitmapFrontEnd or wherever for managing texture only bitmaps to make it easier for the user but also so Flixel can take advantage of them internally, however, there's a couple issues regarding internal usage.

As Geokureli mentioned:

Atlases and frame collections are a good example of places where it makes sense that bitmap editing features should be "opt-in"

My initial thought was to just automatically dump all frame collection's graphics but I believe that would break FlxFilterFrames. I guess we could call graphic.refresh() but I don't know how to feel about that either. Loading a graphic twice seems like a jank solution.

[Geokureli]

I think my entire issue with this is the wording and the clarity. I do think we should have a way to clear unnecessary ram, but the feature should be opt in, to avoid breaking any existing code. I just find it confusing that we are calling disposeImage and still using the image, like that seems like something we would call when we are completely done using the bitmap, and freeImageBuffer has a similar vibe to me. The function refresh

I have a feeling we'll be renaming this again, soon, but honestly this is a simple feature that people could use immediately and see benefits from, so maybe I should just merge it? I'd still love to have a name that better conveyed the intended use and consequences: To make a FlxGraphic only use VRAM at the cost of not allowing its data to be edited

[ACrazyTown]

I think my entire issue with this is the wording and the clarity. I do think we should have a way to clear unnecessary ram, but the feature should be opt in, to avoid breaking any existing code.

Makes sense, though I still think it'd be nice for Flixel to take advantage of this internally at some point in the future.

I just find it confusing that we are calling disposeImage and still using the image, like that seems like something we would call when we are completely done using the bitmap, and freeImageBuffer has a similar vibe to me. The function refresh

I guess this is a design flaw with the Flash/OpenFL API. There's two dispose methods: BitmapData.disposeImage() only gets rid of the image buffer. The bitmap is still valid but internally it will only reference the hardware texture. BitmapData.dispose() completely cleans up all data related to the bitmap.

I definitely struggled with the name for the method. LMK if you have any suggestions.

I have a feeling we'll be renaming this again, soon, but honestly this is a simple feature that people could use immediately and see benefits from, so maybe I should just merge it? I'd still love to have a name that better conveyed the intended use and consequences: To make a FlxGraphic only use VRAM at the cost of not allowing its data to be edited

IMO there's no need to rush this. It's mainly just a helper method to give more visibility. Anyone who's looking to implement this themselves can currently just do graphic.bitmap.disposeImage().

Flixel and OpenFL are definitely in a weird situation here. I think the standard for game engines is to have different classes for software and hardware textures. OpenFL/Flash has this (BitmapData and Texture) but the latter is only available when using Context3D and it's not compatible with Flixel's rendering system currently. Also the aforementioned weirdness where BitmapData stores both software and hardware textures internally, despite the concept of BitmapData referring to a software texture.

[Geokureli]

IMO there's no need to rush this. It's mainly just a helper method to give more visibility. Anyone who's looking to implement this themselves can currently just do graphic.bitmap.disposeImage().

Fair enough. gonna move this to 6.2