Add packet-based GUI backend - #1
Open
Keviro wants to merge 50 commits into
Open
Conversation
There was a problem hiding this comment.
Pull request overview
This PR introduces an internal “packet GUI” backend for the Bukkit platform, intended to render chest-style GUIs using PacketEvents (with a fallback to standard Bukkit inventories when unavailable or unsupported).
Changes:
- Adds a
GuiBackendabstraction with a default Bukkit implementation and an optional PacketEvents-driven packet backend. - Introduces packet GUI session/render/click tracking to open/update/close GUIs via packets.
- Adds PacketEvents as an optional (soft) dependency and updates build configuration to include the required repository and version catalog entry.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
- add handling for closing inventory sessions based on user actions - improve session management by ensuring proper windowId checks - update session inventory state after closing to reflect changes
- implement normalization for item display names and lore - utilize reflection to access ItemMeta properties - ensure non-italic display names for better consistency
Adds coalesced SetSlot-based updates and per-session item conversion caching so normal GUI interactions no longer rebuild and resend the full window. Keeps full WindowItems only for open/reopen/hard repair paths, preserves Folia-safe scheduling, and maintains PacketEvents as the packet API. This massively reduces repeated ItemStack conversion and fullResync overhead during clicks, pagination, and dynamic shop GUI updates.
Handle QUICK_MOVE, SWAP, and CLONE click actions in the packet GUI backend while keeping unsafe drag/drop/double-click actions cancelled and repaired. Shift and keyboard-style clicks now enter the normal click pipeline and trigger a hard repair to avoid client-side ghost item movement.
Handle server-side close packets without sending an extra close packet or forcing updateInventory, preventing packet GUI cleanup from immediately closing dialogs opened from click handlers.
Force a post-click repair for normal left/right PICKUP clicks so client-side prediction cannot leave fake GUI items missing or stuck on the cursor. Keeps shift, swap, clone, and unsafe click repair behavior intact.
Ensure offhand swap clicks repair the client-side offhand slot and cursor so fake GUI items cannot appear as ghost items in the player's hand.
Author
|
Die neuste Version ist auf dem Dev Survival Server zum Testen |
Replace normal-click full window repairs with targeted slot/cursor repair scopes, reduce full WindowItems usage, and simplify packet item caching to avoid expensive render hashing.
Avoid expensive full window item resyncs during normal GUI clicks, remove debug toString calls from hotpaths, and batch packet writes through a guarded send plan.
Build packet GUI render/repair plans on the player scheduler, but move the expensive Bukkit-to-PacketEvents item conversion onto dedicated worker threads. This keeps GUI state access Folia-safe while reducing region thread load from PacketItemConverter/SpigotConversionUtil during packet GUI renders and click repairs. Async conversion is enabled by default and logs a warning when disabled.
Use native outbound inventory packets for packet GUI rendering so surf-api PacketLore can process clientbound GUI items, while falling back to the Bukkit backend when the current Minecraft version is not explicitly supported.
…ent accessors usable Removes the packet-item cache in PacketGuiSession (154 lines), which had no caller since the native sender converts Bukkit items to NMS directly, along with PacketItemConverter#toBukkit, PacketGuiRender#rawBukkitItem, the one-argument PacketGuiRender#from, PacketGuiSession#currentRender and PacketViewContainer#changeBaseTitle. Moves the GUI-window slot mapping into PacketInventoryConstants so the four copies that existed become one, and makes applyPlayerInventoryItem symmetric with playerInventoryItem: armour and offhand can now be written, and an unmappable slot is logged instead of silently dropped. The native sender caches its send method against the Packet supertype, so a single cache entry serves all four send paths. forceNonItalic no longer recurses into child components; children inherit the root decoration, so rewriting them destroyed italics a view had set on purpose. Also reverts the throwing overrides of getInventory/getCursor/setCursor on the synthesized InventoryClickEvent. Keeping the event fully usable is deliberate so plugins written against the Bukkit backend need no rewrite. The accessors resolve against the player's own view; that is now stated in the javadoc together with a warning not to write through them from a packet GUI, instead of the previous claim that they were routed to the packet click origin.
An outside click has two wire forms and the backend only accepted one of them, so clicking outside a packet GUI did nothing at all. PICKUP on a negative slot is what a client sends while holding an item; THROW on a negative slot is what it sends with an empty cursor. A packet GUI never puts anything on the real cursor, so in practice only the THROW form ever arrives - exactly the form that was being rejected. The negative slot stays the discriminator. THROW on a real slot is the drop key and remains denied, and drag (QUICK_CRAFT) carries a negative slot as well but is still never routed, so a single drag cannot run the click pipeline twice. Outside clicks now report ClickType.LEFT/RIGHT rather than DROP, matching the Bukkit backend, where SlotType.OUTSIDE carries that distinction instead of the click type. Also adds -Dinventory-framework.gui-backend.debug-clicks=true, which logs every inbound click with its slot, button, click type, computed repair scope and routing decision. That switch is what identified this bug on a live server.
`cmd /c gradlew.bat` resolves against PATH rather than the task's workingDir, so the delegating shadowJar and publish tasks failed with "gradlew.bat is either misspelled or could not be found". They now invoke the wrapper by absolute path. Adds publishToMavenLocal to the same set, so a branch build can be installed into ~/.m2 and consumed by surf-api, which shades this fork instead of depending on it at runtime. docs/testing-with-surf-api.md describes that round trip and why a composite build is the wrong tool for it.
Picks up the only substantive change from ced7b1c on origin/packet-guis, which is otherwise a whole-file reindentation of SlotClickContext.java from spaces to tabs. That reindentation is not merged: it contradicts the palantir format the project applies everywhere else and turns an eighty-line change into a four-hundred-line diff, which is exactly what makes the click-origin refactor hard to review.
…flection The backend reached the entity scheduler through its own reflection layer, so the module carried two unrelated Folia strategies that had to be kept in sync by hand: this one and the FoliaLib instance BukkitElementFactory already uses. FoliaLib covers every case the reflection did, and exposes the failure signal natively. isOwnedByCurrentRegion now goes through PlatformScheduler, which resolves to a real region check on Folia and to Server#isPrimaryThread on Paper and Spigot, so the "method absent" and "invocation failed" branches disappear along with the cached Method handles. The inline fast path stays: FoliaLib always defers, while several call sites depend on the work having happened by the time they return - open() publishes the session before rendering it, and closeSession runs the close pipeline synchronously. Scheduling failures keep their meaning. runAtEntityWithFallback and the retired callback of runAtEntityLater both discard the session, and a null WrappedTask from the next-tick path does the same, so a dropped render task cannot leave the render request latched.
… opened Opening a GUI while another one was already open sometimes produced no window at all - the command had to be issued several times. The window id was allocated each time, so open() ran; the session was torn down again right after. handleInventoryClosePacket ignored the window id entirely, so any outbound ClientboundContainerClose for the viewer finalized whatever session was registered. Publishing the new session before closing the previous one, which the re-entrancy fix introduced, put that new session in the map exactly while the previous view's CLOSE pipeline was running - and that pipeline is arbitrary developer and framework code which routinely ends up calling Player#closeInventory. Its close packet then finalized the session that was still being opened. open() now marks the viewer while it runs, and close packets seen during that window are ignored: they are caused by our own teardown, not by the viewer. Independently, the handler only reacts to a close for its own window or for the player container - the latter being what Player#closeInventory emits, since the server never knows about the fake window. A close for any unrelated window no longer touches the session.
PacketViewerInventory kept a 46-slot copy of every viewer's inventory plus their cursor, fed from intercepted outbound packets. Nothing ever read it: the bottom rows of a GUI are rendered from live Bukkit reads. It was kept because of a rule that live reads would break surf-api's PacketLore. A test on the server disproves that: the bottom rows go through the native outbound sender, i.e. the same path as vanilla, and an enchanted item in the viewer's inventory shows its enchantment lore inside a packet GUI, decorated exactly once. Making the mirror authoritative would have been the wrong direction anyway - its contents come from packets PacketLore may already have decorated, so resending them risks decorating twice. What is left is PacketViewerWindowTracker, which only remembers the id of the real container window so PacketGuiWindowIds never hands out a colliding fake id. The per-slot repaints stay; they are live behaviour and read from Bukkit, not from a mirror. The listener now skips decoding outbound inventory packets for viewers without a session, so players who have no GUI open stop paying for the interception at all. With the mirror gone, PacketItemConverter no longer converts to PacketEvents items either, which removes the last use of SpigotConversionUtil.
Rendering built a plan of conversion operations, then walked those operations to build a second plan of outbound packets, then sent that. Both phases ran on the viewer thread in the same tick, so the split bought nothing - it was left over from an earlier attempt to convert items off-thread. It cost something, though. The two plans were the same (channel, windowId, generation, List) value object written twice, and canConvertPlan and canSendPlan were byte-identical five-clause guards forty lines apart, so a fix applied to one would silently miss the other. The phases had already drifted: addWindowItems keyed its packet on the plan window id while addPlayerSlotRepairs used the session one. There is now a single PacketGuiSendPlan that collects outbound packets as the render is built, and a single canSend guard, re-checked before each packet exactly as before. Item copies happen while building rather than during a separate conversion walk, which is the same thread and the same tick.
Fills in the twelve checklist rows that were actually exercised on SurfCanvas (Minecraft 26.2) while bringing this branch up, and names the source for each: some were watched on screen, some read off the debug-clicks log. The remaining fourteen rows are left empty on purpose rather than assumed. Two of them matter before this is called done - nobody has yet confirmed that no GUI display item can end up in a real inventory, which is the claim the whole backend exists to make, and the window id collision guard has only been unit-tested, never run end-to-end against a real chest. Also reorders the activation section: the sentence explaining what native=off does had ended up behind the debug-clicks block and read as if it described that instead.
…cursor fix Closing a GUI sends a cursor correction and then a close packet. The correction went through the native connection, which queues, while the close was written straight to the channel by PacketEvents. With a backed-up queue the close can overtake the correction, and the client then applies it to its own inventory menu - reinstating exactly the ghost item the correction exists to clear. ClientboundContainerClosePacket takes nothing but the window id, so the native sender can build it without any chat-component conversion. It is resolved optionally and covered by the startup self-check; if a server does not expose it, the PacketEvents path is still used. The open-screen packet deliberately stays on PacketEvents. It carries a title component, so sending it natively would mean converting Adventure components to NMS ones, and it has no ordering hazard to fix: the content packets that follow it are queued behind the connection and cannot overtake a direct channel write.
The version was still 1.0.4, the same value master carries, so merging would have republished an existing release coordinate with entirely different contents - either failing the release job or silently handing every consumer a different 1.0.4. Nothing verified pull requests either: publish.yml only triggers on pushes to master, so the packet GUI backend and its tests had never been built by anything but a workstation. The new workflow applies the patches, runs the target repository's tests through a delegating task, and fails if makePatches would produce a diff - a patch that no longer round-trips is the one mistake this fork layout makes easy.
The allocator cycled 1..127 and only refused the one real window id the viewer had open at the moment of allocation. Vanilla allocates real container ids from 1..100, so the ranges overlapped almost completely and the guard covered a single instant. The unprotected case was a real container opening while a packet GUI was already on screen. If the server's counter landed on the GUI's id, isGuiWindow() claimed the packet as the backend's own, handleExternalInventoryOpen never ran, the session stayed open on a window the real container now owned, and every click in that container was cancelled and routed into the stale GUI pipeline - a container the player could neither use nor escape. Fake ids now come from 101..127, which vanilla never hands out and which still fits in a signed byte. The forbidden-id skip is kept as defence against another plugin's fake window in the same range.
…doned sessions openingViewers is a set, so a nested open() - which the close pipeline can trigger, since opening a view from onClose is a normal pattern - removed the marker in its own finally block while the outer invocation was still running. The remainder of the outer close pipeline then executed unguarded, and any Player#closeInventory it performed killed the session that had just been opened. That is the same failure 39a19c8 fixed, one level of nesting down. Only the outermost invocation lifts the marker now, and it is taken before closeRealInventory so it covers the whole open sequence. abandonSession dropped a live view at FINE level, which no production server logs, and never released the viewer, so the view kept listing someone who had no window and would never get one back. It now warns and detaches the viewer from the root. The CLOSE pipeline stays unrun on purpose: it executes developer code, and this path exists precisely because no thread will accept work for that viewer any more.
PacketSlotClickEvent exists so that consumers written against the Bukkit backend keep working unchanged, and it got the two most-read accessors wrong. getSlot() returned the view-wide raw slot. Bukkit reports the index inside the clicked inventory, so a bottom click on a 54-slot GUI produced 88 where Bukkit produces 7, and player.getInventory().getItem(event.getSlot()) - the most common thing such a consumer does - read the wrong slot or threw. getClickedInventory() returned null for every click, and null is Bukkit's canonical "clicked outside the window" signal. The usual `if (getClickedInventory() == null) return;` guard therefore discarded every packet GUI click, and comparing it against the player inventory threw. Bottom clicks now report the viewer's inventory; top clicks keep returning null, because there genuinely is no Bukkit inventory behind them. An outside click also reported InventoryAction.PICKUP_ALL while its slot type said OUTSIDE. A packet GUI never fills the real cursor, so NOTHING is both truthful and what Bukkit reports for the same click.
…e window tracker honest The mirror pass sent one slot packet per mirrored slot - 36 of them for every player#updateInventory that happened while a GUI was open, which the backend itself performs on close and other plugins call routinely. It now sends the single container content packet that path already had available. PacketViewerWindowTracker only ever learned about a real window closing when the server initiated it. Vanilla does not echo a close packet for a window the client closed, so pressing escape on a chest left the tracker naming a window that no longer existed. The inbound close packet is already intercepted, so the non-GUI branch now clears the tracker there. Also drops a line in handleWindowClose that could never fire - it asked the tracker to close the fake GUI id, which the tracker never holds - and removes the session monitor from the accessors of immutable fields, which netty threads read on every inbound packet.
The latch and the repair merging were the most state-machine-shaped code in the backend and the only part of it with no tests: a lost update there shows up as a window that silently stops matching what the view thinks it shows. They could not be tested where they were, because PacketGuiSession needs a viewer, a PacketEvents user and a final PacketViewContainer that Mockito 4 cannot mock. PacketGuiRenderRequests now owns that logic and needs nothing but the top size. The session delegates to it. Ten tests pin the behaviour the rest of the backend relies on: the latch only schedules once and reopens after a consume, clear() releases it without discarding the pending frame, flags only ever merge upwards, a full resync drops targeted repairs, and slots outside the window are ignored.
…ange The bottom-click write-back was described as opt-in, requiring setCancelled(false). The click context starts uncancelled, exactly like the Bukkit backend, so it is opt-out - a handler that changes currentItem for display purposes only has to cancel, and the old wording would have led someone to destroy an item. Also records what the synthesized InventoryClickEvent now reports for getSlot() and getClickedInventory(), why fake window ids live in 101-127, and that a discarded session is a WARNING that means onClose did not run.
Sending one container content packet instead of 36 slot packets was not an equivalent optimization. The protocol has no "set these slots" form: a content packet addresses the whole window, so the client redraws the top rows as well. A refresh that was meant to repaint only the viewer's own rows came out as a full window redraw. Reverts only the mirror change from 846d399; the window tracker fix, the dead line removal and the accessor cleanup in that commit stay.
InventoryFramework builds a new context and a new container on every open, including when a view reopens itself to refresh. The backend answered each of those with a freshly allocated window id and an open-screen packet, so the client tore the screen down and built a new one: cursor back to the centre, the whole window flashing, for what the view intended as an in-place update. In surf-shop the refresh button made that obvious - every click produced a new window id. A new session now takes over the window of the one it replaces when the row count matches, adopting its applied render and container state id. renderSession then diffs against what the client is actually showing and sends only the changed slots; a different title or row count still fails sameWindow() and produces a real reopen. Outbound close packets are dropped while a viewer is inside open(). They were already ignored, but ignoring is not enough once no open-screen packet follows: the client does not check which window a close belongs to, it closes whatever is on screen.
…ming The guard was set and cleared on the viewer's thread but is read on netty threads. A close packet emitted during open() - by closeRealInventory, or by the replaced view's close handlers - is written from the viewer's thread and observed on the connection's event loop, because the server defers sends made off that loop. Clearing the marker in a finally block let that observation land after the marker was already gone, and the close was then read as the viewer closing the window that had just been opened. That race was survivable while every open ended in an open-screen packet, which put the screen back. Window reuse removed that safety net, so a miss now loses the GUI outright. The marker is released through the same event loop instead. The loop is FIFO per channel, so everything the open sequence queued is observed first - the ordering holds by construction rather than by luck. Disconnect, quit and unregister clear the marker too, because a channel that dies mid-open would otherwise leave it behind, and a stuck marker silently drops close packets. abandonSession also removed whatever session the viewer happened to be on rather than the one whose task was refused. Scheduler fallbacks fire asynchronously and can arrive long after the task was queued, so this could destroy a live window and detach the viewer from a view it had just opened. It now takes the session and removes it conditionally, the way closeSession already did. Call sites without a session log instead of guessing at one.
…es not own toNmsItem normalized every item it converted, and it converts everything the backend sends - GUI icons, but also the viewer's own items in the bottom rows, on the cursor and in player-inventory slot updates. Styling items the backend does not own is not the transport layer's job, so it now converts verbatim and the normalization happens where GUI icons are produced, in PacketGuiRender#bukkitItem. No behaviour change was observed from this. An anvil-renamed item renders italic inside a GUI both before and after, so the case this was expected to fix - a name that leaves its italic state unspecified, which vanilla renders in italics - does not appear to reach the normalization on a live server. Why is not established; the reflection resolves (adventure.text is not relocated by the shaded build), so the likeliest explanation is that Paper does not hand out such a name with the state unspecified in the first place. Recorded as a hypothesis, not a finding. What is left is the structural argument plus one measurable saving: the clone/getItemMeta/setItemMeta round trip no longer runs for the 36 mirrored slots on every player#updateInventory while a GUI is open. Six tests pin the distinction the normalization rests on: an unspecified italic state is pinned to false, an explicit one - either way - is returned untouched.
…send Two gaps in the same promise: a mismatch is supposed to warn once and leave every GUI on real Bukkit inventories. register() and the native sender's initialize() caught only RuntimeException, but both resolve NMS classes and run their static initializers. A class that is present yet cannot be linked raises NoClassDefFoundError, NoSuchMethodError or ExceptionInInitializerError - all Errors, all escaping the catch, aborting the owning plugin's enable rather than falling back. The presence probe in GuiBackendFactory uses Class.forName without initializing, so this is reachable whenever PacketEvents is on the classpath but broken. Both now also catch LinkageError, which is exactly that family; Throwable is deliberately not caught so OutOfMemoryError still propagates. The send path was never probed at all, because reaching a player's connection needs a live player - so a server whose connection field or send method does not match reported "Packet mode enabled" and then failed every single GUI open, forever, with the Bukkit fallback already discarded. selfCheck now resolves as much of that chain as it can statically and reports what it cannot, without refusing the server over it: an obfuscated runtime may name the field differently and still work through the field-scanning fallback. If the chain does turn out to be unusable, the first failure now disables packet mode and releases every open session back to real inventories, instead of retrying per GUI. A dedicated exception type keeps that apart from a single packet failing to build, which still only takes its own session down.
…ong thread Three findings on the same theme - work happening where it should not. detachAbandonedViewer mutated the context's viewer list, the framework's global viewer map and the root's context list inline. Those are plain collections the main thread iterates, and the method is reached from a scheduler fallback that can fire on a netty thread. It now hops to the global tick thread, which is where framework state belongs - not the viewer's own scheduler, since that one refusing work is what led here. open() returned without doing anything when PacketEvents had no user for the player. By then IF has already registered the viewer with the framework and the context and committed the container, so the viewer stayed bound to a window that did not exist and that nothing could finalize: no close packet, quit handler or external open reaches a session that was never created. The likeliest cause is a connection PacketEvents has not registered yet, so it retries once on the next tick and releases the view properly if that still fails. The inbound close packet for a GUI window was force-uncancelled, which overrode whatever another listener had decided and let vanilla run its close handling for a container the server does not have open - firing an InventoryCloseEvent for the player's own inventory on every packet GUI close. It is now cancelled, like the click packet next to it.
…acket GUI The observable consequence of cancelling the inbound close packet: a plugin listening for InventoryCloseEvent will not hear about a packet GUI closing. A view's own onClose is unaffected. Also notes what the event used to describe, since it was not the GUI - vanilla ran its close handling against the player's own inventory menu, so anything that relied on it was reacting to a misleading event.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This pull request adds an optional packet-based GUI backend for InventoryFramework.
The goal is to render GUI contents as virtual packet items instead of placing real items into Bukkit inventories. This makes GUI item rendering safer against duplication issues while keeping the existing InventoryFramework API unchanged for consumers.
PacketEvents is used as the packet abstraction layer, with the Bukkit inventory backend remaining available as a fallback. The packet backend is designed to work on the current Paper/SurfCanvas/Folia runtime and keeps player-facing inventory state synchronized through packet updates.
Motivation
Inventory GUIs should not need to store their visual items as real server-side inventory contents. Rendering them through packets allows the framework to display the same GUI items to players without making those items physically exist in the server inventory state.
This improves safety for GUI-heavy plugins and provides a cleaner backend model for future inventory rendering behavior.
Notes