Sponge: Items offered to player inventory placed incorrectly

[codeHusky]

I am currently running

• SpongeForge version: Not tested, but reported by others
• SpongeVanilla version: Will post exact soon, but it's something like release 1.12.2-7.1.0-52
• Java version: 1.8.0_151
• Operating System: Win10

Plugins/Mods:

• HuskyCrates
• HuskyUI

Issue Description
Items are placed on the lowest slot position in the players inventory rather than in the hotbar if it's opened, which is, and has been, expected functionality for years.

Additionally, requiring some complicated query code to reach this functionality does not make this not a bug, that's simply not expected functionality.

Currently don't have a code snippet, but the characteristics of this issue should make reproduction rather straight forward. I'll have a code snippet tonight though.

[SimonFlash]

This is not an implementation issue. See SpongeAPI#1795 for a potential solution to expose this to the API.

[codeHusky]

Until then, it's a bug due to unexpected functionality. "Compatibility and consistency" doesn't work when you don't supply the mechanisms to maintain that.

[SimonFlash]

The reason this happens is because of the change in order made to the MainPlayerInventoryLens. Instead of placing the HotBar before the GridInventory, the HotBar now is below because that matches the view of the inventory itself - the hotbar is displayed below the rest of the inventory. Unfortunately, a side-effect of this is that offering items to this inventory puts them into the GridInventory first because its slots now come first.

