Add InventoryEvents to fabric-screen-handler-api-v1 with Test Mod #4942
Conversation
|
I just noticed there is a mistake in the docs in the InventoryEvents class where it states that mod authors should manually sync the client state, I've written this before i was correctly syncing everything, It's not required currently and can be missleading |
…issues with testing - Everything used to work fine but the mod broke after refactoring into the screen-handler-api-v1 module (the api was built as it's seperate module fabric-inventory-events-v1) a seperate test mod could be provided if needed but i have done ample testing to insure stability and cleanliness
Patbox
left a comment
Patbox
left a comment
There was a problem hiding this comment.
Gave some feedback on current implementation.
Also worth noting that this PR:
- This only handles simple cases of inventory slot click and others. Complex changes naturally will still require mixins.
- Doesn't really reduce mod conflicts, as proficient devs would know how to handle it.
- Custom container mechanics and item restrictions shouldn't be implemented purely via click detection, because for example a sorting mod on the server will skip these checks to be faster, which might cause bypasses (wouldn't be the case if you validated it in Slot class, as one should).
| */ | ||
| ActionResult interact( | ||
| ScreenHandler handler, | ||
| Slot slot, |
There was a problem hiding this comment.
This should be marked as @ Nullable, since you do pass null here.
| * @param player The player who clicked the slot | ||
| * @param ci Callback info for canceling the method | ||
| */ | ||
| @Inject(method = "onSlotClick", at = @At("HEAD"), cancellable = true) |
There was a problem hiding this comment.
If you want for it to hold true that it always runs before actual logic, you would need to move this mixin to network handler, as mods can and do override onSlotClick method
| if (result == ActionResult.FAIL) { | ||
| ci.cancel(); | ||
| // Sync client state to prevent desync | ||
| handler.syncState(); |
There was a problem hiding this comment.
I don't think you should sync it like this. I'm pretty sure vanilla should have system for detecting desyncs natively, if not you should use that instead of syncing entire inventory
|
I understand, I got told to only start by adding a simple event. I also know that chest protection shouldn't be added like this, this is just a simple proof of concept that makes use of the simple event added. Thank you for the review! |
| * <p>Canceling with {@code ActionResult.FAIL} stops the click entirely. Returning {@code ActionResult.SUCCESS} allows the action but consumes the event. | ||
| */ | ||
| @FunctionalInterface | ||
| public interface SlotClickCallback { |
There was a problem hiding this comment.
Move this class to your InventoryEvents, example: https://github.com/FabricMC/fabric/blob/1.21.10/fabric-lifecycle-events-v1/src/main/java/net/fabricmc/fabric/api/event/lifecycle/v1/ServerLifecycleEvents.java
edit: And rename to SlotClick, without "Callback"
3 participants
This PR adds 'InventoryEvents' into the 'screen-handler-api-v1' module, providing a simple way to handle inventory slot click events, the API introduces 'InventoryEvents.SLOT_CLICK_EVENT', which allows modders to intercept and modify slot interactions (e.g, 'PICKUP', 'SWAP', 'QUICK_MOVE', 'QUICK_CRAFT') with minimal boilerplate, reducing reliance on complex mixins or custom screen handlers.
The plan was for 3 events to be added SLOT_CLICK_EVENT, INVENTORY_OPEN_EVENT and CRAFT_RESULT_EVENT.
Problems Solved:
Test Mod:
Added 'InventoryEventsTest' to the existing test mod, demonstrating Diamond Protection (prevents moving Diamonds in any container)
Changes:
Testing Instructions: