CyclopsMC / IntegratedCrafting

Craft stuff in Integrated Dynamics networks
MIT License
7 stars 6 forks source link

[Bug]1.16.5 Cross-compatibility mod bug. When using autocrafting and removing a single item, the crafter starts a new crafting recipe #74

Closed MisterObvious25 closed 2 years ago

MisterObvious25 commented 2 years ago

Issue type:


Short description:

This is a cross compatibility bug, not sure if its a bug in integrated crafting or in mouse tweaks.

I tried this in the most recent build and noticed this issue. When i have for example 50 planks in the network, and remove 1 plank (by scrolling with the middle mouse button) , a craft is started to create 4 more planks. For each plank i remove (by scrolling with the middle mouse button) , a new crafting job is started to create 4 planks. If i'm correct this is not the intended behavior? Setup is as in #72.

This is only the behavior when the de removal of the item is done with the middle mouse button(scrolling). When removing for example half a stack with the right mouse button the crafting does not occur

image

  1. Use setup as above
  2. open chest
  3. scroll 1 plank out of the chest with middle mouse button
  4. crafting writer starts crafting 4 more planks

Expected behaviour:

Autocrafting only starts when the item is no longer in the network

Versions:

Common Capabilities kroeser CommonCapabilities-1.16.5-2.8.0.jar

Cyclops Core kroeser CyclopsCore-1.16.5-1.12.0.jar

Integrated Crafting kroeser IntegratedCrafting-1.16.5-1.0.19.jar

Integrated Dynamics kroeser IntegratedDynamics-1.16.5-1.10.3.jar

Integrated Terminals kroeser IntegratedTerminals-1.16.5-1.2.8.jar

Integrated Tunnels kroeser IntegratedTunnels-1.16.5-1.8.7.jar

Mouse Tweaks YaLTeR MouseTweaks-2.14-mc1.16.2.jar

Log file:

rubensworks commented 2 years ago

Thanks for reporting!

rubensworks commented 2 years ago

That sounds really weird. Could you enable the logChangeEvents flag in the ID config file, to see what item changes are being detected by ID in the chest when you're scrolling?

MisterObvious25 commented 2 years ago

Hi mate, Thanks for the quick reply. Here are the requested logs

Enabled the logChangeEvents flag and did some testing. below the log

Log when scrolling out of a chest connected to the network with less then 1 stack of planks LogWhenScrollingOutOfChest.log

Log when scrolling out of a chest connected to the network with more then 1 stack of planks(4 stacks in this case) ChestHas4StacksTake10outByScrolling.log

Log when scrolling out of a ID Terminal LogWhenScrollingOutOfIDTerminal.log

It seems like the following

  1. when scrolling 1 out of the chest it generates a "deletion" event for the stack.
  2. at the same time i generates a "addition" event for stack -1
  3. in the split second between deletion and addition, a crafting job is started because the system thinks stock on the network is 0.

Hope this helps

rubensworks commented 2 years ago

Thanks for the logs!

So it looks like Mouse Tweaks may be taking the full stack out of the chest, modifying it, and placing the remainder back in the chest. But it looks like this process can span multiple ticks, which causes ID to detect item deletions, and additions later on.

AFAICS, there's no way for me to fix this on the side of ID (except for delaying change detection, but this will cause more problematic delay issues elsewhere). (I however add a customizable delay option to the crafting writer though)

@MisterObvious25 Could you open an issue on the MT issue tracker and link back here? Because it seems to me like the MT process could be isolated within single ticks (instead of spanning multiple ticks as it does now), so that other mods (like ID) detect no unneeded changes.

MisterObvious25 commented 2 years ago

Alright, Posted it on the MT issue tracker with a link back to this issue. Thanks for the help!!

YaLTeR commented 2 years ago

Hello, I make Mouse Tweaks. I haven't looked into the issue in detail yet but:

So it looks like Mouse Tweaks may be taking the full stack out of the chest, modifying it, and placing the remainder back in the chest.