If you were to attempt to fix this at an implementation level, you would either have to revert that change (which isn't a solution as it was changed for a reason) or alter the order of slots used when offering to the inventory. If this is even possible to do, then we now have an inventory construct in a different order - not a good idea - and those changes may not be reflected into other lenses.

I don't think there's a solution to this that works - I don't believe we should try to alter the order of slots during when offering to put the hotbar first. This is going to result in some lasting confusion, but unfortunately it's a implementation quirk from how the system is written. That does not, however, mean it is a bug - the solution is not to change the implementation but to adjust the API to support the extended behavior, as shown in the issue I linked.

[codeHusky]

Again, none of that really matters. This is a question of expected default functionality. I'm not expecting to add items to the inventory and then they appear in the backpack portion, instead of the hotbar. Adding to the API definitely won't solve the problem in any short span of time rather than just reverting the change until a suitable solution is added to the API to do this programmatically. Once that is actually implemented and usable in the API, the change can be put back to the way it is, but that NEEDS to be documented somewhere obvious so others aren't confused.

[Faithcaio]

Why do you expect the hotbar to come first?
If you got a Grid Inventory would you not expect it to go from top-left to bottom right by default?
Why should the player inventory be an exception here?
Going from bottem left to bottom right and then up again to top left and back to down right is just weird.

Consistency is definitly the most important thing here.
Calling the same method on the same interface should do the same thing.
You would not want
player.getInventory().query(QueryOperationTypes.INVENTORY.of(MainPlayerInventory.class).offer(stack)
and
event.getTargetInventory.query(QueryOperationTypes.INVENTORY.of(MainPlayerInventory.class).offer(stack)
to behave differently.

Which it was doing before because in Minecraft for some reason your inventory is ordered differently closed vs opened (IInventory vs Container)
And as I found out some weeks ago not only maininventory/hotbar also the Armor Slots order are reversed.

[codeHusky]

My main thing is that, if I remember correctly, the default /give command puts the item into your first available hotbar slot if possible. Its the same for item pickup, which is why this is counter-intuitive in my opinion.

It would make more sense to try and be consistent with Minecraft itself rather than the way generic containers work in the API. I'd prefer to have it be the former, since I can actually replicate that behavior in game in a few ways, but the latter might make sense to the person writing, not using, the API itself.

[SimonFlash]

@codeHusky Claiming "none of that really matters" is a ludicrous and ignorant statement. I explained why this discrepancy is happening and why the change was made, so understand that any alteration to make things have your expected behavior within SpongeCommon is a change to the underlying implementation of the inventory.

As Faith has said though, this change is the result of an inconsistency between how the order of the GridInventory and Hotbar were represented and led to some strange results. While our goal is to emulate Minecraft, this doesn't mean that we are required to have the API be the same - in fact, then are many differences between how we do things in Sponge versus how they'd be done in Bukkit; both of which vary from how it would be done using NMS alone. It's not always possible (or idea) to have this be done in the same was as NMS especially when you take mods into account. If we were 'consistent' with Minecraft in the way you're claiming for everything, it would be an absolute nightmare to work with.

I do, however, agree that in the interests of compatibility for API 7 this change should have been considered a breaking and delayed until API 8. Whether to revert this for now or require people to use a new API method is yet to be determined, and I personally don't have a preference between them.

[codeHusky]

The main point is just the change with no actual solution. It's frustrating that such a change wasn't made very clear and seems to have appeared as an improvement rather than part of API 8, which I'd suggest for these changes to be postponed to. I'd rather not have a ton of inconsistent behavior in API 7 (like previous API versions) is all, really. I don't remember the inventory behavior being like this in the initial release of API 7, correct me if I'm wrong.

[Faithcaio]

Didn't I change this before API 7 got released? let me check...
nvm it was after
But then again
people complained Container and Inventory order being different

[codeHusky]

I feel like it was one of those last minute PRs if anything, since I don't remember this until API7 went to release.

[Faithcaio]

I'd rather not have a ton of inconsistent behavior in API 7 (like previous API versions) is all, really. I don't remember the inventory behavior being like this in the initial release of API 7, correct me if I'm wrong.

It was always like this for Containers so event.getTargetInventory()
not for player.getInventory()

thats what I call inconsistent

[codeHusky]

It's consistent with Minecraft, not necessarily the API itself. I prefer consistency with minecraft, or the capability to be able to be for user friendly behavior

[Faithcaio]

I must disagree. The API has to be consistent or it is worth nothing.

But yes we need easier ways to put stuff in inventories in a different order.
See the Issue Simon linked earlier.
So we could add API like this:
inventory.offer(stack, OfferStrategy.HOTBAR_FIRST)
And HOTBAR_FIRST would basicly be a predefined lists of QueryOperations:

[QueryOperationTypes.INVENTORY.of(HotBar.class), 
QueryOperationTypes.INVENTORY.of(MainPlayerInventory.class)]

Btw. the tools to do that now are already present:
inv.query(query1).union(inv.query2)
offering to that will offer to slots returned by query1 first then slots from query2.

I just can't clone myself. I've been working a lot on Ore the last week. Had no time to work on inventories.

[codeHusky]

query1 is what? If there's a workaround, it would be wonderful if someone posted a code snippet that restores default behavior <3

[Faithcaio]

Step 1. getHotbarFirst(player.getInventory)

public Inventory getHotbarFirst(Inventory inventory)
    {
        return inventory.query(QueryOperationTypes.INVENTORY_TYPE.of(Hotbar.class))
        .union(inventory.query(QueryOperationTypes.INVENTORY_TYPE.of(MainPlayerInventory.class)));
    }

Step 2. offer(stack) to it
Step 3. ???
Step 4. Profit

[clienthax]

This seems familar :)
https://github.com/SpongePowered/SpongeCommon/issues/939

[Faithcaio]

Back then we didnt think/check that the Player Inventory/Container are ordered differently in the Minecraft internals.
Which caused those really fun bugs where SlotIndex.of(0) was in the middle of the 9x3 Grid because the hotbar offsets it.

[githubl0ver69]

codeHusky is raising valid ponts, you guys need to quit gate guarding good ideas already.

[ryantheleach]

@githubl0ver69 If you read the second half of the issue thread you will see that

1. It's been said that viewing order needs to be kept consistent with the views you see for expected behavior.
2. That Husky has a valid reason for wanting it, but that Faith would rather it be implemented / exposed differently (perhaps with some sort of strategy) for a valid reason.
3. There is a valid work around.

This doesn't mean that people are 'gate guarding good ideas', and frankly a more positive/constructive comment would probably be more likely to get someone to work on it sooner.

[codeHusky]

I'll bring this up now, I suppose.

So, some weird stuff is going on with the player inventory. It seems that if you get the slot index of each slot, 0 starts from the left most hotbar slot and then it ticks up. Now, if you go for the first slot in the .slots() return, you get the upper left most inventory slot. I don't understand why for one, we're not offering to the lowest slot index possible at all times, and two why this behavior is there but mostly ignored.

[Cybermaxke]

@Faithcaio This issue got resolved with https://github.com/SpongePowered/SpongeAPI/pull/1800?