Improvement: use minecraft's keybind structure for inventory move #131
Conversation
|
This is a regression as the user is no longer able to rotate using the arrow keys |
|
oh sorry i didn't mean to remove that feature. I'll fix it. I don't think any of the changes i made should affect it |
…nto improvement/inventory-move # Conflicts: # common/src/main/kotlin/com/lambda/module/modules/player/InventoryMove.kt
There was a problem hiding this comment.
Pull Request Overview
This PR improves the InventoryMove module by switching from manual movement input handling to using Minecraft's native keybind system. This allows the module to respect player-configured keybinds and delegates movement logic to Minecraft's built-in systems.
- Removed custom movement input handling and replaced it with keybind-based movement detection
- Added keyboard input interception to maintain movement key states when inventory screens are open
- Introduced selective key unpressing to preserve movement keys while clearing other keybinds during screen transitions
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lambda.accesswidener | Exposes KeyBinding internals for accessing key mappings and reset functionality |
| InventoryMove.kt | Refactored from manual input handling to keybind detection with arrow key rotation option |
| KeyboardMixin.java | Added input interception to maintain movement key states during inventory screens |
| MinecraftClientMixin.java | Modified key unpressing behavior to preserve movement keys during screen transitions |
|
|
||
| @Redirect(method = "setScreen", at = @At(value = "INVOKE", target = "Lnet/minecraft/client/option/KeyBinding;unpressAll()V")) | ||
| private void redirectUnPressAll() { | ||
| if (InventoryMove.INSTANCE.isDisabled() || InventoryMove.hasInputOrNull(currentScreen)) { |
There was a problem hiding this comment.
The condition InventoryMove.INSTANCE.isDisabled() || InventoryMove.hasInputOrNull(currentScreen) is duplicated in KeyboardMixin. Consider extracting this logic into a utility method to reduce code duplication.
| if (InventoryMove.INSTANCE.isDisabled() || InventoryMove.hasInputOrNull(currentScreen)) { | |
| if (shouldUnpressAll(currentScreen)) { |
| KeyBinding.KEYS_BY_ID.values().forEach(bind -> { | ||
| if (!InventoryMove.isKeyMovementRelated(bind.boundKey.getCode())) { | ||
| bind.reset(); | ||
| } | ||
| }); |
There was a problem hiding this comment.
Using forEach with a lambda on every screen transition could be inefficient. Consider using a traditional for-each loop or caching the filtered keybindings for better performance.
| KeyBinding.KEYS_BY_ID.values().forEach(bind -> { | |
| if (!InventoryMove.isKeyMovementRelated(bind.boundKey.getCode())) { | |
| bind.reset(); | |
| } | |
| }); | |
| for (KeyBinding bind : KeyBinding.KEYS_BY_ID.values()) { | |
| if (!InventoryMove.isKeyMovementRelated(bind.boundKey.getCode())) { | |
| bind.reset(); | |
| } | |
| } |
|
Thanks but i had to redo it from scratch due to massively outdated target branch :/ |
alters the inventory move module to use the players keybinds over default, and lets minecraft handle the movement logic