inventory API is confusing when a window is open

[andrewrk]

The rules for when you can and cannot use bot.inventory while another window is open are confusing and undocumented. This is very not obvious and error prone. Make it so that accessing inventory 'just works' even while interacting with other open windows.

[nevercast]

From my understanding, any GUI that shows that is not the inventory, does not directly interact with the inventory, a GUI can show however many inventory items as it sees fit to do so.

Interfacing with a Window that is not the inventory should not be assumed to be the inventory, and all inventory interaction should actually be unavailable ALWAYS when another window is displayed.

There should be some helpful functions provided to window that allow moving items around. But a bot should treat every GUI uniquely. I believe the server will update the client on the change of inventory items when an item is moved.

Do you know if there are edge cases where the server does not update the player's GUI when an item is moved ?

[Gjum]

While this issues is very old and minecraft changed a lot, the inventory api is still confusing.

treat every GUI uniquely

While it is technically possible to interact with multiple windows at the same time (eg. moving armor from the player inventory to an enchantment table), the player inventory will not get updated directly by the server (eg. picking up items updates the gui window, which at this point also contains the 4*9 player inventory slots).

There should only be one window open at any time, which is either the player inventory or a gui window. The api should thus reflect interacting with the open window, which also is the expected way for users coming from vanilla.

edge cases where the server does not update the player's GUI when an item is moved

The server should never update any slots when clicking, just when opening a new window or when a click action was illegal The client is expected to predict any clicking outcome, including updating multiple slots at once (eg. double-clicking, crafting).

[TheDudeFromCI]

What kind of changes would need to be made to inventory handling to fix this issue? I assume that moving code outside of the Mineflayer repo and into external repos like prismarine-windows has helped a lot.

Is there any other changes that would need to be made to cleanly smooth out the API and expected usage?

[Karang]

I think that adding some kind of synchronization between the inventory and the currently open window can help.

The way windows works is when you open a window, it creates a new inventory and the server resend the content of your inventory if necessary (for instance in a chest). Then while the window is open only this window is modified (even the inventory part). When the window is closed, the server sends an update for the inventory.

This mean that while a window is open, the client cannot modify the inventory but must instead work with the active window (where slots are not the same).

Maybe the easiest way to simplify the api would be to disable the inventory while an other window is open.

[TheDudeFromCI]

It would be easy to fix that by adding a wrapper for the inventory. If a window is open, it would read/write to that window. If the window is closed, it would read/write to bot.inventory.

Just make the editing of bot.inventory directly would be depreciated.

[Karang]

It would also mean depreciating all functions that have direct slot access. (different windows have different slots scheme)

[TheDudeFromCI]

Yes. Everything should be done through functions and getters. Never direct access.

[Karang]

No, what I meant is functions like clickWindow (slot, mouseButton, mode, cb) https://github.com/PrismarineJS/mineflayer/blob/266f2558d3b54664e310c86851c030aba6392f77/lib/plugins/inventory.js#L529 could not be exposed anymore because they require a specific slot id. A solution can be to have a slot conversion layer, but this is not ideal either because some of the slots are simply not available (armor for instance). Anyway this would be an api breaking change.

[TheDudeFromCI]

I'm not proposing to change the current API, but write a new layer on top of it that accesses the current API. The new layer would be a higher-level wrapper that behaves a lot more smoothly and acts a lot closer to how you would expect it to. that API would simply call the existing API.

Long-term the existing API can be phased out can be replaced with a more stable container under the wrapper.

[TheDudeFromCI]

The best way to approach this might be to start by writing the wrapper as a plugin instead. Then slowly updating all of the API for that plugin to not rely on existing Mineflayer inventory code.

When Mineflayer finally gets ready for a 3.0.0 release, deprecate all of the existing code and merge the plugin into the core project.

[TheDudeFromCI]

Thinking about the process further, it might also be possible to convert the existing API to getters and settings without breaking anything.

For example, rename bot.inventory.slots to bot.inventory._slots and then create a custom getter and setter function for bot.inventory.slots that would return _slots when not in a window, and would return the respective section of the currentWindow's inventory slots if a window is open.

This would not break any existing API but should fix some of the issues.

This would be done to all of the inventory API, of course. Not just slots.

[u9g]

Thinking about the process further, it might also be possible to convert the existing API to getters and settings without breaking anything.

For example, rename bot.inventory.slots to bot.inventory._slots and then create a custom getter and setter function for bot.inventory.slots that would return _slots when not in a window, and would return the respective section of the currentWindow's inventory slots if a window is open.

This would not break any existing API but should fix some of the issues.

This would be done to all of the inventory API, of course. Not just slots.

Is this still the ideal idea?