[flame_texturepacker] make Region immutable

[gnarhard]

What could be improved

The Region class should be immutable and all redundant fields in TexturePackerSprite should be removed. WIP: perf/immutable_region_data

https://github.com/gnarhard/flame/tree/perf/immutable_region_data

This _parse method is also not the friendliest to read. I think there could be some improvement there too.

Why should this be improved

I've already confirmed that this will reduce TexturePackerSprite memory consumption by ~50%. These fields that are in the TexturePackerSprite class that I've removed are redundant and already stored in the region data. My branch removes all of that code that shouldn't be mutated anyways, and makes Region immutable. However, I'm hung up on the tests and need help here.

_parse method could be improved to be easier to read and maintain.

Risks

This will be a breaking change. Currently, I don't have access to the texture pack files in the testing environment. Each field needs to be set every time, but since I don't have access to the texture pack save files, I can't check and see which algorithm is being used to update the atlas files accordingly.

More information

No response

Other

• Are you interested in working on a PR for this?

[spydon]

Maybe @Brixto or @Yayo-Arellano knows more about the tests and could give some input.

[Yayo-Arellano]

To be honest. When I created Region, Page, etc. I just followed whatever was on the Libgdx code https://libgdx.com/wiki/graphics/2d/spritebatch-textureregions-and-sprites. So I guess Libgdx has a reason to make the classes mutable.

Moving forward with Flame. @gnarhard I do not really understand what do you need. You said:

but since I don't have access to the texture pack save files, I can't check and see which algorithm is being used to update the atlas files accordingly.

What files do you need? The sprite sheets are already in the example folder including the atlas map. https://github.com/flame-engine/flame/blob/main/packages/flame_texturepacker/example/assets/images/atlas_map.atlas

• The sprite sheets were created using Texture Packer https://github.com/crashinvaders/gdx-texture-packer-gui
• The original Robot assets can be downloaded from https://opengameart.org/content/toon-characters-1

[gnarhard]

To be honest. When I created Region, Page, etc. I just followed whatever was on the Libgdx code https://libgdx.com/wiki/graphics/2d/spritebatch-textureregions-and-sprites. So I guess Libgdx has a reason to make the classes mutable.

Moving forward with Flame. @gnarhard I do not really understand what do you need. You said:

but since I don't have access to the texture pack save files, I can't check and see which algorithm is being used to update the atlas files accordingly.

What files do you need? The sprite sheets are already in the example folder including the atlas map. https://github.com/flame-engine/flame/blob/main/packages/flame_texturepacker/example/assets/images/atlas_map.atlas

• The sprite sheets were created using Texture Packer https://github.com/crashinvaders/gdx-texture-packer-gui
• The original Robot assets can be downloaded from https://opengameart.org/content/toon-characters-1

Thanks for the quick response! I read what you linked and didn't find any reference of where it needs to be mutable. Conceptually, a region should never be mutated because it represents the area that the sprite exists in the packed texture. I think the only possible use case for mutating the region would be to bound a spritesheet versus individual sprites on demand. However, this isn't possible if you're exporting with the maxRects option. It should be an option from the start to bound spritesheets, if that's needed, so the region could still be instanced as immutable. Also, a 50% performance improvement is pretty massive and worth the trouble of making it immutable. Maybe I'm not thinking of some use case tho, so please fill me in if you think of anything.

Regarding what I need, I would just want the TPS file in the testing directories. This will save testers a step because there are quite a few options in this program that are saved in the tps file and not in the atlas json file.

Separate issue: We're forced to use the legacy output mode in the program which is disabled by default so we should definitely look into modernizing the package to adhere to the new standard.

[gnarhard]

@Yayo-Arellano it looks like the new output mode is supported. Disregard that.

[Yayo-Arellano]

Yeah, actually I do not know what is the reason behind that in Libgdx those clases are mutable. But if making immutable has improvements this should good to go.

About the GDX Texture Packer Save files, they are gone.. I never thought they will be needed to I did not store them anywhere. If you really need save files, you will have to recreate them, and maybe update the tests cases if needed.

@Yayo-Arellano it looks like the new output mode is supported. Disregard that.
Yes. In all my projects I use the new output mode.

cc: @gnarhard

[gnarhard]

It isn't the end of the world. The textures you added were simple and easy enough to use. I really appreciate the hard work you've put into this package so far. I just made a PR of all of my suggested changes and added as many optimizations as I could. I'm not sure how it could be optimized more at this point haha! Check it out when you get the chance. Eventually it'd be cool to get this thing fully featured with what LibGDX has to offer. I think there's more to be done on the bleed/extrude side which I might take a look at soon.