Add DisposalHelper class

[Anton-Samarskyi]

DisposalHelper is designed to help with disposal process of multiple resources. There could be a potential memory leak when exception occurs while disposing multiple resources. DisposalHelper provides handy way to avoid such situation.

Skin class was updated to use DisposalHelper to show an example of usage.

Here is another one-line example:
DisposalHelper.disposeAll(batch, font /* any other resources that have to be disposed one by one */);

[obigu]

I don't quite see the need for this. As far as I understand, if an exception occurs when disposing a resource, that is a bug that has to be fixed on the app itself and the potential memory leak in that scenario will disappear as soon as it's fixed. I see it may add some convenience not to have to do some null checks but I'm not sure it justifies the overhead.

[Anton-Samarskyi]

I don't quite see the need for this. As far as I understand, if an exception occurs when disposing a resource, that is a bug that has to be fixed on the app itself and the potential memory leak in that scenario will disappear as soon as it's fixed. I see it may add some convenience not to have to do some null checks but I'm not sure it justifies the overhead.

Overhead is not big and only occurs at disposal phase. So, I believe it worth it.
Regarding "app should fix a bug", it's a bit naive. Bugs are going to be anyway. Usage of this DisposalHelper is going to reduce possible consequences of bugs. And sometimes exceptions are not so critical to waste time by fixing them. For instance, double disposal may produce "Pixmap already disposed!" exception. It might be easier to just ignore this exception, but when it occurs in the Skin class, there is no guarantee that other resources were already disposed. DisposalHelper provide such guarantee.
And yes, my example is pretty straightforward. But in the real world bugs may be floating, difficult to fix or just never caught by the developer while app is already released. There are so many cases when DisposalHelper could fix possible issues, so I insist it should be added.

[Berstanio]

Overhead is not big and only occurs at disposal phase.

I think adding a new class and new exception is a bit overkill.
For what I see use cases, if you really don't care about proper disposal, I could see a public static void disposeSilently(Disposable... disposable) method, which is just a loop and catches any exceptions. However, no internal libGDX API should in my opinion use this, since it just hides exceptions and disables any other meaningful way of handling them.
And I'm also not fully sold on that method, since it just ignores bugs in your app. Double/failed disposal might lead to leaks/bugs further down the line, however you will never notice, because the exception will never occur.

[Anton-Samarskyi]

Overhead is not big and only occurs at disposal phase.

I think adding a new class and new exception is a bit overkill. For what I see use cases, if you really don't care about proper disposal, I could see a public static void disposeSilently(Disposable... disposable) method, which is just a loop and catches any exceptions. However, no internal libGDX API should in my opinion use this, since it just hides exceptions and disables any other meaningful way of handling them. And I'm also not fully sold on that method, since it just ignores bugs in your app. Double/failed disposal might lead to leaks/bugs further down the line, however you will never notice, because the exception will never occur.

I agree disposeSilently should never be used in internal libGDX code. But it might be convenient to use in some cases in the application code. By default it would show a stacktrace in case of an exception, so issues could be traced during the development.
I can remove that "silent" methods if you consider them too dangerous. But I believe DisposalHelper overall is a good thing.

[obigu]

Overhead is not big and only occurs at disposal phase. So, I believe it worth it. Regarding "app should fix a bug", it's a bit naive. Bugs are going to be anyway. Usage of this DisposalHelper is going to reduce possible consequences of bugs. And sometimes exceptions are not so critical to waste time by fixing them. For instance, double disposal may produce "Pixmap already disposed!" exception. It might be easier to just ignore this exception, but when it occurs in the Skin class, there is no guarantee that other resources were already disposed. DisposalHelper provide such guarantee. And yes, my example is pretty straightforward. But in the real world bugs may be floating, difficult to fix or just never caught by the developer while app is already released. There are so many cases when DisposalHelper could fix possible issues, so I insist it should be added.

Probably "overhead" was a wrong choice of words, I meant too much complexity for something simple (such as adding a whole new Exception class), maybe "overkill" would be more appropriate.

The use case regarding Pixmap is quite valid but maybe, even if it's a separate discussion, we should consider if it makes sense to throw an Exception if it's already disposed, I don't think that happens with the other classes that implement the Disposable interface.

I still don't see all those use cases in which this is useful. This doesn't fix non detected bugs in apps, it may just potentially prevent some memory leak in a buggy app by crashing the app a bit later or avoiding the crash altogether (disposeSilently) by swallowing exceptions (which I wouldn't recommend but that's something that can be done easily with a simple try catch block withouth the need for this).

