search

Sonnenlicht-as / Trajectory-Estimation

This mod adds client-side rendered trajectory for some items in game.
GNU General Public License v3.0
5 stars 1 forks source link

[Suggestion] Do some caching #3

Open Kasualix opened 3 months ago

Kasualix commented 3 months ago

ModLoaded caching

https://github.com/Sonnenlicht-as/Trajectory-Estimation/blob/master/src/main/java/sonnenlichts/tje/client/util/ModUtils.java#L32-L90

These methods check for the existence of the mod each time they are called, they can actually be cached into final boolean fields so the check will be executed only once.

Item status caching

https://github.com/Sonnenlicht-as/Trajectory-Estimation/blob/master/src/main/java/sonnenlichts/tje/client/util/ModUtils.java#L92-L374

Mixin

We can use Mixin to give Item two properties: tje$isPointRenderable, tje$canPlayHitSound, and then assign a value to this property when the game initializes or user changes the config, and your long list of checks can be optimized directly to ((ItemExt)stack.getItem()).tje$isPointRenderable() / ((ItemExt)stack.getItem()).tje$canPlayHitSound() (even more details, mcmod.cn account needed or 403 will occur)

HashSet

Instead of creating and initializing the properties, we can create two final HashSet (POINT_RENDERABLE and HIT_SOUND_PLAYABLE eg), store identifiers (maybe ResourceLocation) of items somewhere and finally call HashSet#contains on the original checking position.

https://github.com/Sonnenlicht-as/Trajectory-Estimation/blob/master/src/main/java/sonnenlichts/tje/client/util/ModUtils.java#L377-L418 I suppose this method could do with a bit of cache optimization as well, but I'm too lazy to gather my thoughts any further.

Sonnenlicht-as commented 3 months ago

Thanks for your suggestion. This mod indeed needs optimization to save unnecessary performance loss.

MikhailTapio commented 3 months ago

Such a mixin-powered cache makes the check no longer ItemStack-sensitive. It can be used as a pre-check, but not as a full replacement for the original check.

Kasualix commented 3 months ago

Such a mixin-powered cache makes the check no longer ItemStack-sensitive. It can be used as a pre-check, but not as a full replacement for the original check.

Oh, is there any other methods in ItemStack used besides getItem for the stack? I probably didn't see it. I thought if it just kept calling getItem and checking with instanceof, it could have just been replaced with Item in the first place (and so no longer have anything to do with "ItemStack-sensitive")?

MikhailTapio commented 3 months ago

It depends. For example, NBT and damage are missing on an Item, while types of some ItemStacks may be differentiated again based on these two values, and thus be associated with more than one configuration value. There are more such cases in earlier versions (e.g. the metadata system).

Kasualix commented 3 months ago

It depends. For example, NBT and damage are missing on an Item, while types of some ItemStacks may be differentiated again based on these two values, and thus be associated with more than one configuration value. There are more such cases in earlier versions (e.g. the metadata system).

But the checks also don't seem to care about ItemStack's NBT, do they? It only cares what class the Item behind the ItemStack is.

Am I missing something?

MikhailTapio commented 3 months ago

It depends. For example, NBT and damage are missing on an Item, while types of some ItemStacks may be differentiated again based on these two values, and thus be associated with more than one configuration value. There are more such cases in earlier versions (e.g. the metadata system).

But the checks also don't seem to care about ItemStack's NBT, do they? It only cares what class the Item behind the ItemStack is.

Am I missing something?

Ofc there is. If an item has two "modes of operation", and the player might want it to not trigger the TJE visual effect at all when it's in one of the modes of operation, you'd need two configuration values for each of the two modes of operation. This is something that a single boolean value attached to an Item cannot do.

However, if the mod has stopped there and won't try to implement new compatibility, your method is absolutely perfect.