Describe the bug
If the feeding upgrade tries to feed you while using an inventorio tool the tool will duplicate a ghost copy into the currently selected hotbar slot, voiding whatever was there before.

To Reproduce
Steps to reproduce the behavior:

1. Equip the feeding upgrade (or advanced feeding upgrade) to any backpack and add food to the backpack.
2. Get to low enough hunger to trigger the feeding while mining something with any tool currently equipped in an inventorio tool slot.
3. The tool will overwrite whichever hotbar slot you currently have selected with a ghost copy of the tool.
   (i have not tested with segmented hotbar enabled in inventorio settings it may behave differently there)

Expected behavior
It should just feed you without the inventorio tool doing anything.

Screenshots
(ghost pickaxe)

Versions
minecraft 1.20.1
forge 47.1.3
sophisticated backpacks: 3.19.5.988
sophisticated core 0.5.113.524
inventorio 1.9.2

@P3pp3rF1y

I've had time to stew on this (and at this point in time, Inventorio is not my mod -- I'm tempted to try to get comfortable with Java & Minecraft's APIs and offer to assist over there though), and I partly agree & partly disagree.

but storing the storage in player's main hand

Let me try to clarify what I think Inventorio is doing here.

Inventorio introduces dedicated slots for the tools; it's not quite modifying any existing slots in the inventory, instead it's returning "hey, the main hand is currently holding the pickaxe tool from the pickaxe slot" (when it's appropriate).

The true core issue of the desynchronization is occurring because of that .selected property on the hotbar not being in sync with what's currently in the hand. I think if I could change out what that .selected property is returning, these issues could go away.

I'm not as comfortable in the Java Ecosystem as I am in other languages, so I'm unfamiliar with whether it's possible to use mixins to override a property (i.e. setting a getter in other languages). It didn't seem like this was a language feature when I learned Java 😅

If that's not possible, I'm sure I'll figure out a simpler, more primitive way to address the issue.

Because much of the discussion has already occurred here, and not on the other thread, I'm going to add some information here:

It appears that Sophisticated Core calls the following:

https://github.com/P3pp3rF1y/SophisticatedCore/blob/9eec6dd5bf0c35e56be92b2ff4a7057f53a91bce/src/main/java/net/p3pp3rf1y/sophisticatedcore/upgrades/feeding/FeedingUpgradeWrapper.java#L78

Meanwhile, Inventorio hijacks it here:
https://github.com/Lizard-Of-Oz/Inventorio/blob/0ca21343c7eb6772626f95fcdd71bd0ab156c08d/common/src/main/java/me/lizardofoz/inventorio/mixin/PlayerInventoryMixin.java#L36

Returning the display item instead of the actual item.

Inventorio has a function in the codebase that returns the actual item, but I agree with @P3pp3rF1y's take that the fault lies solely with Inventorio.

This bug could easily end up popping up in other mods.

I might see if I can apply a change, and what the consequences are for disabling the mixin :)

Edit: Disabling the mixin entirely is a no-go with how the mod currently works; it won't show the displayed item properly (figures). Will contemplate alternative approaches...

Meh; I attempted to write a mixin that would pick up on this interaction and replace the item appropriately, but then I realized that Sophisticated Core modifies the list structure directly, thus making it difficult to intercept.

player.getInventory().items.set(player.getInventory().selected, stack);

So I don't think I'll be able to do any post-processing mixins.

I wonder if there is a simple, conditional way to modify the GetMainHandStack mixin instead 😬

@P3pp3rF1y I've a request, and I can apply the PRs myself.

I've come up with 3 solutions to this issue:

Solution 1

Make a One Line Change to Sophisticated Core's FeedingUpgrade Class
Only modifies Sophisticated Core

ItemStack mainHandItem = player.getMainHandItem();
// => 
ItemStack mainHandItem = player.getInventory().items.get(player.getInventory().selected);

Rationale

This class only currently partially supports the mixin-supported APIs; other than fetching the main hand item, it is directly modifying the inventory storage list.

If we make it consistent by making it fetch from the inventory item list, this should continue functioning as intended, while also fixing this issue (I've tested this to make sure, I can record a video if you'd like)

Solution 2

An Ugly Hack on Inventorio's Side
Only modifies Inventorio

I... am not fond of this solution. I can shim in some code that checks if the call is coming from Sophisticated, and make the getMainHand call return the item in the inventory as Sophisticated expects.

Essentially doing something stupid like:

StackTraceElement[] st = Thread.currentThread().getStackTrace();
for (StackTraceElement ste : st)
{
    if (ste.getClassName().contains("sophisticated")) return;
    if (ste.getClassName().contains("Feeding")) return;
}

(Yes, I know this is horrible code; I'm only so invested in fixing the issue though.)

Solution 3

Modify both SophisticatedCore and Inventorio

This would be the alleged proper solution,

We can modify the FeedingUpgrade class in SophisticatedCore to strictly use the PlayerInventory APIs so that the mixins can intercept the calls appropriately for the modifications.

Then on Inventorio, I can code the mixins to intercept the modification calls, and ensure that it's not inserting the "ghost item".

Personally, I believe solution one might be the easiest 😅, would you be okay with it?

Nice, glad someone got a solution!

@TotalTechGeek
I mean making that small change in the first solution wouldn't be that big of a deal, but I couldn't promise it won't get changed back in the future because that's public API and the other thing is what are you going to do about any other mod that uses this public api and modifies the stack in some way?

Overall I feel like the way Inventorio has its storage implemented (at least I understand that it uses the stack in mainhand for storage) is really wrong. There are multiple ways this could be done, but storing the storage in player's main hand definitely isn't one if you actually want your mod to be compatible with any other mod that may modify the stack in player's hand.

It caused the problem with sweet berries, (alexsmobs) cooked catfish, golden carrots, and cooked porkchops. I mainly used sweet berries when investigating this bug more but I doubt its food item specific. I did forget to mention i have spice of life apple pie installed, maybe that caused it?

edit: it still happens when spice of life is removed

What food was the feeding upgrade feeding you when this happened?

I have an idea that could fix this, since I have gotten no response from the issue I made on Inventorio's github. You could add a config option for which hand the feeding upgrade puts the item in, if the item goes into the offhand instead of the main hand Inventorio wouldn't have an issue.

Sure but then very likely some other modded food would have an issue with eating from offhand and won't allow eating so no I am not going to make that change. Again Inventorio needs to fix this, they are making very intrusive changes into base code and this is the result of that.

Ok, I took a quick look at this. Feeding upgrade puts food in player's hand, then makes them eat it and finally puts back in their hand what they had in it before.
Inventorio does so many mixins that it would be way too difficult to figure out for me how this could break it and honestly it shouldn't really matter as player gets the item back in their hand immediately. So you will need to report this to Inventorio and they can figure out how to fix on their side as the fix on sophisticated would mean dropping support for different modded foods which require the food to actually be in player's hand when it's eaten.

Thanks, I'll report this to inventorio instead. Also this isn't exactly a non issue because it can overwrite an item you're holding with the ghost inventorio tool, voiding the item.

I am not really suggesting it's not an issue, but it's an issue that needs to be fixed on inventorio side most likely as I don't see how placing back item into player's hand that was in there before could cause this unless the mod does some really funky logic in a response to that. Hence Inventorio should take a look at this

sure