TE raytracing result can have incorrect matches on remote servers

[VADemon]

Concerning this piece of logic: WDLEvents.java#L181-L184

Since you (Pokechu) only recently started looking at #78 I think you too have no PoC fix for this one and I'd like to discuss it.

You can watch the video instead of reading the first block below: https://www.youtube.com/watch?v=NxR2eNvtCco

For anyone uninvolved, here's the run down:

• There's no "inventory data received for chest at XYZ" event or alike in Minecraft, so the code relies on just receiving the inventory data and trying to guess the position it received this data for.

Replicate with:

1. Move mouse at a high speed
2. Press Right Click while hovering over Chest1
3. <Server + network lag takes time to send WindowOpen packet>
4. At the time of receiving WindowOpen, your mouse is already pointing at Chest2 client-side
5. Close the inventory window
6. WDL will assume the window and inventory data belong to Chest2, although the server sent the data for Chest1
7. Chest1 inv ends up saved into Chest2

This is the worst-case outcome. The "less worst-case" is when you point at a non-TE block client-side and the data simply ends up not saved (and you see the "onItemGuiClosed could not get TE at Pos[]" message if you enabled messages at all)

All of the above is worsened by network lag (100ms, jitter) and a server lagging behind on TPS (could be as simple as STW of GC).

Tested on (litemod): litemodwdl-4.0.3.1-mc1.12.2 and litemodwdl-4.0.6.2-mc1.12.2 (has different error messages)

Discussion:

The irony is, I actually knew of the window/inventory/block relation problem and wanted to see if you had solved it somehow in a fancy way only to find it's not on the radar at all (searched in issues and online).

So what are the feasible improvements to this problem? Are there any at all?

From a Vanilla point of view, there's a set set of blocks for which you know they are openable containers and you await inventory windows from them. Theoretically we can track client-side Right Clicks ("RMB" from now on) in a queue to correlate to incoming OpenWindow packets

For modded/servers this breaks. I think it's possible to have some stone block contain a virtual inventory (Bukkit) and modded is a wild west anyway.

Speaking of virtual inventories - if a virtual inventory is invoked by a command (usual for a Bukkit API plugin), will WDL try to match that inventory to a random raytraced block when it receives the packet?

Raytracing at the time of clicking RMB ain't better (even if you start adding logic to correctly enqueue/dismiss events): Paper allows you to skip Ocelot checks sitting on top of chests. You just can't handle that correctly client-side.

---

I understand this one is tough to improve and asking you for input to start a discussion.

[Pokechu22]

There really isn't a good way of fixing it.

IMO, the simplest improvement would be recording the location of the last right-clicked block, instead of the block that was being targeted when the inventory appeared. The same logic would then apply of only using that location when the inventory is closed. Adding a queue into it would just make things more complicated, especially since it could desync in the cases you mentioned.

With regards to virtual inventories: see https://github.com/Pokechu22/WorldDownloader/issues/150#issuecomment-571828724 - yes, if you're looking at a chest when one is opened, then it'll be saved into that chest; this is a semi-feature. In previous versions, weirder stuff would happen (in particular, if the ray trace didn't hit anything, it'd save into the last chest you did look at, or crash), though I think that was restricted to 1.14+ or so (due to changes in how the ray tracing worked).

IMO, saving into the wrong chest is the worse of the two cases; failing to save with an informative message is preferable. Both the container close info and warning messages are enabled by default, specifically to make spotting something like this possible.

[VADemon]

IMO, the simplest improvement would be recording the location of the last right-clicked block, instead of the block that was being targeted when the inventory appeared. The same logic would then apply of only using that location when the inventory is closed. Adding a queue into it would just make things more complicated, especially since it could desync in the cases you mentioned.

No this can't work this easily, though it's only true for pathological cases. Like during https://bugs.mojang.com/browse/MC-140507

When the server is lagging for a prolonged period of time, you can manage to click multiple chests in a row before the OpenWindow is received. These packets are basically queued: FIFO - oldest click is received first, o+1 is r+1 ... That's why relying on just latest right-click would also break - you would relate the newest click to the oldest packet (and dismiss the rest or overwrite the last block multiple times)

At this point I wanted to say how hit-and-miss the chat messages were, but TD and you together brought me to an idea:

Lets compare client-side Right Click to Raytrace result at the time of OpenWindow:

• If the results are equal, we know that the cursor is still pointing at the right block/TE ✔
• If the results mismatch, we know we are pointing somewhere else ✔
  • You can choose now whether try to guess which block that inventory packet belongs to
  • Or choose to dismiss the result with the informative chat message ✔

Eventually the user will read the error messages ("Chest not saved, looking at a different block") and learn to slow down his Quake-aim to successfuly save chests.

The right click event should be timed (time-limited to ~5s) to account for possible lag, but to exclude issues like virtual inventories (if desired). That is, if OpenWindow is received within 5s of last RMB click, it probably still does belong to that clicked block. If more time has passed - it's probably a virtual inventory/something else.

[Pokechu22]

Yeah, good point. I've run into that case with super laggy servers before, but I assumed the user would notice (since I generally do when I run into that). But it silently saving multiple inventories into the same chest is a problem, and I guess a queue would fix that. I still don't think trying to reconcile data when it needs to be queued is a good idea (far too easy to mix it up).

The other thing that complicates matters with laggy situations is that either the user is closing the GUI for each queued chest as it appears, or is letting them all appear at once over each other. I'm not sure if the server sends close window packets before open window packets in the second case, and all around it seems kinda fragile.

One other thing that would be possible is making use of the block action packet — that's what controls when the chest animates open or shut (and WDL already listens to it for note blocks). It'd still be slightly fragile to use it to determine what chest is open (especially with multiple players nearby) but could be used as additional confirmation. Of course, only chests and trapped chests have the opening animation, so it wouldn't work for e.g. furnaces or whatever.