search

LonamiWebs / Klooni1010

libGDX game based on the original 1010!
https://lonamiwebs.github.io/klooni
GNU General Public License v3.0
234 stars 72 forks source link

assets MUST NOT be loaded in static code #42

Open andidevi opened 6 years ago

andidevi commented 6 years ago

There are issues when loading asses in static code as that is exposed to a race condition. Loading assets with SkinLoader must not happen before class Klooni is initiated. In other cases Gdx.graphics will be null. It happens that this condition is accidently met for the current state of the code.

The order classes are loaded and initialized is subject to the class loader involved and that may change in the future, especially as this is an Android app and Google is experimenting much with it's runtime.

There are more reason to not load assets in static code, ie. error handling and the like.

Example of issues (initiate effects in static code from class Klooni): SkinLoader static Exception in thread "main" java.lang.ExceptionInInitializerError at io.github.lonamiwebs.klooni.effects.WaterdropEffectFactory.(WaterdropEffectFactory.java:38) at io.github.lonamiwebs.klooni.Klooni.(Klooni.java:46) at io.github.lonamiwebs.klooni.desktop.DesktopLauncher.main(DesktopLauncher.java:34) Caused by: java.lang.NullPointerException at io.github.lonamiwebs.klooni.SkinLoader.(SkinLoader.java:43) ... 3 more

Lonami commented 6 years ago

Is this still an issue after #43 got merged? (I see you moved the Waterdrop static initialization into the constructor which is nice).

Also regarding the FIXME you introduced: https://github.com/LonamiWebs/Klooni1010/blob/ef2fb8fa84bc1fa2f6c5cea777a91ae64a8fff99/core/src/io/github/lonamiwebs/klooni/Klooni.java#L46-L47

Sure, it's static, but it's not loading anything. It defaults to null, and is loaded later on create: https://github.com/LonamiWebs/Klooni1010/blob/ef2fb8fa84bc1fa2f6c5cea777a91ae64a8fff99/core/src/io/github/lonamiwebs/klooni/Klooni.java#L82-L95

So, is this still wrong?

andidevi commented 6 years ago

The text is according to your TODO that have been there before. I did'n have any look into it beside the code of SkinLoader.

As to to the comment in Line 89 says it is a singleton, you should not allow public write access to it and use a public getter ... and have a look if that's really a performance hit or not ;)

As it isn't loaded that early there's no need for static at all. If other classes need access, they should simply know Klooni. This would lead to more refactoring, ie. making the cellTexture a parameter of Cell.draw or supplying it at cell creation and changing the parameter of set to the already resolved texture to having Cells and maybe other to not needing to know Klooni.

Alternatively you could have a private static variable to store the Klooni instance and supply it with a public static getter.

... much changes and I don't see a practical benefit now. As a big difference to effects you only have one Theme class and there's no need to change as long as that one size fits all.