Repository navigation
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
The rewrite drops the Spigot fallback and failure/history handling in NMS entity-spawning code and adds an unbounded static UUID set, and its runtime correctness cannot be fully verified here, so it needs human review.
Pull request overview
This PR fixes copy/pasting of entities that carry passengers (e.g. a pig riding a cow, an item_display on a boat) in FastAsyncWorldEdit, targeting the 26.2 NMS adapter. Previously passengers were dropped and an "Unknown entity tag" error was logged; pasted riders also broke //undo. The change reworks entity spawning to use EntityType.loadEntityRecursive (which preserves passengers), rewrites passenger UUIDs to avoid collisions, and makes entity removal recurse into FAWE-created passengers so //undo cleans them up without deleting unrelated riders.
Changes:
- In
PaperweightGetBlocks, spawn entities vialoadEntityRecursiveand add them (with passengers) throughaddFreshEntityWithPassengersor a new no-events recursive add helper. - Track FAWE-created passenger UUIDs in a static set and add
removeEntityRecursiveso//undoremoves pasted passengers correctly. - In
IChunkExtent.createEntity, returnnullinstead of NPE-ing when the entity NBT isnull.
File summaries
| File | Description |
|---|---|
worldedit-core/.../queue/IChunkExtent.java |
Guards against null entity NBT by returning null early (consistent with the nullable Extent.createEntity contract). |
worldedit-bukkit/adapters/adapter-26.2/.../PaperweightGetBlocks.java |
Reworks entity paste to preserve passengers, rewrites passenger UUIDs, and adds recursive add/remove helpers; introduces a static UUID-tracking set. |
Review details
Suppressed comments (3)
worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightGetBlocks.java:718
- The failure handling that previously guarded entity spawns has been lost. In the prior code, a failed add (
!nmsWorld.addFreshEntity(...)returningfalse) invokedonError.run()anditerator.remove()with the note "Unsuccessful create should not be saved to history" (seeadapter-1_21_9/.../PaperweightGetBlocks.java:704-708). NowonError/iterator.remove()only run whenloadEntityRecursivereturnsnull;addFreshEntityWithPassengers(andaddRecursiveNoEvents) results are not checked, so entities that fail to spawn are still persisted to//undohistory. Consider detecting add failures and removing them from history as before.
if (!set.getSideEffectSet().shouldApply(SideEffect.ENTITY_EVENTS)) {
addRecursiveNoEvents(createdEntity, nmsWorld);
} else {
nmsWorld.addFreshEntityWithPassengers(createdEntity, CreatureSpawnEvent.SpawnReason.CUSTOM);
}
worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightGetBlocks.java:825
addRecursiveNoEventscallsnmsWorld.moonrise$getEntityLookup().addNewEntity(...)unconditionally, dropping thePaperSupport.isPaper()guard and the Spigot fallback (PaperweightPlatformAdapter.getEntitySectionManager(nmsWorld).addNewEntity(entity)) that every other adapter still has (e.g.adapter-1_21_9/.../PaperweightGetBlocks.java:689-702).moonrise$getEntityLookup()only exists on Paper, so on Spigot this path now fails at runtime even though this 26.2 adapter still otherwise supports Spigot (PaperweightPlatformAdapter.getEntitiesat lines 609-621). Additionally, the boolean returned byaddNewEntityis ignored, so add failures no longer trigger the previousonErrorwarning. Please restore the Paper/Spigot branching (and failure handling) here.
private void addRecursiveNoEvents(Entity entity, ServerLevel nmsWorld) {
entity.spawnReason = CreatureSpawnEvent.SpawnReason.CUSTOM;
entity.generation = false;
nmsWorld.moonrise$getEntityLookup().addNewEntity(entity, false);
for (Entity passenger : entity.getPassengers()) {
addRecursiveNoEvents(passenger, nmsWorld);
}
}
worldedit-bukkit/adapters/adapter-26.2/src/main/java/com/sk89q/worldedit/bukkit/adapter/impl/fawe/v26_2/PaperweightGetBlocks.java:106
CREATED_PASSENGERS_UUIDSis astaticset that only ever grows:randomizePassengerUuidsInTagadds a fresh UUID for every pasted passenger, but nothing ever removes entries (not even after the passenger is removed viaremoveEntityRecursiveor on undo). On a long-running server this leaks memory unboundedly for the plugin's lifetime. Consider removing UUIDs once they've served their purpose (e.g. after the passenger is removed), scoping the tracking to the paste operation instead of a global static, or using a bounded/expiring structure.
private static final Set<UUID> CREATED_PASSENGERS_UUIDS = new HashSet<>();
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
MattBDev
left a comment
MattBDev
left a comment
There was a problem hiding this comment.
There are a few problems in this PR. I added comments explaining my concerns. There is one I figured I would mention here though.
The set grows forever!
These are the only three places the set is used in this PR:
// line 106: declared once for the whole server (all worlds, all players, all edits)
private static final Set<UUID> CREATED_PASSENGERS_UUIDS = new HashSet<>();
// line 795: every pasted rider is added
CREATED_PASSENGERS_UUIDS.add(fresh);
// line 786: removal only reads it
if (CREATED_PASSENGERS_UUIDS.contains(passenger.getUUID())) {There is no remove() or clear() anywhere, so nothing ever takes an entry out. Not when the edit is undone, not when the rider dies, etc.
At scale: a schematic of a stable with 20 horses, each with a rider, pasted 5,000 times on a creative plot server adds 100,000 UUIDs to a HashSet that never shrinks. Each entry costs a UUID object plus a HashMap node, so memory use grows with how much people paste, not with how many entities actually exist.
…pasted, add back Spigot support, bring back the error handling
|
Hello @MattBDev , I tried to fix all the issues you mentioned. |
|
This is only targeting one version, I also question if the new functions should be in the adapters rather than getblocks |
| default Entity createEntity(Location location, BaseEntity entity, UUID uuid) { | ||
| final IChunk chunk = getOrCreateChunk(location.getBlockX() >> 4, location.getBlockZ() >> 4); | ||
| if (entity.getNbt() == null) { | ||
| return null; // don't attempt to spawn "empty" entity |
There was a problem hiding this comment.
Well, it was
I changed it to simply return null because passengers are contained inside the mounted entity.
Therefore, when pasting, the createEntity method is called for each of the entities copied but the passengers have no NBT which was throwing an error
|
Hello @dordsor21 , thanks for your review ! :)
for the location of the new functions, you know more the codebase than me so if you want, I could move those in the adapter file :)
It is ! And as I said in the first PR text : "As you can see, for now, I only fixed the 26.2 version. As I am not using other versions, it would be great if someone could do it." |
|
Platform adapter is probably best yeah. If this.isnonky going to target 26.2 and 3 then we are unlikely to merge unfortunately |
|
Found a method to quickly test on other versions :) |
There was a problem hiding this comment.
🟡 Changes recommended
Redo duplicates passengers, valid NBT-less entities are dropped, and Spigot fallback spawning regresses.
19 open findings
1 resolved since last review
🧠 Review effort: Balanced
| if (entity.getNbt() == null) { | ||
| return null; // don't attempt to spawn "empty" entity | ||
| } | ||
| Map<String, LinTag<?>> map = new HashMap<>(entity.getNbt().value()); //do not modify original entity data |
There was a problem hiding this comment.
When doing this, an error appears when trying to paste the entities :
[23:45:24 INFO]: PapiCapi issued server command: //paste -e
[23:45:24 ERROR]: Unknown entity tag: EagerFaweCompoundTag[linTag=LinCompoundTag{UUIDMost=LinLongTag[-6738082543195897364], Pos=LinListTag[LinDoubleTag[-39.70890305216909], LinDoubleTag[76.1875], LinDoubleTag[20.917291816848504]], UUIDLeast=LinLongTag[-7101391990200382312], WorldUUIDMost=LinLongTag[-6738082543195897364], PersistentIDLSB=LinLongTag[-7101391990200382312], PersistentIDMSB=LinLongTag[-6738082543195897364], Id=LinStringTag[minecraft:villager], UUID=LinIntArrayTag[-1568832096, 2039235052, -1653421668, 357387416], WorldUUIDLeast=LinLongTag[-7101391990200382312]}]
|
Undo after a redo is now removing the passengers :) |
|
Hello, do I need to do some changes ? |
Overview
Fixes #2091
Fixes #2427
Partially fixes #2140
Description
Allow copied passengers from being spawned when pasting.
I also had to modify a part of the removal so that //undo successfully removes the entities.
In this context of //undo, I introduced a new field : CREATED_PASSENGERS_UUIDS to track which entity was spawned by the //paste.
Doing this prevents the removal of other entities riding pasted entities.
Here are the tests that I did :
As you can see, for now, I only fixed the 26.2 version. As I am not using other versions, it would be great if someone could do it.
Submitter Checklist
@since TODO.