It'd also be limited to games that manually dispose stuff and don't use AssetManager which is the libGDX standard way to manage asset dependencies.

[Anton-Samarskyi]

Well, idk. My vision is that overall it's good to wrap some constantly repeatable actions into some single unit (class or method) with convenient interface and use it even if it looks a bit complex inside.

I agree "silent" methods might be dangerous, I can remove them. But what about other parts of DisposalHelper? I still believe it's good thing to have.

P.S. anyway thank you for paying attention on my PR!

[MobiDevelop]

I personally don't really see the need for this at the framework level - it feels like an application layer utility.

[PokeMMO]

The use case regarding Pixmap is quite valid but maybe, even if it's a separate discussion, we should consider if it makes sense to throw an Exception if it's already disposed, I don't think that happens with the other classes that implement the Disposable interface.

From my experience, everything in libgdx is safe to double dispose. I'd argue that everything should be safe to call dispose on multiple times. Ideally you should never end up doing it, but it shouldn't be a pitfall if you do.

[Anton-Samarskyi]

From my experience, everything in libgdx is safe to double dispose.

I don't know your experience. But I'm pretty new here and already faced a case when Skin produced an exception due to double dispose (that Pixmap that I've mentioned).
As result I assumed it might frequent issue in case something was utilized in not a "standard" way. And I decided to try to make it better.

I personally don't really see the need for this at the framework level - it feels like an application layer utility.

TBH, I don't know how to correctly dispose Skin object in my case without this change (and without reflection magic). If you interested in details, here is an example on Kotlin:
fun createSkinForFont(font: BitmapFont): Skin {
val skin = Skin()
skin.add("font", font)
skin.addRegions(TextureAtlas(Gdx.files.internal("skin/skin.atlas")))
skin.load(Gdx.files.internal("skin/skin.json"))
return skin
}

And skin.json part:
com.badlogic.gdx.graphics.g2d.BitmapFont: {
default: font
}
So I just tried to create Skin with custom font, but for some reason it's added twice. And on skin.dispose() it produces "Pixmap already disposed!" exception. According to inners of the Skin, when disposal failed for one inner element, it's not performed for others that are left. So, this is exactly what I tried to fix as it may occur in other situations.

[PokeMMO]

I don't know your experience. But I'm pretty new here and already faced a case when Skin produced an exception due to double dispose (that Pixmap that I've mentioned).

Oh, looks like you're right. I wonder why we throw an exception instead of just ignoring the second call.

[MobiDevelop]

I think your scenario is probably more correctly described as a question of resource ownership - in other words, Skin doesn't know that it doesn't own the resource you are providing (the BitmapFont) and tries to dispose of it when the skin is disposed. In the snippet provided, there is not enough surrounding code to tell but I assume the ownership of the bitmap font is retained by the caller and it is disposed separately from the skin.

To me, it would be worth looking into whether Skin should know that it doesn't own a resource and not try to dispose it.

[Anton-Samarskyi]

I think your scenario is probably more correctly described as a question of resource ownership - in other words, Skin doesn't know that it doesn't own the resource you are providing (the BitmapFont) and tries to dispose of it when the skin is disposed. In the snippet provided, there is not enough surrounding code to tell but I assume the ownership of the bitmap font is retained by the caller and it is disposed separately from the skin.

To me, it would be worth looking into whether Skin should know that it doesn't own a resource and not try to dispose it.

I create font in the next way:
val generator = FreeTypeFontGenerator(Gdx.files.internal(TTF_NAME))
val parameter = FreeTypeFontGenerator.FreeTypeFontParameter()
parameter.size = 30
val font30 = generator.generateFont(parameter)
generator.dispose() // font is not disposed here
bigFontSkin = createSkinForFont(font30)

And I do not store it anywhere out of Skin. But debugging showed that Skin stores this font using two links (I don't know why). So, it's like it owns font twice. And this leads to an exception at the end.

I know, this case may be fixed. Someone is going to say I'm doing something wrong. I agree, but this example works OK except the disposal issue. And how many more situations are possible were owning was not correct and the only result of it is just double disposing? Idk, but again, at the moment of creating this PR I assumed these situations may occur pretty frequent.

[obigu]

I've created #7347 independently of a possible proper fix in Skin (either because there's a bug or because Skin should care about the resources it owns).