This PR is meant to address an issue where if the slots are reloaded and the player opens the Creative inventory screen before the process is done, the game will crash with a NullPointerException.
At a very basic level, simply changing the implementation to avoid unboxing a null object is enough to address the issue and that's part of the PR. The PR also includes various additional null checks and annotations to further guard against any potential NPEs related to the maps used in the PlayerScreenHandlerMixin. Although the second part of this PR should avoid this problem entirely, I imagine it's better to be safe and handle null cases appropriately.
Solving the Desync
As for the underlying issue that causes the map values to be null in the first place, EntitySlotLoader.INSTANCE being a singleton results in a desync in single player worlds when the slots are reloaded. The slots reload immediately but there is a lag before the client updates its stored group references, causing any access attempts to fail during the interim.
The proposed solution is to split the entity slot loader into client and server instances. The server instance is the only one that actually reloads and syncs to the client, the client only serves as an intermediary to sync with the server and then relay the slot information to other client behavior.
Since checking for the logical side necessitates a world instance (to my knowledge at least), the PR includes additional methods added to TrinketsApi to deprecate getPlayerSlots and getEntitySlots(EntityType<?>). One variant takes a world and an entity type, while another variant takes an entity itself. I imagine the latter would see more use, but I added the former since there may be times when a modder wishes to access the slot information of an entity type without an actual entity instance.
Another Small Bugfix
While testing the PR, I noticed there was a bug where the group counts for rendering slots in Trinket screens would not update properly when slots reload. The result is that the border for extra groups would not accurately reflect the correct number (but this would only ever be noticed if there were more than 4 extra groups to begin with). This PR addresses that as well with a one-line edit to how the group count is assigned in PlayerScreenHandlerMixin.
This PR is meant to address an issue where if the slots are reloaded and the player opens the Creative inventory screen before the process is done, the game will crash with a NullPointerException.
This should close issues https://github.com/emilyploszaj/trinkets/issues/175, https://github.com/emilyploszaj/trinkets/issues/173, https://github.com/emilyploszaj/trinkets/issues/156, https://github.com/emilyploszaj/trinkets/issues/149 and supersedes PR https://github.com/emilyploszaj/trinkets/pull/195.
The Actual Crash Part
The direct cause of the NPE, and why it only occurs in the Creative inventory screen, is because of this: https://github.com/emilyploszaj/trinkets/blob/af941085551c0535d46ffcb405216f56809329e3/src/main/java/dev/emi/trinkets/mixin/CreativeInventoryScreenMixin.java#L138 https://github.com/emilyploszaj/trinkets/blob/af941085551c0535d46ffcb405216f56809329e3/src/main/java/dev/emi/trinkets/mixin/PlayerScreenHandlerMixin.java#L174-L176 The map values can be null and attempts to unbox the null object as a primitive int results in a crash.
At a very basic level, simply changing the implementation to avoid unboxing a null object is enough to address the issue and that's part of the PR. The PR also includes various additional null checks and annotations to further guard against any potential NPEs related to the maps used in the
PlayerScreenHandlerMixin
. Although the second part of this PR should avoid this problem entirely, I imagine it's better to be safe and handle null cases appropriately.Solving the Desync
As for the underlying issue that causes the map values to be null in the first place,
EntitySlotLoader.INSTANCE
being a singleton results in a desync in single player worlds when the slots are reloaded. The slots reload immediately but there is a lag before the client updates its stored group references, causing any access attempts to fail during the interim.The proposed solution is to split the entity slot loader into client and server instances. The server instance is the only one that actually reloads and syncs to the client, the client only serves as an intermediary to sync with the server and then relay the slot information to other client behavior.
Since checking for the logical side necessitates a world instance (to my knowledge at least), the PR includes additional methods added to
TrinketsApi
to deprecategetPlayerSlots
andgetEntitySlots(EntityType<?>)
. One variant takes a world and an entity type, while another variant takes an entity itself. I imagine the latter would see more use, but I added the former since there may be times when a modder wishes to access the slot information of an entity type without an actual entity instance.Another Small Bugfix
While testing the PR, I noticed there was a bug where the group counts for rendering slots in Trinket screens would not update properly when slots reload. The result is that the border for extra groups would not accurately reflect the correct number (but this would only ever be noticed if there were more than 4 extra groups to begin with). This PR addresses that as well with a one-line edit to how the group count is assigned in
PlayerScreenHandlerMixin
.