Yeah, that sounds like what Mouse Tweaks usually does.

But it looks like this process can span multiple ticks

Mouse Tweaks calls GuiContainer.handleMouseClick() for every click, it all happens in one go on the client but it can likely span multiple ticks server-side. I don't think there's any way to do the clicks atomically from the client, so they are guaranteed to happen in a single server-side tick?

Mouse Tweaks also has special handling for crafting output slots where it doesn't put the items back (because it's impossible). This could be extended to IC, e.g. it could implement the Mouse Tweaks interface and override the "is crafting output slot" method. Although I have to confess that I'm not aware of anyone trying to implement this interface yet, so it might not actually work all that well...

rubensworks commented 2 years ago

Thanks for the quick reply @YaLTeR!

it all happens in one go on the client but it can likely span multiple ticks server-side. I don't think there's any way to do the clicks atomically from the client, so they are guaranteed to happen in a single server-side tick?

What I assume MT is doing, is something like this:

(Please correct me if I'm wrong...)

What could be done instead (which would resolve the issue here), is something like this:

In the second approach, the changes are applied immediately to the container, and ID/IC will detect no incorrect item deletions.

Mouse Tweaks also has special handling for crafting output slots where it doesn't put the items back (because it's impossible). This could be extended to IC, e.g. it could implement the Mouse Tweaks interface and override the "is crafting output slot" method.

The problem of the issue here is that the problem does not directly occur within an ID/IC container, but in a vanilla chest. and ID is merely detecting changes that cause IC to trigger a crafting job (because an item is temporarily missing).

YaLTeR commented 2 years ago

What I assume MT is doing, is something like this:

  • Player starts scrolling
  • MT sends packet to server to take stack out of container
  • Player stops scrolling
  • MT calculates total scrolling amount and sends packet to server to place remainder of stack back in container

(Please correct me if I'm wrong...)

So, MT is client-side only. When the player scrolls the wheel (usually you get a single scroll wheel "click" on a given frame because the game's FPS is usually higher than how fast someone can scroll their mouse wheel) MT does the following actions:

  1. left click the selected slot (to pick up the items)
  2. right click the target slot (to put 1 item there)
  3. left click the selected slot again (to put the remaining items back)

It does this using GuiContainer.handleMouseClick(), which is pretty close to real mouse clicks. That is to say, MT doesn't manipulate the items or send any packets directly, it literally emulates mouse clicks.

MT immediately calculates the size of this scroll, and sends a single packet to the server that does the following (server-side):

  • Decrement stack in container by X
  • Increment stack in player container by X

Yeah, that would be the server-side approach, however I would like to keep MT client-side-only, because this way it works with the widest range of custom containers, and also this way it is impossible for MT to introduce item dupe or similar issues. E.g. cpw's Invertory Sorter, which works server-side, has seen several dupe issues with custom containers.

The problem of the issue here is that the problem does not directly occur within an ID/IC container, but in a vanilla chest. and ID is merely detecting changes that cause IC to trigger a crafting job (because an item is temporarily missing).

Oh. Well, that makes it more complicated. Maybe there's some sort of atomic way to handle clicks in Minecraft or Forge after all that I'm not aware off? So all clicks in a "transaction" are guaranteed to be handled within a single server tick.

rubensworks commented 2 years ago

Hmm, I see. I definitely get the point of keeping things client-side.

Maybe there's some sort of atomic way to handle clicks in Minecraft or Forge after all that I'm not aware off?

There's no such thing indeed AFAIK.


So it looks like this behaviour is a consequence of how things work in MT (and likely for other potentially similar mods as well), and there's no way around that.

The only possible resolution I see is to add acustomizable delay option to the crafting writer (set by default to a low value, e.g. 5 ticks). This would mean that when the crafting writer detects a missing item, it will wait for this amount of ticks to be sure that the item doesn't quickly pop up later.