maxuser0 / minescript

Python scripting for Minecraft
GNU General Public License v3.0
33 stars 3 forks source link

minescript.player_inventory_slot_to_hotbar() creating ghost items #5

Closed TimGeel1998 closed 1 year ago

TimGeel1998 commented 1 year ago

Summary: I have a script that finds the first eadeble item in the players inventory and moves it to the hotbar. When using the minescript.player_inventory_slot_to_hotbar() to move the item to the hotbar it then generates a ghost (nonexistent) item in that slot. The original item (that was supposed to be moved) has become invisible in the inventory, if clicked the item returns. Opening the inventory and clicking the ghost item removes it, Dropping the item with 'Q' does also not work

Expected behavior: I expect the real item to be moved to the hotbar.

Actual behavior: Steps to Reproduce

1. Start Minecraft with the minescript mod enabled.
2. Load a world (Singleplayer or Multiplayer).
3. Run any script with minescript.player_inventory_slot_to_hotbar() that moves an item to the hotbar.
4. Enjoy your new ghost items :)

Environment

Operating System:    [Pop!_OS 22.04 LTS]
Minecraft version:   [I have tested version 1.20 and 1.20.2]
Java version:        [Oracle Java 17 JDK]
Minecraft Modloader: [Fabric with Fabric API 0.90.7+1.20.2]
Version/Commit:      [minescript-mc1.20-fabric-mod-3.1.jar From curseforge.com]
Link to script used: [https://gitlab.timgeel.nl/geel/minecripts/-/blob/a69356ae833df0e87fd839d32785f4a086a294f9/test.py]

latest.log:

[11:01:52] [Render thread/INFO]: Processing command from chat event: \test [11:01:53] [Server thread/INFO]: [Not Secure] \<user> First acceptable item found in slot 9. Swapping to hotbar slot . [11:01:53] [Render thread/INFO]: [CHAT] \<user> First acceptable item found in slot 9. Swapping to hotbar slot . [11:01:53] [job-1-test/INFO]: Exited script event loop for job `[1] Done: test`

*Left out real username

related issues or pull requests. The closest thing i could find to this is: https://bugs.mojang.com/browse/MC-219018

Should you require additional information, I would be happy to provide it.

MCT32 commented 1 year ago

Was able to replicate the issue on fabric, but the issue likely also works forge. The issue, for fabric at least, seems to be that the script is executed on the client, however minescript.player_inventory_slot_to_hotbar() uses the function swapSlotWithHotbar() in PlayerInventory which is likely either meant only to be executed by the server, or executed by both the server and the client at the same time. I may fork the repo and try fixing this issue later, but a potential fix is to use functions from the GUI that will send the relevant packets to the server when executed.

maxuser0 commented 1 year ago

@TimGeel1998 Can you confirm that the issue occurs on a local single-player world? If it does, then it would appear to not be an issue with client and server getting out of sync, as local single-player worlds run with an integrated server.

Also, can you post a short snippet of code for where you're getting the inventory slot that you're passing into player_inventory_slot_to_hotbar(...)? It should be the "slot" attribute of an item returned from player_inventory(). The following Minescript command should print the valid slot numbers:

\eval "[item['slot'] for item in player_inventory() if item['slot'] >= 9]"

MCT32 commented 1 year ago

I replicated the error on local singleplayer, however i believe to my understanding that although the game is in singleplayer, there is still a seperate "server" and "client" under the hood, only they are running on the same machine. these are know as a "logical server" and a "logical client". because of this, whether the code is executed on the server or the client still matters in a singleplayer world. this is explained in the forge documentation, specifically read the "logical server" section: https://docs.minecraftforge.net/en/latest/concepts/sides/

MCT32 commented 1 year ago

this section also explains how even in singleplayer, data must be sent between logical client and server using network packets: https://docs.minecraftforge.net/en/latest/concepts/sides/#reaching-across-logical-sides

maxuser0 commented 1 year ago

Thanks for the info.

Can you post an example of your usage, specifically how you're getting the value of the slot you're passing to player_inventory_slot_to_hotbar(...)?

MCT32 commented 1 year ago

Not at my computer right now so i cant get the exact script but it was something like this:

import minescript

minescript.player_inventory_slot_to_hotbar(9)

then i just put an item in my top left most inventory slot and the ghost item is created as described. i tried it with a block and it couldn't be placed and if i dropped it it would disappear. in creative i couldn't get the original back by clicking in empty space like described in the first post but i could in survival.

TimGeel1998 commented 1 year ago

@maxuser0 Sorry for my late reply. i can confirm that the issue occurs on a local single-player game and in multyplayer. but like @MCT32 already said, even single player-starts a server.

as for the code question:

import minescript

acceptable_items = ["bread", "apple", "baked_potato", "cooked_beef", "cooked_chicken", "cooked_mutton", "cooked_cod", "cooked_porkchop", "cooked_rabbit", "cooked_salmon"]

def find_first_acceptable_item_slot():
    # Get the player's inventory
    inventory = minescript.player_inventory()

    for item_data in inventory:
        item_name = item_data.get('item', '')
        if item_name in acceptable_items:
            return item_data.get('slot', None)

    # Return None if no acceptable item was found
    return None

def swap_to_hotbar(slot):
    if slot is not None:
        # Swap the item from the found slot to hotbar slot 
        minescript.player_inventory_select_slot(8)
        minescript.player_inventory_slot_to_hotbar(result)
        #try flush?
        minescript.flush()

# Example usage
result = find_first_acceptable_item_slot()

if result is not None:
    print(f"First acceptable item found in slot {result}. Swapping to hotbar slot .")
    swap_to_hotbar(result)
else:
    print("No acceptable items found in the inventory.")
MCT32 commented 1 year ago

I think I've managed to fix the issue! Instead of directly calling inventory code on the client, I'm sending a packet to the server to move the item to my hotbar, the the server handles everything for us. I've got this working on fabric but the same fix should work on forge. I'll implement the fix on forge then create a pull request.

MCT32 commented 1 year ago

Hmm, I'm having trouble getting forge to send a packet. It seems like sending packets in forge is a bit more difficult. I'll make a draft pull request with the fabric changes until i can work out forge.

maxuser0 commented 1 year ago

I've approved and merged your pull request into the main branch. Thanks for the fix! I'll get that integrated into the versioned branches, probably deploying Minescript 3.1.1 with the bug fix in the coming days.

I have the equivalent fix for forge using:

import net.minecraft.network.protocol.game.ServerboundPickItemPacket;
...
minecraft.getConnection().send(new ServerboundPickItemPacket(slot));

I haven't run into any issues with ghost items with the original implementation, so I don't see any difference in behavior before/after applying the fix. Can you confirm that the forge fix works for you? Or is this what you already tried?

MCT32 commented 1 year ago

I've approved and merged your pull request into the main branch. Thanks for the fix! I'll get that integrated into the versioned branches, probably deploying Minescript 3.1.1 with the bug fix in the coming days.

I have the equivalent fix for forge using:

import net.minecraft.network.protocol.game.ServerboundPickItemPacket;
...
minecraft.getConnection().send(new ServerboundPickItemPacket(slot));

I haven't run into any issues with ghost items with the original implementation, so I don't see any difference in behavior before/after applying the fix. Can you confirm that the forge fix works for you? Or is this what you already tried?

It did make a difference for me on fabric, and I'm pretty sure i had the issue on forge but im less sure about forge. did you find not issue on forge or fabric? also make sure you either try placing a block or moving the item in survival because i found moving the item in my inventory whilst in creative had no issue before the fix, only in survival. either way, i think using network packets is better in general, in my opinion at least.

MCT32 commented 1 year ago

also @maxuser0 you should reopen the issue if it still doesn't work on forge, accepting the pull request automatically closed it.

maxuser0 commented 1 year ago

Reopening until forge issue is fixed. Will try testing in survival mode as suggested by @MCT32 .

maxuser0 commented 1 year ago

Ah, I see now what you mean by a ghost item. Here are the steps I used to repro:

  1. Set gamemode to survival.
  2. Break a block and pick it up.
  3. Move the item into your inventory.
  4. Use player_inventory_slot_to_hotbar() to move the item to the hotbar.
  5. Place the item into the world.
  6. See the item briefly appear then disappear.

I submitted a fix for forge, and confirmed that the fix works for both fabric and forge.

I plan to release these fixes in version 3.1.1 in the coming days.

Thanks for the help debugging and fixing this issue!