Migrate to Mojang mapping #474
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughSummary by CodeRabbit
WalkthroughA comprehensive upgrade migrating the Fabric mod from legacy Minecraft APIs to current standards, including recipe data generation API migration, systematic type updates across all mixins and core code (EntityType, ServerPlayerEntity, Identifier, Text, World, Inventory mappings), and adjustment of networking, GUI, and block entity implementations to newer API surfaces. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120+ minutes Key areas requiring careful attention:
Pre-merge checks❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/gemini help |
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. |
|
/gemini summary |
|
/gemini summary |
|
It looks like you're trying to trigger both a summary and a review using |
Summary of ChangesThis pull request focuses on modernizing the project by migrating to Mojang's official mappings, updating dependencies, and refactoring internal APIs to improve compatibility and maintainability with the latest Minecraft versions. Additionally, it includes updates to the build environment and copyright notices. Highlights
Changelog
Activity
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinSkipSyncRegistry.java (1)
45-53: CRITICAL: Mixin target method signature is incorrect for Minecraft 1.21.10 and will not apply.The project targets Minecraft 1.21.10 with Fabric API 0.138.3. In MC 1.21.10, Registry.getKey(T value) returns Optional<RegistryKey>, not ResourceLocation.
The mixin target annotation specifies an incorrect return type:
target = "Lnet/minecraft/core/Registry;getKey(Ljava/lang/Object;)Lnet/minecraft/resources/ResourceLocation;"This method signature does not exist in the target version, so the mixin will not match. The
require = 0setting allows it to fail silently without error. The TODO comment "no target class" reflects this unresolved issue—the mixin needs to be corrected to handle the new return type (Optional<RegistryKey>) or replaced with an alternative approach.Action required: Update the mixin to match the current Registry.getKey() signature in Minecraft 1.21.10, or provide an alternative implementation for filtering galaxy namespace entries.
src/main/kotlin/one/oktw/galaxy/block/event/BlockEvents.kt (1)
49-70: Likely typo in off‑hand/main‑hand emptiness check
if (!player.isSecondaryUseActive || player.offhandItem.isEmpty && player.offhandItem.isEmpty)checksoffhandItemtwice, so the condition effectively ignores the main‑hand state and is looser than intended. This can change whenCustomBlockClickListenerclicks are processed.If the goal is “no secondary use OR both hands empty”, consider:
- if (!player.isSecondaryUseActive || player.offhandItem.isEmpty && player.offhandItem.isEmpty) { + if (!player.isSecondaryUseActive || (player.mainHandItem.isEmpty && player.offhandItem.isEmpty)) {This restores the likely original intent and makes the logic explicit.
src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt (1)
50-105: Fixlockleak when player disappears during countdown (and consider try/finally cleanup)There’s a subtle but user‑visible bug in the spawn flow:
- You add the player UUID to
lockbefore launching the coroutine.- After the 5‑second countdown, you resolve the live player instance with
originPlayer.level().server.playerList.getPlayer(originPlayer.uuid) ?: return@launch.- If the player logs out (or otherwise isn’t in the player list) during the countdown, this line returns null and you
return@launchwithout removing the UUID fromlock.- That player will then be permanently blocked from using
/spawnuntil the server restarts.You can fix this by removing the UUID when
getPlayer(...)returns null, and (optionally) wrapping the rest of the coroutine in atry/finallyso that the lock is always cleared even if later logic throws:private fun execute(source: CommandSourceStack) { val originPlayer = source.player if (originPlayer == null || lock.contains(originPlayer.uuid)) return lock += originPlayer.uuid main?.launch { try { for (i in 0..4) { originPlayer.displayClientMessage( Component.translatable("Respond.commandCountdown", 5 - i) .withStyle { it.withColor(ChatFormatting.GREEN) }, true ) delay(TimeUnit.SECONDS.toMillis(1)) } val player = originPlayer.level().server.playerList.getPlayer(originPlayer.uuid) ?: run { lock -= originPlayer.uuid return@launch } // … existing teleport logic … } finally { // Ensure we don’t leak the lock on any exit path lock -= originPlayer.uuid } } }You already remove the UUID explicitly in some branches; with a
finallyblock you can delete those individual removals and centralize the cleanup.src/main/kotlin/one/oktw/galaxy/block/entity/ModelCustomBlockEntity.kt (1)
37-103: Guard against nullentityUUIDwhen updating/killing display entityTwo critical null-pointer crashes can occur due to unsafe
!!operators on the nullableentityUUIDfield:
Facing setter (Line 47)
The property setter is invoked duringloadAdditional()→readCopyableData()(viaCustomBlockEntity), butentityUUIDis not set until aftersuper.loadAdditional()returns (Line 68). AccessingentityUUID!!at this point crashes.
setRemovedoverride (Line 92)
If the block is destroyed before the display entity spawns (first tick),entityUUIDremains null and the!!operator crashes.Apply these fixes:
open var facing = facing set(direction) { if (facing != null && direction != null && direction in allowedFacing) { field = direction - (level as? ServerLevel)?.getEntity(entityUUID!!)?.yRot = direction.toYRot() + val serverLevel = level as? ServerLevel + entityUUID?.let { uuid -> + serverLevel?.getEntity(uuid)?.yRot = direction.toYRot() + } setChanged() } }override fun setRemoved() { super.setRemoved() - (level as ServerLevel).getEntity(entityUUID!!)?.kill(level as ServerLevel) + val serverLevel = level as? ServerLevel ?: return + entityUUID?.let { uuid -> + serverLevel.getEntity(uuid)?.kill(serverLevel) + } }src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (1)
44-127: Fix type mismatch in self-join check and inconsistent lock key at line 116Two bugs need fixing:
Line 91: Impossible equality check
sourcePlayer.gameProfile(GameProfile) is compared totargetPlayer(NameAndId), which are incompatible types. The condition always evaluates to false, so the self-join message is never shown. Compare UUIDs instead:val joiningSelf = sourcePlayer.uuid == targetPlayer.id source.sendSuccess( { Component.nullToEmpty(if (joiningSelf) "正在加入您的星系" else "正在加入 ${targetPlayer.name} 的星系") }, false )Line 116: Nullable key inconsistency
lock[source.player]?.unlock()usessource.player, which is nullable and inconsistent with other lock accesses (lines 110, 125 correctly usesourcePlayer). Change to:lock[sourcePlayer]?.unlock()src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt (1)
72-121:lockmay never be released whenrealWorldor respawn data is nullInside the coroutine,
lockis only removed in therealTeleportTarget.missingRespawnBlock()branch and at the end of theif (realWorld != null && realSpawnPointPosition != null)block. IfrealWorldisnullorrealSpawnPointPositionisnullafter teleport, the coroutine finishes without removing the UUID fromlock, permanently blocking that player from using/homeuntil restart.You can fix this by moving the unlock outside the
realWorld/realSpawnPointPositioncheck so it always runs when the coroutine completes (barring exceptions):- val realTeleportTarget = player.findRespawnPositionAndUseSpawnBlock(player.wonGame, TeleportTransition.DO_NOTHING) + val realTeleportTarget = player.findRespawnPositionAndUseSpawnBlock(player.wonGame, TeleportTransition.DO_NOTHING) if (realTeleportTarget.missingRespawnBlock()) { player.displayClientMessage(Component.translatable("block.minecraft.spawn.not_valid").withStyle { it.withColor(ChatFormatting.RED) }, false) - lock -= player.uuid return@launch } player.teleport(realTeleportTarget) val realSpawnPointPosition = player.respawnConfig?.respawnData?.pos() val realWorld = source.server.getLevel(player.respawnConfig?.respawnData?.dimension() ?: OVERWORLD) - if (realWorld != null && realSpawnPointPosition != null) { + if (realWorld != null && realSpawnPointPosition != null) { val blockState = realWorld.getBlockState(realSpawnPointPosition) if (!player.wonGame && blockState.`is`(Blocks.RESPAWN_ANCHOR)) { // Consume Respawn Anchor Charge realWorld.setBlockAndUpdate( realSpawnPointPosition, blockState.setValue(RespawnAnchorBlock.CHARGE, blockState.getValue(RespawnAnchorBlock.CHARGE) - 1) ) realWorld.updateNeighborsAt(realSpawnPointPosition, blockState.block) player.connection.send( ClientboundSoundPacket( SoundEvents.RESPAWN_ANCHOR_DEPLETE, SoundSource.BLOCKS, realSpawnPointPosition.x.toDouble(), realSpawnPointPosition.y.toDouble(), realSpawnPointPosition.z.toDouble(), 1.0f, 1.0f, realWorld.getRandom().nextLong() ) ) } - lock -= player.uuid - } + } + lock -= player.uuidOptionally, wrapping the coroutine body in
try { ... } finally { lock -= player.uuid }would also protect against unlocks being skipped on unexpected exceptions.
🧹 Nitpick comments (25)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinOptimizeArmorStand_ArmorStandEntity.java (1)
29-29: Consider renaming the mixin class for consistency.The mixin class name still contains the old
ArmorStandEntitysuffix. While this doesn't affect functionality, renaming it toMixinOptimizeArmorStand_ArmorStandwould improve consistency with the new target class name.src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_ProjectileEntity.java (1)
33-34: Add explicitprivatemodifier to the unique field.The
maxAgefield should be explicitly marked asprivateto follow best practices for@Uniquemixin fields and match the pattern used in related code (e.g.,private int intoWaterinMixinThrownCountdown_Entity.java).Apply this diff:
@Unique - int maxAge = 0; + private int maxAge = 0;src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_NetworkHandler.java (1)
48-61: Item-interact inject flow and cancel/swing semanticsThe injection into
handleUseItembeforeServerPlayerGameMode.useItem(...)pluscancellable = trueis correct and matches the block-interact handler pattern. The cancel path (callinginfo.cancel(), sendingClientboundEntityEventPacketwithEntityEvent.USE_ITEM_COMPLETE,ClientboundSetHealthPacket, andcontainerMenu.sendAllDataToRemote()) is sound and consistent with the block mixin minus the block updates.One subtle semantic point: for
PlayerInteractItemEvent,swingandcancelare independent (unlikePlayerInteractBlockEvent, where the setter forcescancel = truewhenswingis set). In this handler:
- If
cancelis true, the server action is canceled and the client is resynced.- If
swingis true, the hand swings regardless of whethercancelis true or false (since the swing check is outside the cancel block).If this independence is intentional (allowing “swing only”, “cancel only”, or “cancel + swing”), then the current implementation is good. If you instead want “setting swing should always imply cancel” (like the block event), you’d either need to mirror the custom setter used in
PlayerInteractBlockEventor move the swing call inside the cancel block and document the behavior.Also, since the cancel/resync logic is nearly identical between item and block handlers, you might consider a small shared helper to send
USE_ITEM_COMPLETE+ health + container sync to reduce duplication and keep future protocol changes in one place.src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_RconBase.java (1)
30-31: Consider making the field explicitlyprotected.The
isLocalfield is accessed by the subclassMixinRCON_RconClient(as shown in the relevant code snippet). While package-private access works since both mixins are in the same package, explicitly declaring it asprotectedwould make the inheritance intent clearer and improve maintainability.Apply this diff to make the visibility explicit:
@Unique -boolean isLocal = false; +protected boolean isLocal = false;src/main/java/one/oktw/galaxy/mixin/tweak/MixinGlobalDataPack_VanillaDataPackProvider.java (1)
21-33: Mixin target and descriptors look consistent; just verify against your MC version and consider renaming
- The migration of the mixin target to
ServerPacksSourceand the new@ModifyArgsignature keeps the behavior (forcing datapacks toPath.of("datapacks")) intact.- The method and invoke descriptors in
@ModifyArglook structurally correct, but if the upstream signature changes even slightly, this mixin will silently fail to apply at runtime. Please confirm against the exact Minecraft/Paper/Fabric jar you compile against (check for MixinInvalidInjectionExceptionor “0 targets” warnings on startup).- Optional: the class name
MixinGlobalDataPack_VanillaDataPackProvideris now misleading since the target isServerPacksSource; renaming it on a future cleanup pass would reduce confusion.src/main/java/org/spongepowered/common/mixin/realtime/world/dimension/PortalManagerMixin_RealTime.java (2)
32-32: Mixin target change looks correct; consider class/file rename laterTargeting
PortalProcessor.classmatches the new portal API and is consistent with the shadow and injection descriptors below. One small nit: the class (and file) name still refers toPortalManager, which could be confusing long term now that the target isPortalProcessor. Not a blocker, but consider renaming the class/file on a future cleanup pass to reflect the new target.Please double-check that the mixin config (if any) still references this class by its current name before making any rename, to avoid breaking mixin discovery.
43-47: Decay-hook injection is correctly updated; minor duplication onlyThe second injection at the
INVOKEofPortalProcessor.decayTick():final int ticks = (int) ((RealTimeTrackingBridge) world).realTimeBridge$getRealTimeTicks() - 1; if (ticks > 0) { this.portalTime = Math.max(0, this.portalTime - ticks * 4); }correctly mirrors the earlier logic while updating to
ServerLevelandportalTime:
- The same real‑time tick delta is used to accelerate portal decay, matching vanilla’s “4 per tick” behavior via
ticks * 4.- The
ticks > 0guard andMath.max(0, ...)prevent underflow and ensureportalTimenever goes negative.- Targeting
processPortalTeleportationand thedecayTick()call site reflects the new PortalProcessor API.Only small nit is the duplication of the
tickscomputation between the two injectors, but that’s acceptable in a mixin context and doesn’t need to block this PR.If you’re already inspecting the new
processPortalTeleportationbody, also confirm that thisdecayTick()call is still the intended hook point (and hasn’t moved or been duplicated), so this injector continues to run exactly once per decay step.src/main/kotlin/one/oktw/galaxy/block/CustomBlockHelper.kt (1)
72-77: Use a null-defensive pattern instead of chained!!operatorsThe concern is valid: both
CustomBlock.registry.get()andtoItem()have nullable return types per their API signatures.
CustomRegistry.get(identifier: ResourceLocation)returnsT?(see CustomRegistry.kt:35)CustomBlock.toItem()returnsCustomBlockItem?(CustomBlock.kt:60)ModelCustomBlock.toItem()override uses safe cast and can also return nullThis makes the double
!!chain at line 75 unsafe. While normally working, it will crash if the ID becomes unregistered or iftoItem()returns null. The codebase already uses null-defensive patterns elsewhere (GetItem.kt, RegisterBlock.kt, and the Java mixin all null-check registry lookups or useinstanceofguards).Recommended refactor (lines 72–77):
fun destroyAndDrop(world: ServerLevel, pos: BlockPos) { val blockEntity = world.getBlockEntity(pos) as? ModelCustomBlockEntity ?: return world.setBlockAndUpdate(pos, Blocks.AIR.defaultBlockState()) val customBlock = CustomBlock.registry.get(blockEntity.getId()) val item = customBlock?.toItem() if (item != null) { Block.popResource(world, pos, item.createItemStack()) } world.playSound(null, pos, SoundEvents.METAL_BREAK, SoundSource.BLOCKS, 1.0F, 1.0F) }This prevents crashes from missing registry entries or null
toItem()results, and aligns with defensive patterns used elsewhere in the codebase.src/main/kotlin/one/oktw/galaxy/block/event/BlockEvents.kt (1)
72-93: Placement and crowbar behavior migrate cleanly to new APIs
BlockPlaceContext(event.context)for custom block placement, plusisShiftKeyDownandplayer.level()/world.server.tickCountusage for crowbar destruction, all look consistent with the Mojang APIs and preserve the previous behavior (including updatingusedLockonce an action succeeds).src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_NetworkHandler.java (1)
21-45: Event cancel + resync logic looks good; consider aligning mixin superclassThe new injection into
handlePlayerActionwithServerboundPlayerActionPacketandClientboundBlockUpdatePacketis straightforward: on cancel you revert the block atpacket.getPos()and resync the container menu, which mirrors the pattern used in the other interaction mixins.One thing to consider for consistency with
MixinPlayerInteractBlock_NetworkHandlerandMixinPlayerInteractItem_NetworkHandleris declaring this mixin as:@Mixin(ServerGamePacketListenerImpl.class) public abstract class MixinPlayerAction_NetworkHandler extends ServerCommonPacketListenerImpl { // matching ctor ... }That keeps the mixin’s superclass aligned with the target’s hierarchy and gives you the same helpers (
send(...),serverfield, etc.) those mixins rely on, reducing the chance of subtle mixin compatibility issues in future changes.src/main/kotlin/one/oktw/galaxy/block/event/Elevator.kt (1)
43-55: Minor: avoid redundantBlockPosallocation in sound playback
doTeleportcorrectly usesteleportTo, zeroes velocity, sends the motion packet, and plays a teleport sound on the server level. One small cleanup:BlockPos(pos)is redundant whenposis already aBlockPos; you can passposdirectly toplaySoundto avoid an extra allocation.- player.level().playSound( - null, - BlockPos(pos), + player.level().playSound( + null, + pos, SoundEvents.CHORUS_FRUIT_TELEPORT, SoundSource.BLOCKS, 1.0f, 1.0f )src/main/kotlin/one/oktw/galaxy/block/entity/DummyBlockEntity.kt (1)
29-37: Capturelevelonce inloadAdditionalfor robustness and clarityThe migration to
ValueInputandResourceLocationlooks good, and the logic of swapping the dummy block entity for the real one and pipingreadCopyableData(view)into it is clear.To make this a bit safer and easier to read, consider capturing
levelinto a local and early‑returning if it’s null, instead of callinglevel?twice. That keeps removal and insertion operating on the same non‑null level instance and avoids any future surprises if the lifecycle changes:override fun loadAdditional(view: ValueInput) { super.loadAdditional(view) val level = level ?: return view.getStringOr("id", "")?.let(ResourceLocation::tryParse)?.let(CustomBlock.registry::get)?.let { if (it != CustomBlock.DUMMY) { level.removeBlockEntity(worldPosition) level.setBlockEntity(it.createBlockEntity(worldPosition).apply { readCopyableData(view) }) } } }src/main/kotlin/one/oktw/galaxy/gui/GUISBackStackManager.kt (2)
31-45: Weak-key map does not actually prevent player leak here
GUISBackStackManagerstores a strong reference toServerPlayerwhilemanagersusesweakKeys(). Because the staticmanagersmap strongly holds eachGUISBackStackManager, and each manager strongly holds itsplayer, theServerPlayerremains strongly reachable even after disconnect, so the weak key won’t allow it to be GC’d.If you intended
weakKeys()to avoid leakingServerPlayerinstances, consider one of:
- Removing the strong
playerfield fromGUISBackStackManager(passServerPlayerinto methods instead, or store aUUID), or- Changing
playerto aWeakReference<ServerPlayer>and null‑checking before use, or- Dropping
weakKeys()and explicitly removing entries when players leave.
48-71: GUI open/close backstack logic looks sound but could be made more robustThe migration to
openMenuand the coroutine-based “next tick” workaround for close‑then‑open behavior look consistent, and the use ofConcurrentLinkedDequeplusisSameThreadchecks keeps things reasonably safe.If you want to harden this further:
- In the reopen loop, consider using
peekLast()+pollLast()instead oflast()to avoid potential surprises if another thread mutatesstackbetweenisNotEmpty()andlast().- Optionally handle the case where
openMenufails inopen(gui)the same way you already do in the backstack reopen loop (currently the first open ignores the return value).These are non-blocking polish items; the current behavior is acceptable as-is.
src/main/kotlin/one/oktw/galaxy/gui/utils/InventoryEditor.kt (1)
21-45: Container/MenuType migration here looks correctThe switch to
Container,MenuType<out AbstractContainerMenu>,setItem, andcontainerSizematches the new API shape, and the helper methods (set,fill,fillAll) preserve previous behavior.Only minor note: this still relies on
InventoryUtils.xyToIndexnever producing an index ≥inventory.containerSize. If that helper is ever reused with different menu sizes, it might be worth asserting or clamping, but that’s not new to this PR.src/main/kotlin/one/oktw/galaxy/item/Gui.kt (1)
21-36: Clean migration to Component/ResourceLocation for GUI itemThe constructor wiring (
ResourceLocation.fromNamespaceAndPath,CustomModelData,Component?) and thehideTooltip = name == nullflag all look consistent with the new API.If you want to tighten up style, you could replace
Component.nullToEmpty("")withComponent.empty()for the fallback name, but that’s purely cosmetic.src/main/kotlin/one/oktw/galaxy/command/commands/admin/RegisterBlock.kt (1)
21-65: Command/argument migration is consistent; behavior unchangedThe move to
CommandSourceStack/Commands,ResourceLocationArgument.id(),BlockPosArgument.blockPos(), andsendFailure/sendSuccesswithComponentis consistent with the rest of the command migration.The execution logic—validating the
CustomBlock, resolving a loadedBlockPos, replacing the block entity vialevel.removeBlockEntity/setBlockEntity, and emitting a plain-text success message—preserves the prior behavior.If you ever revisit this command, two optional improvements would be:
- Use a translatable success
Componentinstead of a hard-coded English string for better i18n.- Consider also ensuring the correct block state is set at
blockPosifCustomBlockrequires a specific backing block.src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_NetworkHandler.java (1)
21-55: Chat mixin migration appears correct; consider usingsenderfor clarityThe redirect target descriptor and argument types match the new
broadcastChatMessageoverload, and the event emission plus cancel behavior (skipping broadcast, logging viaserver.logChatMessage) keep prior semantics with the newPlayerChatMessageAPI. One small clarity tweak would be to passsenderintoPlayerChatEventinstead of the shadowedplayerfield, since they should be identical butsenderis the method parameter.Given this depends on the exact
ServerGamePacketListenerImpl.broadcastChatMessageandPlayerList.broadcastChatMessagesignatures, please re‑check against your mapped Minecraft version that the descriptor in the@Attarget is still accurate.src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionFile.java (1)
103-121: Read path mirrors vanilla semantics; consider whether lock coverage is sufficientThe injected locks on
clear,write, andgetOffsetplus the unsynchronizedgalaxy$getChunkInputStreamNoSyncgive you a parallel read path while still serializing core metadata operations. The body ofgalaxy$getChunkInputStreamNoSyncmatches the structure of the vanilla reader (sector math, header checks, external-chunk handling, and the profiler hook viaJvmProfiler.INSTANCE.onRegionFileRead(info, pos, version, n)), so behavior should be equivalent aside from synchronization.If you ever run into evidence of chunks being read while a write is in progress, widening the critical section (e.g., also guarding the
file.readwith the samelock) would be the lever to trade some parallelism for stronger safety, but that’s a design choice rather than a correctness blocker given this mod’s existing async IO model.Also applies to: 133-169
src/main/kotlin/one/oktw/galaxy/block/entity/HTCraftingTableBlockEntity.kt (2)
175-191: Animation job loop is reasonable; just ensure the scope lifecycle is acceptableThe coroutine that cycles multi-example ingredients until
(player as ServerPlayer).hasDisconnected()keeps the display responsive and is correctly tied to the GUI lifecycle viagui.onClose { job.cancel() }. As long asHTCraftingTableBlockEntity’sCoroutineScopeis cleaned up when the block entity is removed (or you’re fine with a long-lived scope per instance), this pattern is fine.
196-203: Guard the cast toServerPlayerinonClickor document the server-only assumption
onClickassumesplayeris always aServerPlayer:GUISBackStackManager.openGUI(player as ServerPlayer, getListGui()) return InteractionResult.SUCCESS_SERVERIf
CustomBlockClickListener.onClickis guaranteed to be invoked only on the server, this is fine; otherwise, a client-side call would throw aClassCastException. Consider either asserting/guarding the cast (e.g.,if (player is ServerPlayer) ...) or explicitly documenting that this callback is server-only.src/datagen/kotlin/one/oktw/galaxy/datagen/util/ShapedRecipeJsonBuilder.kt (1)
71-100:savewiring tracks the modern RecipeBuilder/RecipeOutput contract
savecorrectly:
- Remaps the
ResourceKey<Recipe<*>>withexporter.getRecipeIdentifier(recipeKey.location()).- Builds a
ShapedRecipePatternfrominputsandpattern.- Configures the advancement with
RecipeUnlockedTrigger,AdvancementRewards, andAdvancementRequirements.Strategy.OR, then adds all criteria.- Constructs a
ShapedRecipeusingRecipeBuilder.determineBookCategory(category)and theshowNotificationflag.- Passes everything to
exporter.accept(...)with the"recipes/<category.folderName>/"prefix.The only minor nit is the shadowing of the
recipeKeyparameter by the localval recipeKey; renaming the local to something likeresolvedKeywould slightly improve readability, but behavior is correct as written.src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_StorageIoWorker.java (1)
87-99: storePendingChunk/pendingWrites integration is coherent with the new PendingStore typeThe overwritten
storePendingChunk()groupspendingWritesby region, schedules region batches on thePriorityConsecutiveExecutorwithPriority.FOREGROUND, and uses a BACKGROUND task pluswriteLockto keep draining the queue without overlapping store cycles. TherunStoreinjector that first doespendingWrites.remove(pos, result)and cancels when the entry has already changed ensures that stalePendingStoreinstances are skipped, preserving “last write wins” semantics.Given this relies on
ConcurrentSkipListMapiteration and the executor to serialize the actual writes, the structure looks sound. If you ever observe weird edge cases around coalescing or ordering under heavy write load, the first place to tweak would be the grouping/scheduling strategy instorePendingChunk(), but nothing here is obviously unsafe.Also applies to: 127-132
src/main/kotlin/one/oktw/galaxy/gui/GUI.kt (2)
99-103: Add a message when throwingIndexOutOfBoundsException
getItem(index)throws a bareIndexOutOfBoundsException()whenindexis outside0 until inventory.containerSize. This matches the static analysis warning and makes debugging harder.Consider including context:
- if (index !in 0 until inventory.containerSize) throw IndexOutOfBoundsException() + if (index !in 0 until inventory.containerSize) { + throw IndexOutOfBoundsException("GUI inventory index out of range: index=$index, size=${inventory.containerSize}") + }
245-248: Use the actual slot’s item forGUIClickEvent, notinventory.getItem(slot)Here
GUIClickEventreceivesinventory.getItem(slot)as the clicked stack, but for slots bound to external containers (e.g., block entities, custom SimpleContainers) the backingSlot’sitemmay differ from the GUI’s internalinventory.To ensure click handlers always see the real stack under the cursor, consider:
- val event = GUIClickEvent(player as ServerPlayer, x, y, action, inventory.getItem(slot)) + val slotRef = slots[slot] + val event = GUIClickEvent(player as ServerPlayer, x, y, action, slotRef.item)This keeps GUIClickEvent aligned with what the player actually clicked, regardless of the underlying container.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (107)
.circleci/config.yml(1 hunks)build.gradle.kts(2 hunks)src/datagen/kotlin/one/oktw/galaxy/datagen/GalaxyRecipeProvider.kt(6 hunks)src/datagen/kotlin/one/oktw/galaxy/datagen/util/ShapedRecipeJsonBuilder.kt(4 hunks)src/main/java/one/oktw/galaxy/mixin/accessor/AbilitiesAccessor.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/accessor/BeaconLevelAccessor.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_NetworkHandler.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_MeCommand.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_NetworkHandler.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_SayCommand.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_NetworkHandler.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_NetworkHandler.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_NetworkHandler.java(3 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerUseItemOnBlock_ItemStack.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/interfaces/IThrownCountdown_Entity.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/interfaces/InventoryAvailableSlots.java(0 hunks)src/main/java/one/oktw/galaxy/mixin/interfaces/RegionFileInputStream.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionBasedStorage.java(3 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionFile.java(4 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_ServerPlayNetworkHandler.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_StorageIoWorker.java(7 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinCustomBlockEntity_BarrierBlock.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinCustomBlockEntity_ChunkData.java(3 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinCustomBlockEntity_Structure.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinFixBeacon_BeaconBlockEntity.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinGUI_ServerPlayNetworkHandler.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinGlobalDataPack_VanillaDataPackProvider.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinItemStackEqualAllowNull_ItemStack.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinOptimizeArmorStand_ArmorStandEntity.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_RconBase.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_RconClient.java(3 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinSkipSyncRegistry.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_Entity.java(3 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_ProjectileEntity.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_ThrownEntity.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/SignStyle_NetworkHandler.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/accessor/AbstractFurnaceBlockEntityAccessor.java(3 hunks)src/main/java/org/spongepowered/common/mixin/realtime/accessor/BrewingStandBlockEntityAccessor.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/blockentity/AbstractFurnaceBlockEntityMixin_RealTime.java(4 hunks)src/main/java/org/spongepowered/common/mixin/realtime/blockentity/BrewingStandBlockEntityMixin_RealTime.java(3 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/EntityMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/ExperienceOrbEntityMixin_RealTime.java(1 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/ItemEntityMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/LivingEntityMixin_RealTime.java(3 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/CreeperEntityMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/MobEntityMixin_RealTime.java(1 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/ZombieVillagerEntityMixin_RealTime.java(1 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/passive/PassiveEntityMixin_RealTime.java(1 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/player/PlayerEntityMixin_RealTime.java(3 hunks)src/main/java/org/spongepowered/common/mixin/realtime/server/MinecraftServerMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerPlayNetworkHandlerMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerPlayerInteractionManagerMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/world/WorldMixin_RealTime.java(4 hunks)src/main/java/org/spongepowered/common/mixin/realtime/world/dimension/PortalManagerMixin_RealTime.java(3 hunks)src/main/kotlin/one/oktw/galaxy/Main.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/block/CustomBlock.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/block/CustomBlockEntityTicker.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/block/CustomBlockHelper.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/block/DummyBlock.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/block/HTCraftingTable.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/block/HarvestBlock.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/block/ModelCustomBlock.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/block/TestGuiBlock.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/block/TrashcanBlock.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/block/entity/CustomBlockEntity.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/block/entity/DummyBlockEntity.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/block/entity/HTCraftingTableBlockEntity.kt(6 hunks)src/main/kotlin/one/oktw/galaxy/block/entity/HarvestBlockEntity.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/block/entity/ModelCustomBlockEntity.kt(3 hunks)src/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt(6 hunks)src/main/kotlin/one/oktw/galaxy/block/entity/TrashcanBlockEntity.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/block/event/AngelBlock.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/block/event/BlockEvents.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/block/event/Elevator.kt(3 hunks)src/main/kotlin/one/oktw/galaxy/block/listener/CustomBlockClickListener.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/chat/Exchange.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/command/Command.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Admin.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/admin/Creative.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/admin/FlySpeed.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/admin/GetItem.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/admin/RegisterBlock.kt(3 hunks)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/event/type/PlayerActionEvent.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/event/type/PlayerChatEvent.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/event/type/PlayerInteractBlockEvent.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/event/type/PlayerInteractItemEvent.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/event/type/PlayerJumpEvent.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/event/type/PlayerSneakEvent.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/event/type/PlayerSneakReleaseEvent.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/event/type/PlayerUpdateSignEvent.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/event/type/PlayerUseItemOnBlock.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/event/type/ProxyResponseEvent.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/gui/GUI.kt(8 hunks)src/main/kotlin/one/oktw/galaxy/gui/GUIClickEvent.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/gui/GUISBackStackManager.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/gui/utils/InventoryEditor.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/gui/utils/InventoryUtils.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/item/CustomBlockItem.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/item/CustomItem.kt(3 hunks)src/main/kotlin/one/oktw/galaxy/item/CustomItemHelper.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/item/Gui.kt(1 hunks)
⛔ Files not processed due to max files limit (32)
- src/main/kotlin/one/oktw/galaxy/item/Material.kt
- src/main/kotlin/one/oktw/galaxy/item/Misc.kt
- src/main/kotlin/one/oktw/galaxy/item/Tool.kt
- src/main/kotlin/one/oktw/galaxy/item/Upgrade.kt
- src/main/kotlin/one/oktw/galaxy/item/Weapon.kt
- src/main/kotlin/one/oktw/galaxy/item/category/Block.kt
- src/main/kotlin/one/oktw/galaxy/item/category/CustomItemCategory.kt
- src/main/kotlin/one/oktw/galaxy/item/category/Material.kt
- src/main/kotlin/one/oktw/galaxy/item/category/Tool.kt
- src/main/kotlin/one/oktw/galaxy/item/category/Upgrade.kt
- src/main/kotlin/one/oktw/galaxy/item/category/Weapon.kt
- src/main/kotlin/one/oktw/galaxy/item/event/CustomItemEventHandler.kt
- src/main/kotlin/one/oktw/galaxy/item/event/Wrench.kt
- src/main/kotlin/one/oktw/galaxy/item/gui/GuiModelBuilder.kt
- src/main/kotlin/one/oktw/galaxy/item/recipe/Crowbar.kt
- src/main/kotlin/one/oktw/galaxy/item/recipe/CustomItemRecipe.kt
- src/main/kotlin/one/oktw/galaxy/item/recipe/Elevator.kt
- src/main/kotlin/one/oktw/galaxy/item/recipe/HTCraftingTable.kt
- src/main/kotlin/one/oktw/galaxy/item/recipe/Harvest.kt
- src/main/kotlin/one/oktw/galaxy/item/recipe/TrashCan.kt
- src/main/kotlin/one/oktw/galaxy/item/recipe/Wrench.kt
- src/main/kotlin/one/oktw/galaxy/network/ProxyAPIPayload.kt
- src/main/kotlin/one/oktw/galaxy/network/ProxyChatPayload.kt
- src/main/kotlin/one/oktw/galaxy/player/Harvest.kt
- src/main/kotlin/one/oktw/galaxy/util/CustomRegistry.kt
- src/main/kotlin/one/oktw/galaxy/util/HarvestUtil.kt
- src/main/kotlin/one/oktw/galaxy/util/KotlinCoroutineTaskExecutor.kt
- src/main/kotlin/one/oktw/galaxy/util/MinecraftAsyncExecutor.kt
- src/main/kotlin/one/oktw/galaxy/util/NbtUuidHelper.kt
- src/main/kotlin/one/oktw/galaxy/util/Registrable.kt
- src/main/resources/accessor.mixin.json
- src/main/resources/galaxy.accesswidener
💤 Files with no reviewable changes (1)
- src/main/java/one/oktw/galaxy/mixin/interfaces/InventoryAvailableSlots.java
🧰 Additional context used
🧬 Code graph analysis (41)
src/main/java/org/spongepowered/common/mixin/realtime/entity/ExperienceOrbEntityMixin_RealTime.java (3)
src/main/java/org/spongepowered/common/mixin/realtime/blockentity/AbstractFurnaceBlockEntityMixin_RealTime.java (1)
Mixin(55-111)src/main/java/org/spongepowered/common/mixin/realtime/entity/EntityMixin_RealTime.java (1)
Mixin(55-86)src/main/java/org/spongepowered/common/mixin/realtime/entity/ItemEntityMixin_RealTime.java (1)
Mixin(53-73)
src/main/kotlin/one/oktw/galaxy/block/entity/DummyBlockEntity.kt (4)
src/main/kotlin/one/oktw/galaxy/block/entity/CustomBlockEntity.kt (1)
readCopyableData(48-48)src/main/kotlin/one/oktw/galaxy/block/entity/HarvestBlockEntity.kt (1)
readCopyableData(134-137)src/main/kotlin/one/oktw/galaxy/block/entity/ModelCustomBlockEntity.kt (1)
readCopyableData(71-74)src/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt (1)
readCopyableData(128-131)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinCustomBlockEntity_BarrierBlock.java (3)
src/main/kotlin/one/oktw/galaxy/block/CustomBlock.kt (2)
createBlockEntity(62-62)toItem(60-60)src/main/kotlin/one/oktw/galaxy/block/entity/CustomBlockEntity.kt (2)
type(29-49)getId(30-30)src/main/kotlin/one/oktw/galaxy/gui/GUI.kt (3)
type(48-415)type(147-203)blockEntity(194-197)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_NetworkHandler.java (4)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_NetworkHandler.java (1)
Mixin(38-56)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_NetworkHandler.java (1)
Mixin(42-70)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_NetworkHandler.java (1)
Mixin(39-62)src/main/kotlin/one/oktw/galaxy/event/type/PlayerActionEvent.kt (1)
packet(24-24)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinSkipSyncRegistry.java (1)
src/main/kotlin/one/oktw/galaxy/block/DummyBlock.kt (1)
ResourceLocation(25-27)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_NetworkHandler.java (2)
src/main/kotlin/one/oktw/galaxy/event/type/PlayerJumpEvent.kt (1)
player(23-23)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_MeCommand.java (3)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_NetworkHandler.java (1)
Mixin(38-56)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_SayCommand.java (1)
Mixin(35-59)src/main/kotlin/one/oktw/galaxy/event/type/PlayerChatEvent.kt (1)
player(24-24)
src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerPlayerInteractionManagerMixin_RealTime.java (4)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_NetworkHandler.java (1)
Mixin(42-70)src/main/java/org/spongepowered/common/mixin/realtime/server/MinecraftServerMixin_RealTime.java (1)
Mixin(53-71)src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerPlayNetworkHandlerMixin_RealTime.java (1)
Mixin(58-80)src/main/java/org/spongepowered/common/mixin/realtime/world/WorldMixin_RealTime.java (1)
Mixin(64-100)
src/main/java/org/spongepowered/common/mixin/realtime/entity/passive/PassiveEntityMixin_RealTime.java (2)
src/main/java/org/spongepowered/common/mixin/realtime/entity/EntityMixin_RealTime.java (1)
Mixin(55-86)src/main/java/org/spongepowered/common/mixin/realtime/server/MinecraftServerMixin_RealTime.java (1)
Mixin(53-71)
src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/ZombieVillagerEntityMixin_RealTime.java (2)
src/main/java/org/spongepowered/common/mixin/realtime/entity/EntityMixin_RealTime.java (1)
Mixin(55-86)src/main/java/org/spongepowered/common/mixin/realtime/entity/LivingEntityMixin_RealTime.java (1)
Mixin(53-99)
src/main/kotlin/one/oktw/galaxy/block/entity/CustomBlockEntity.kt (3)
src/main/kotlin/one/oktw/galaxy/block/entity/HarvestBlockEntity.kt (1)
readCopyableData(134-137)src/main/kotlin/one/oktw/galaxy/block/entity/ModelCustomBlockEntity.kt (1)
readCopyableData(71-74)src/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt (1)
readCopyableData(128-131)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_RconBase.java (1)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_RconClient.java (1)
Mixin(32-43)
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java (1)
src/main/java/org/spongepowered/common/mixin/realtime/server/MinecraftServerMixin_RealTime.java (1)
Mixin(53-71)
src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt (1)
src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt (1)
execute(50-105)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_SayCommand.java (2)
src/main/kotlin/one/oktw/galaxy/event/type/PlayerChatEvent.kt (1)
player(24-24)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)
src/main/kotlin/one/oktw/galaxy/block/entity/ModelCustomBlockEntity.kt (2)
src/main/kotlin/one/oktw/galaxy/block/entity/HarvestBlockEntity.kt (1)
setChanged(71-74)src/main/kotlin/one/oktw/galaxy/block/entity/CustomBlockEntity.kt (1)
getId(30-30)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_NetworkHandler.java (5)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_NetworkHandler.java (1)
Mixin(33-46)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_NetworkHandler.java (1)
Mixin(39-62)src/main/kotlin/one/oktw/galaxy/block/event/BlockEvents.kt (1)
onPlayerInteractBlock(49-70)src/main/kotlin/one/oktw/galaxy/event/type/PlayerInteractBlockEvent.kt (1)
packet(24-33)src/main/kotlin/one/oktw/galaxy/event/type/PlayerInteractItemEvent.kt (1)
packet(24-26)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerUseItemOnBlock_ItemStack.java (2)
src/main/kotlin/one/oktw/galaxy/event/type/PlayerUseItemOnBlock.kt (1)
context(23-32)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)
src/main/java/one/oktw/galaxy/mixin/accessor/BeaconLevelAccessor.java (1)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinFixBeacon_BeaconBlockEntity.java (1)
Mixin(41-72)
src/main/java/org/spongepowered/common/mixin/realtime/world/WorldMixin_RealTime.java (3)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java (1)
Mixin(33-45)src/main/java/org/spongepowered/common/mixin/realtime/blockentity/AbstractFurnaceBlockEntityMixin_RealTime.java (1)
Mixin(55-111)src/main/java/org/spongepowered/common/mixin/realtime/entity/EntityMixin_RealTime.java (1)
Mixin(55-86)
src/main/kotlin/one/oktw/galaxy/gui/GUISBackStackManager.kt (1)
src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (1)
CoroutineScope(43-131)
src/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt (1)
src/main/kotlin/one/oktw/galaxy/gui/GUI.kt (3)
addBinding(105-109)addBinding(111-115)addBinding(117-123)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_Entity.java (1)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_ThrownEntity.java (1)
Mixin(35-52)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionBasedStorage.java (2)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionFile.java (1)
Mixin(46-170)src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_StorageIoWorker.java (1)
Mixin(48-171)
src/main/kotlin/one/oktw/galaxy/block/entity/TrashcanBlockEntity.kt (2)
src/main/kotlin/one/oktw/galaxy/gui/GUI.kt (3)
addSlot(171-177)addSlot(179-179)editInventory(125-131)src/main/kotlin/one/oktw/galaxy/gui/utils/InventoryEditor.kt (1)
fillAll(43-45)
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/PlayerEntityMixin_RealTime.java (4)
src/main/java/org/spongepowered/common/mixin/realtime/entity/EntityMixin_RealTime.java (1)
Mixin(55-86)src/main/java/org/spongepowered/common/mixin/realtime/entity/ExperienceOrbEntityMixin_RealTime.java (1)
Mixin(53-63)src/main/java/org/spongepowered/common/mixin/realtime/entity/ItemEntityMixin_RealTime.java (1)
Mixin(53-73)src/main/java/org/spongepowered/common/mixin/realtime/entity/LivingEntityMixin_RealTime.java (1)
Mixin(53-99)
src/main/kotlin/one/oktw/galaxy/gui/GUI.kt (3)
src/main/kotlin/one/oktw/galaxy/block/entity/HarvestBlockEntity.kt (1)
getItem(172-172)src/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt (1)
getItem(166-166)src/main/kotlin/one/oktw/galaxy/block/entity/TrashcanBlockEntity.kt (1)
getItem(48-48)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_NetworkHandler.java (4)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_NetworkHandler.java (1)
Mixin(42-70)src/main/kotlin/one/oktw/galaxy/event/type/PlayerInteractBlockEvent.kt (1)
packet(24-33)src/main/kotlin/one/oktw/galaxy/event/type/PlayerInteractItemEvent.kt (1)
packet(24-26)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_NetworkHandler.java (5)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_MeCommand.java (1)
Mixin(34-51)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_SayCommand.java (1)
Mixin(35-59)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_NetworkHandler.java (1)
Mixin(42-70)src/main/kotlin/one/oktw/galaxy/event/type/PlayerChatEvent.kt (1)
player(24-24)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)
src/main/kotlin/one/oktw/galaxy/block/entity/HTCraftingTableBlockEntity.kt (2)
src/main/kotlin/one/oktw/galaxy/gui/GUI.kt (1)
onUpdate(141-143)src/main/kotlin/one/oktw/galaxy/command/commands/admin/Creative.kt (1)
getListGui(53-129)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_StorageIoWorker.java (1)
src/main/kotlin/one/oktw/galaxy/util/KotlinCoroutineTaskExecutor.kt (1)
schedule(42-45)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_ServerPlayNetworkHandler.java (3)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_NetworkHandler.java (1)
Mixin(42-70)src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java (1)
Mixin(33-45)src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerPlayNetworkHandlerMixin_RealTime.java (1)
Mixin(58-80)
src/main/java/org/spongepowered/common/mixin/realtime/world/dimension/PortalManagerMixin_RealTime.java (1)
src/main/java/org/spongepowered/common/mixin/realtime/entity/EntityMixin_RealTime.java (1)
Mixin(55-86)
src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerPlayNetworkHandlerMixin_RealTime.java (1)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_ServerPlayNetworkHandler.java (1)
Mixin(38-70)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_ThrownEntity.java (1)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_Entity.java (1)
Mixin(32-56)
src/main/kotlin/one/oktw/galaxy/block/entity/HarvestBlockEntity.kt (3)
src/main/kotlin/one/oktw/galaxy/gui/GUI.kt (4)
addSlot(171-177)addSlot(179-179)getItem(97-97)getItem(99-103)src/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt (2)
getItem(166-166)setItem(178-184)src/main/kotlin/one/oktw/galaxy/block/entity/TrashcanBlockEntity.kt (2)
getItem(48-48)setItem(54-54)
src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/CreeperEntityMixin_RealTime.java (2)
src/main/java/org/spongepowered/common/mixin/realtime/entity/EntityMixin_RealTime.java (1)
Mixin(55-86)src/main/java/org/spongepowered/common/mixin/realtime/entity/ExperienceOrbEntityMixin_RealTime.java (1)
Mixin(53-63)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_ProjectileEntity.java (2)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_Entity.java (1)
Mixin(32-56)src/main/java/org/spongepowered/common/mixin/realtime/entity/ExperienceOrbEntityMixin_RealTime.java (1)
Mixin(53-63)
src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/MobEntityMixin_RealTime.java (2)
src/main/java/org/spongepowered/common/mixin/realtime/entity/EntityMixin_RealTime.java (1)
Mixin(55-86)src/main/java/org/spongepowered/common/mixin/realtime/entity/LivingEntityMixin_RealTime.java (1)
Mixin(53-99)
src/main/java/org/spongepowered/common/mixin/realtime/server/MinecraftServerMixin_RealTime.java (2)
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java (1)
Mixin(53-69)src/main/java/org/spongepowered/common/mixin/realtime/world/WorldMixin_RealTime.java (1)
Mixin(64-100)
src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (2)
src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt (1)
execute(52-122)src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt (1)
execute(50-105)
🪛 detekt (1.23.8)
src/main/kotlin/one/oktw/galaxy/gui/GUI.kt
[warning] 100-100: A call to the default constructor of an exception was detected. Instead one of the constructor overloads should be called. This allows to provide more meaningful exceptions.
(detekt.exceptions.ThrowingExceptionsWithoutMessageOrCause)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Summary
| @Inject(method = "flush", at = @At("HEAD")) | ||
| private void syncLock(CallbackInfo ci) { | ||
| lock.lock(); | ||
| } | ||
|
|
||
| @Inject(method = "sync", at = @At("RETURN")) | ||
| @Inject(method = "flush", at = @At("RETURN")) | ||
| private void syncUnlock(CallbackInfo ci) { | ||
| lock.unlock(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Lock around flush can be left held if flush() throws
Extending the lock/unlock pattern to flush() keeps behavior consistent with getRegionFile/close, but the HEAD / RETURN injections mean that if RegionFileStorage.flush() ever throws before a normal return, lock.unlock() won’t run and the ReentrantLock stays permanently held, effectively deadlocking future region operations on this instance.
If flush() is known not to throw in practice this may be acceptable, but it’s still a brittle pattern. Where feasible, consider a structure that guarantees unlock() in a finally‑equivalent path (for example, an overwrite wrapper or an additional injection on exceptional exit if/when Mixin adds such support), or switching to a best‑effort pattern like tryLock plus guarded critical section to avoid permanent lock retention on unexpected failures.
I'll verify this lock management issue by examining the codebase structure and understanding the exception handling context.
🏁 Script executed:
#!/bin/bash
# Inspect the full Mixin file to understand the lock field and context
cat "src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionBasedStorage.java"Length of output: 3322
🏁 Script executed:
#!/bin/bash
# Search for RegionFileStorage flush method to understand if it throws
find . -name "RegionFileStorage.java" -o -name "*RegionFile*.java" | head -20Length of output: 151
🏁 Script executed:
#!/bin/bash
# Look for getRegionFile and close method patterns in the mixin file to compare
rg -n "getRegionFile|close" "src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionBasedStorage.java"Length of output: 323
🏁 Script executed:
#!/bin/bash
# Search for try-finally or try-catch patterns with locks in the codebase
rg -n "try\s*\{" -A 5 "src/main/java/one/oktw/galaxy/mixin/" | head -50Length of output: 2024
🏁 Script executed:
#!/bin/bash
# Look for ReentrantLock usage patterns in the codebase
rg -n "ReentrantLock|\.lock\(\)|\.unlock\(\)" "src/main/java/one/oktw/galaxy/mixin/" -B 2 -A 2Length of output: 8816
🏁 Script executed:
#!/bin/bash
# Check the Minecraft/Forge RegionFileStorage source to understand flush() throws
find . -path "*/minecraft/*" -o -path "*/net/minecraft/*" -name "RegionFileStorage.java" 2>/dev/null | head -5Length of output: 0
I need to verify whether flush() can actually throw exceptions and check the context of its usage.
🏁 Script executed:
#!/bin/bash
# Search for all calls to flush() in the codebase to see exception handling context
rg -n "\.flush\(\)" -B 3 -A 3 --type=javaLength of output: 0
🏁 Script executed:
#!/bin/bash
# Search for storage-related I/O operations that might throw
rg -n "IOException|throws" "src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionBasedStorage.java"Length of output: 257
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation about exception handling in related files
rg -n "throws|IOException|catch" "src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionFile.java" | head -30Length of output: 387
🏁 Script executed:
#!/bin/bash
# Look for close() method signature to see if it declares throws
rg -n "void close\(\)" "src/main/java/one/oktw/galaxy/mixin/" -A 2Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for any @At("THROW") or exception-handling injections in the mixin framework usage
rg -n "@At.*THROW|@At.*CATCH|invoke.*Exception" "src/main/java/one/oktw/galaxy/mixin/"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any explicit exception handlers in the parent codebase around these operations
rg -n "flush|close" "src/main/java/one/oktw/galaxy" -A 2 -B 2 --type=java | grep -A 2 -B 2 "catch\|throws"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for CallbackInfo usage to see if any injections check for exceptions
rg -n "CallbackInfo" "src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionBasedStorage.java" -A 3 | head -40Length of output: 837
🏁 Script executed:
#!/bin/bash
# Search for any Mixin features related to exception handling in injections
find . -name "*.java" -path "*/mixin/*" | xargs grep -l "getCancelled\|setReturnValue\|throwException" 2>/dev/null | head -5Length of output: 253
🏁 Script executed:
#!/bin/bash
# Check what methods are exposed in the mixin interface to understand available tools
rg -n "interface.*CallbackInfo|class.*CallbackInfo" --type=java | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Determine if getRegionFile or close have exception handlers in the mixin file or are known to throw
cat "src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionFile.java" | head -150Length of output: 4811
🏁 Script executed:
#!/bin/bash
# Check the Redirect methods to see if they throw or handle exceptions
rg -n "@Redirect|throws IOException" "src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionBasedStorage.java"Length of output: 657
🏁 Script executed:
#!/bin/bash
# Look for Mixin-related exception handling support in the project
find . -name "pom.xml" -o -name "build.gradle" | xargs cat 2>/dev/null | grep -i "mixin\|asm" | head -10Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for @Inject usage with exception callbacks
rg -n "ExceptionHandler|@Inject.*THROW" "src/main/java/one/oktw/galaxy/mixin/"Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if there are any references to handler injection points
rg -n "@At.*HEAD|@At.*RETURN" "src/main/java/one/oktw/galaxy/mixin/tweak/" | wc -lLength of output: 2
🌐 Web query:
Minecraft RegionFileStorage flush IOException throws exception
💡 Result:
Short answer: RegionFileStorage (RegionFile/RegionFileStorage) calls underlying I/O methods (flush/sync/write/force) which declare and propagate IOException — so a flush can throw when the JVM’s underlying stream or file channel encounters an I/O error (disk full, permission/lock, closed stream, hardware/OS error, antivirus/backup lock, network filesystem glitch, etc.). See the RegionFile API which declares these methods throw IOException and standard Java docs for flush/force behavior. [1][2][3]
What to check and how to fix (quick checklist)
- Check the exact stack trace to confirm RegionFileStorage/RegionFile → which stream/channel threw the IOException.
- Disk space / quota: ensure target disk isn’t full.
- File permissions / ownership: process must be able to write.
- Concurrent access / locking: ensure no other process (backup/antivirus/another server) holds or deletes the file; synchronize chunk save threads.
- Filesystem type and reliability: network filesystems (NFS/SMB) and some virtual filesystems can cause intermittent IOExceptions — prefer local disk.
- Handle the exception: catch IOException around flush/sync and log full stack + file path; retry/backoff if transient.
- Close/replace broken streams: if a stream is closed or corrupted, recreate it (use try-with-resources).
- JVM / Minecraft version bug: update server implementation (vanilla/Spigot/Paper) if issue is reproducible and linked to a known bug.
- Hardware/OS logs: check dmesg/syslog for drive errors.
Relevant references
- RegionFile / RegionFileStorage Java API (methods declare IOException). [1][3]
- Java OutputStream/DataOutputStream flush() throws IOException — explains underlying I/O errors surface as IOException. [2]
If you paste the stack trace and Minecraft/server version (vanilla/Spigot/Paper and exact build), I can point to the exact failing call and give a targeted fix.
Sources:
[1] RegionFile (Yarn/Minecraft) API — constructor/methods throw IOException. (maven.fabricmc.net)
[2] Java DataOutputStream.flush() documentation (flush throws IOException). (docs.w3cub.com)
[3] RegionFile sync/writeChunk/delete methods (declare throws IOException). (maven.fabricmc.net)
Citations:
- 1: https://maven.fabricmc.net/docs/yarn-1.21.2-pre5%2Bbuild.2/net/minecraft/world/storage/RegionFile.html?utm_source=openai
- 2: https://docs.w3cub.com/openjdk~11/java.base/java/io/dataoutputstream?utm_source=openai
- 3: https://maven.fabricmc.net/docs/yarn-1.20.1%2Bbuild.9/net/minecraft/world/storage/RegionFile.html?utm_source=openai
🌐 Web query:
Spongepowered Mixin exception handler injection point @At THROW
💡 Result:
Summary
- @at("THROW") targets the bytecode ATHROW instruction and injects immediately before that throw. Use ordinal to pick a specific throw and shift if you need to move the insertion point relative to the matched instruction. [1][2][3]
Minimal example
- Inject before every throw in method myMethod():
@Inject(method = "myMethod", at = @at("THROW"))
private void onThrow(CallbackInfo ci) {
// runs immediately before the ATHROW
}
Notes / tips
- Use @at(value="THROW", ordinal = N) to select the Nth ATHROW in the method. [1][2]
- You can apply shift (At.Shift.BEFORE / AFTER / BY) to move the insertion relative to the matched instruction if needed. [1][3]
- The ATHROW instruction consumes a Throwable on the operand stack; the injection runs with the throw about to occur (so the exception reference is on the stack in bytecode terms), but capturing it in the Java callback requires careful use of local capture / correct callback signature and may not be available automatically. If you need the exception object, prefer redirecting the code that constructs/throws it or use a slice and an appropriate local-capturing injector. [1][2]
Sources
[1] Mixin — Injection Points / @at documentation.
[2] Mixin Wiki — Advanced Mixin Usage (Callback injectors).
[3] Mixin Javadoc (At / injector options: ordinal, shift, etc.).
Lock injection pattern leaves locks held permanently if wrapped methods throw exceptions
The review comment is valid. RegionFileStorage and RegionFile methods can throw IOException from disk full, permissions, concurrent access, or other I/O failures. With @At("HEAD") / @At("RETURN") injections, if flush() (or getRegionFile() or close()) throws before normal return, the @At("RETURN") unlock injection never runs, permanently holding the lock and deadlocking future region operations on this instance.
This pattern affects all three methods in lines 42–50 (getRegionFile), 52–60 (close), and 62–70 (flush). Consider using @At("THROW") injection (supported by Mixin) to unlock on exception paths, or refactor to a try-finally-equivalent pattern to guarantee unlock in all exit scenarios.
🤖 Prompt for AI Agents
In
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionBasedStorage.java
around lines 42–70, the current pattern injects lock.lock() at method HEAD and
lock.unlock() at RETURN causing locks to remain held if the target methods
throw; update each affected method pair (getRegionFile lines 42–50, close lines
52–60, flush lines 62–70) to guarantee unlock on all exit paths by either adding
corresponding @Inject(at=@At("THROW")) handlers that call lock.unlock() or by
refactoring the injections into a try-finally-equivalent: acquire the lock at
method entry and ensure unlock is always invoked in both RETURN and THROW
injection points (or a single handler that wraps the whole method body if
feasible), so exceptions no longer leave the lock permanently held.
src/main/java/one/oktw/galaxy/mixin/tweak/MixinCustomBlockEntity_BarrierBlock.java
Show resolved
Hide resolved
src/main/java/one/oktw/galaxy/mixin/tweak/MixinFixBeacon_BeaconBlockEntity.java
Show resolved
Hide resolved
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java
Outdated
Show resolved
Hide resolved
...in/java/org/spongepowered/common/mixin/realtime/entity/mob/ZombieVillagerMixin_RealTime.java
Show resolved
Hide resolved
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request is a significant undertaking to migrate the project to Mojang mappings. The changes are extensive, touching almost every part of the codebase, from build scripts to game logic and data generation. The migration appears to be thorough and correct, with numerous class, method, and field renames reflecting the new mappings. Dependency versions have been updated accordingly. The code quality is maintained throughout the changes. I have one minor suggestion regarding a leftover TODO comment in the code.
| @WrapOperation(method = "createAndPopulateRegistryMap", at = @At(value = "INVOKE", target = "Lnet/minecraft/registry/Registry;getId(Ljava/lang/Object;)Lnet/minecraft/util/Identifier;"), require = 0) | ||
| private static Identifier skipGalaxyEntries(Registry<?> instance, Object t, Operation<Identifier> original) { | ||
| Identifier id = original.call(instance, t); | ||
| // TODO(Ravel): no target class |
There was a problem hiding this comment.
This TODO comment seems to be outdated. The @WrapOperation targets a valid method Registry.getKey, and the mixin appears to be correct. It's good practice to remove resolved or obsolete TODO comments to keep the codebase clean and avoid confusion for future developers.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java (1)
63-63: Fix misleading method name (still present from previous review).The method name
realTimeImpl$adjustForRealTimeExperiencePickUpDelayis incorrect—it adjusts item cooldowns, not experience pickup delays. This appears to be a copy-paste error from another mixin.Apply this diff to correct the method name:
- private void realTimeImpl$adjustForRealTimeExperiencePickUpDelay(final ItemCooldowns self, final int modifier) { + private void realTimeImpl$adjustForRealTimeItemCooldowns(final ItemCooldowns self, final int modifier) {src/main/kotlin/one/oktw/galaxy/item/CustomItem.kt (1)
78-78: Fix type mismatch:Component?cannot be passed toITEM_NAME.
getName()returnsComponent?, but line 78 passes it directly toset(DataComponents.ITEM_NAME, ...), which expects a non-nullComponent. This will cause a compilation error.Apply this fix:
- set(DataComponents.ITEM_NAME, this@CustomItem.getName()) + this@CustomItem.getName()?.let { set(DataComponents.ITEM_NAME, it) }Alternatively, if the name should always be present, change the API signature to return non-null
Component:- abstract fun getName(): Component? + abstract fun getName(): ComponentBased on learnings from past reviews.
🧹 Nitpick comments (1)
src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (1)
80-130: Consider usingComponent.literalinstead ofComponent.nullToEmpty.The messaging API migration is correct (
sendSuccessanddisplayClientMessage), butComponent.nullToEmptyis typically used for handling potentially null strings. For hardcoded strings,Component.literalwould be more idiomatic and consistent with theComponent.translatableusage in other command files.Example for line 83:
-source.sendSuccess({ Component.nullToEmpty("請稍後...") }, false) +source.sendSuccess({ Component.literal("請稍後...") }, false)Note: Ideally, these hardcoded strings should use translation keys (like
Component.translatable), but that would be a larger refactoring effort.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java(2 hunks)src/main/kotlin/one/oktw/galaxy/block/event/AngelBlock.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/block/event/BlockEvents.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt(3 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/item/CustomItem.kt(3 hunks)src/main/kotlin/one/oktw/galaxy/item/Tool.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/item/Upgrade.kt(2 hunks)src/main/kotlin/one/oktw/galaxy/item/Weapon.kt(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt (1)
src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt (1)
execute(50-109)
src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (2)
src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt (1)
execute(52-121)src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt (1)
execute(50-109)
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java (1)
src/main/java/org/spongepowered/common/mixin/realtime/server/MinecraftServerMixin_RealTime.java (1)
Mixin(53-71)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java (1)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_ServerPlayNetworkHandler.java (1)
Mixin(38-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Docker-image
- GitHub Check: Docker-image
- GitHub Check: Summary
🔇 Additional comments (41)
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java (6)
3-3: LGTM! Copyright year updated appropriately.
45-45: LGTM! Mixin target updated correctly for Mojang mapping.The import and Mixin target have been updated from
ItemCooldownManagertoItemCooldowns, which aligns with the Minecraft API migration.Also applies to: 53-53
55-56: Excellent! Critical issue fixed.The
lastTickNanosfield is now instance-scoped (non-static), which resolves the critical race condition identified in the previous review. EachItemCooldownsinstance will now independently track its own time state, ensuring consistent and fair cooldown progression for all players.
59-59: LGTM! Shadow field updated for Mojang mapping.The shadow field correctly renamed from
ticktotickCountto match the target class field name.
61-62: LGTM! Redirect annotation correctly updated.The redirect target has been properly updated to reference
ItemCooldowns.tickCountwith the correct bytecode target.
67-67: LGTM! Implementation correctly updated.The cooldown accumulation now correctly uses
this.tickCountand properly castsrealTimeTicksto int.src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java (5)
3-3: LGTM!Copyright year updated appropriately for 2025.
21-28: LGTM!Import statements correctly updated to Mojang mappings as part of the migration.
40-41: No changes needed —getChunkNowis the correct method for map rendering.The
getChunkNowmethod does not trigger loading or generation; it only returns a currently-loaded chunk, making it a fast, synchronous lookup appropriate for map rendering that can tolerate missing chunks. The null check on line 41 correctly handles cases where chunks are not loaded, which is the proper pattern for this use case.
43-43: No issues found; pattern is safe for all environments.Verification shows this biome registry lookup pattern is isolated to this single location and safe. The PLAINS biome is a core vanilla Minecraft biome that always exists and cannot be removed by mods, making the
lookupOrThrowandgetOrThrowchain guaranteed to succeed in all environments. The codebase contains no error handling for registry operations, indicating developer confidence in this approach.
39-39: Remove the TODO comment at line 39.The replacement of
isTickingFutureReadywithisPositionTicking(chunkPos.toLong())is correct. The web search confirms thatisPositionTicking(long)is the modern API method for checking whether a chunk is actively ticking, withisTickingFutureReadybeing an older mapping for the same method. The code properly converts the chunk coordinates to a packed long position usingchunkPos.toLong(), and the semantic intent—verifying chunk ticking status before accessing it—is preserved. Remove the TODO comment since the change has been verified.src/main/kotlin/one/oktw/galaxy/item/CustomItem.kt (5)
36-43: Constructor parameter migrations look good.The type migrations align correctly with Mojang mappings (
ResourceLocation,CustomModelData), and themaxStackparameter naming is appropriate.
71-77: DataComponent migrations are correct.The component API migrations look good:
DataComponents.ITEM_MODELwithResourceLocationDataComponents.UNBREAKABLEwithUnit.INSTANCEItemAttributeModifierswith empty list
79-90: Component API migrations look correct.The remaining DataComponent migrations are properly implemented:
RARITY,MAX_STACK_SIZE,TOOLTIP_DISPLAYwith correct API usageTooltipDisplayconstructor andwithHidden()chaining patternCUSTOM_MODEL_DATAconditional assignment
92-100: Galaxy NBT data storage migration is correct.The migration from direct NBT manipulation to
DataComponents.CUSTOM_DATAwithCustomData.EMPTYand theupdate()pattern correctly preserves the galaxy data storage semantics.
65-69: The review comment is incorrect.getStringOr()is a built-in Minecraft NBT API method.The NBT API provides
getStringOr(key, fallback)on NBT containers such as CompoundTag, so line 66 is valid and requires no changes. The code correctly uses the standard Minecraft method; no compilation errors will occur.Likely an incorrect or invalid review comment.
src/main/kotlin/one/oktw/galaxy/item/Tool.kt (2)
21-27: Type migrations are correct.The migration to Mojang mappings is properly implemented:
ComponentandResourceLocationimportsResourceLocation.fromNamespaceAndPath("galaxy", "tool/$id")correctly replacesIdentifier.ofmaxStack = 1parameter naming matches the parent class
35-35:getName()signature updated correctly.The return type change from
TexttoComponentwithComponent.translatable()aligns with the Mojang mapping migration.src/main/kotlin/one/oktw/galaxy/item/Weapon.kt (2)
21-27: Type migrations are correct.The migration follows the same correct pattern as
Tool.kt:
- Proper
ComponentandResourceLocationimportsResourceLocation.fromNamespaceAndPath("galaxy", "weapon/$id")usagemaxStack = 1parameter
57-57:getName()implementation is correct.The return type and implementation align with the Mojang mapping migration using
Component.translatable().src/main/kotlin/one/oktw/galaxy/item/Upgrade.kt (2)
21-28: Type migrations are correct.The migration to Mojang mappings is properly implemented with:
Component,ResourceLocation, andItemStackimportsResourceLocation.fromNamespaceAndPath("galaxy", "upgrade/${type.id(level)}")maxStack = 1parameter
74-75:getName()implementation is correct.The
Component.translatable()usage with conditional level appending is properly implemented. Thealsoblock withappend()correctly builds the display name for leveled upgrades.src/main/kotlin/one/oktw/galaxy/block/event/BlockEvents.kt (4)
39-39: LGTM! API migration changes are consistent.The mechanical API changes throughout the file are consistent with the Mojang mapping migration:
ServerPlayerEntity→ServerPlayerentityWorld→level()ticks→tickCountHand→InteractionHandActionResult→InteractionResultgetStackInHand→getItemInHandisSneaking→isShiftKeyDownItemPlacementContext→BlockPlaceContextThe logic flow and behavior are preserved across all changes.
Also applies to: 44-44, 54-54, 59-62, 64-65, 67-67, 74-75, 80-80, 85-87, 91-91, 100-100
66-66: API pattern verified as correct. No changes needed.InteractionResult.Success is a record that carries a SwingSource, and the swingSource field is part of the record structure. SwingSource.SERVER exists as a valid enum value. The safe cast to
InteractionResult.Successfollowed by theswingSource()accessor method call and comparison toInteractionResult.SwingSource.SERVERis the correct API pattern.
78-78: BlockPlaceContext constructor signature verified as correct.The
BlockPlaceContextclass has a public constructor acceptingUseOnContext, and the codebase confirms thatPlayerUseItemOnBlock.contextis typed asUseOnContext. The codeBlockPlaceContext(event.context)correctly invokes this constructor with the proper type. No issues found.
22-26: Import statements verified as correct Mojang mappings for Minecraft 1.21.All imports in lines 22-26 match the official Mojang mappings:
CriteriaTriggers(net.minecraft.advancements.CriteriaTriggers),ServerPlayer(net.minecraft.server.level.ServerPlayer),InteractionHand(net.minecraft.world.InteractionHand),InteractionResult(net.minecraft.world.InteractionResult), andBlockPlaceContext(net.minecraft.world.item.context.BlockPlaceContext).src/main/kotlin/one/oktw/galaxy/block/event/AngelBlock.kt (4)
76-76: Past review concern has been resolved.The previous review flagged a redundant sound call on this line, but the current code shows only
justBroke.add(player)with no sound playback. The issue appears to have been addressed.
38-39: LGTM! API migration changes are consistent.The mechanical API changes are consistent with the Mojang mapping migration:
ServerPlayerEntity→ServerPlayer(lines 38-39)getStackInHand→getItemInHand(line 57)BlockPlaceContextconstructor with proper parameters (line 60)ServerboundPlayerActionPacket.Actionenum usage (line 71)entityWorld→level()(lines 72, 75)The logic and behavior are preserved.
Also applies to: 57-57, 60-60, 71-72, 75-75
22-27: All imports verified as correct for Minecraft 1.21 Mojang mappings.The imports accurately reflect the Minecraft 1.21 official mappings: ServerboundPlayerActionPacket, ServerPlayer, SoundEvents, SoundSource, BlockPlaceContext, and BlockHitResult all map to their correct package locations.
59-59: Verify the pick() method availability and correctness for Minecraft 1.21.10.Web search results are conflicting regarding method names in your target version. The initial search indicated that
pickmay be a legacy method being replaced byraycast, but subsequent results for 1.21.10 are inconclusive.You should verify directly in your environment:
- Confirm
pick()method exists and is valid forServerPlayerin Minecraft 1.21.10- If
pickis deprecated, migrate to the current replacement method- Verify the return type is indeed
BlockHitResult(or requires casting from a parent type)Check your IDE's method resolution or the Minecraft 1.21.10 Mojang mappings documentation for the authoritative Entity/ServerPlayer method signature.
src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt (5)
24-34: LGTM! Imports updated for Mojang mappings.The imports correctly reflect the migration from Fabric intermediary names to Mojang's official mappings.
43-50: LGTM! Command registration migrated correctly.The command registration has been properly updated to use
CommandSourceStackandCommands.literal, consistent with Mojang mappings.
67-88: LGTM! Teleport validation and countdown logic updated correctly.The teleport target validation uses
findRespawnPositionAndUseSpawnBlockwith the newTeleportTransition.DO_NOTHINGparameter, and the countdown displays correctly viadisplayClientMessagewith the actionbar flag.
90-115: LGTM! Teleport execution and respawn anchor handling migrated correctly.The teleport execution uses the new
teleportAPI accepting aTeleportTransition, world access usesgetLevel, and the respawn anchor charge consumption logic correctly uses the updated block state API.
52-65: Respawn data API migration verified correctly.The code correctly calls
pos()as a method (per Mojang's official mappings whereServerPlayerEntity.Respawn.pos()is the accessor returningBlockPos). The safe navigation chaining and messaging API usage are sound.src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (2)
26-44: LGTM! Imports and lock map updated for Mojang mappings.The imports and lock map key type (
ServerPlayer) correctly reflect the API migration.
46-78: LGTM! Command registration and argument handling migrated correctly.The registration correctly uses
CommandSourceStack,Commands.literal,GameProfileArgument, andNameAndId, withplayerOrExceptionreplacing the oldplayerOrThrowmethod.src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt (4)
25-48: LGTM! Imports and command registration updated correctly.The imports and command registration properly reflect the Mojang mapping migration, using
CommandSourceStack,Commands.literal, and other new API classes.
50-71: LGTM! Execute method signature and player refetch logic are correct.The countdown logic correctly uses
displayClientMessagewithComponent.translatable. The player refetch at line 66 is a good practice that handles the case where the player disconnects during the countdown, with proper lock cleanup on early return.
73-95: LGTM! Dimension checks and End spawn teleport logic migrated correctly.The world/level access uses
level(), dimension checks useLevelconstants, and the End spawn teleport correctly usesServerLevel.END_SPAWN_POINT.centerwithconnection.teleportandresetPosition.
97-108: LGTM! Spawn location calculation updated with new API methods.The spawn location logic correctly uses new API methods:
adjustSpawnLocation,bottomCenter,respawnData.pos()for world spawn data, andabsSnapTofor collision avoidance. The final teleport viaconnection.teleportis correct.
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_MapItem.java
Show resolved
Hide resolved
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_MapItem.java
Show resolved
Hide resolved
james58899
force-pushed
the
migrate-mapping
branch
from
9d4f51a to
c34ac95
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (1)
46-77: Fix self-target path: GameProfile cannot be passed directly to NameAndId constructor; extract UUID and name instead.The argumented path is correct. In Minecraft 1.21,
GameProfileArgument.getGameProfiles()returnsCollection<NameAndId>, which matches yourexecute()signature—no mapping needed.However, the self-target path has a mismatch:
execute(context.source, listOf(NameAndId(context.source.playerOrException.gameProfile)))
NameAndIdis a record with constructorNameAndId(UUID id, String name), notNameAndId(GameProfile). You need to extract the UUID and name from the GameProfile:val profile = context.source.playerOrException.gameProfile execute(context.source, listOf(NameAndId(profile.id, profile.name)))
🧹 Nitpick comments (6)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java (1)
39-39: Verify theisPositionTickingcheck for map rendering logic.The TODO comment flags uncertainty about this check. The
isPositionTickingmethod determines if a chunk is within the ticking range, but for map rendering, you may want to render any loaded chunk, not just actively ticking ones. This condition could potentially skip rendering valid loaded chunks that aren't in the ticking range.Consider whether this check should be
hasChunkor removed entirely to allow rendering of all loaded chunks. Would you like me to help investigate the appropriate condition for map rendering in the Mojang mapping context?src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java (1)
54-54: Consider renaming class for consistency.The class name
ItemCooldownManagerMixin_RealTimestill uses the old Yarn mapping name, while the mixin targetsItemCooldowns.class(Mojang mapping). For consistency and clarity, consider renaming the class toItemCooldownsMixin_RealTime.Apply this diff to align the class name with the mixin target:
-public abstract class ItemCooldownManagerMixin_RealTime { +public abstract class ItemCooldownsMixin_RealTime {Note: This change would also require updating the filename to match the new class name.
src/main/kotlin/one/oktw/galaxy/command/commands/admin/Creative.kt (1)
55-105: Creative GUI wiring looks correct; consider minor Slot/index refactor.The migration to
SimpleContainer+MenuType.GENERIC_9x3, and the separation betweenaddSlot(x=2..7) andeditInventory(columns 0 and 8) aligns well with the Galaxy GUI semantics whereeditInventoryonly affects non‑overlaid slots. That avoids the issue from the retrieved learning where GUI edits could conflict withaddSlot‑bound containers.Within the custom
Slot(Lines 73–87),slotIndex = i - 1works because it’s captured at construction time, but you can simplify and future‑proof this by using the built‑inSlot.indexfield insideonTakeinstead of tracking your ownslotIndex. Inventory population viainventory.clearContent()+inventory.setItem(i, ...)then naturally stays consistent with the slot indices.src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt (1)
50-107: Spawn execute migration looks correct; consider hardening lock + coroutine flow.The switch to
CommandSourceStack,Level.NETHER/END,player.position(),world.respawnData.pos(), andconnection.teleport(...)is consistent with Mojang mappings and mirrors the updatedHomelogic. The re‑lookup of the player after the countdown and the dimension checks all look functionally sound.Two robustness nits you may want to address (non‑blocking for this mappings PR):
lockis a plainHashSet<UUID>modified both on the command thread and inside the coroutine; ifmaindispatches onto a different thread, consider either guarding it (e.g., with a mutex) or switching to a concurrent structure to avoid subtle races.- Wrapping the coroutine body in a
try/finally { lock -= ... }would ensure the lock is always released even if an exception is thrown during teleport (same applies to theHomecommand).src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt (1)
52-121: Home respawn/anchor migration is aligned with new APIs; watch lock/threading semantics.This refactor to
CommandSourceStack,Componentmessaging,respawnConfig?.respawnData?.pos(),findRespawnPositionAndUseSpawnBlock(..., TeleportTransition.DO_NOTHING), and explicit respawn‑anchor charge consumption mirrors vanilla semantics and looks logically correct.As with
Spawn, the only caveat is structural:
lockis a mutableHashSet<UUID>touched from the command handler and from the coroutine started viamain?.launch. If that coroutine isn’t confined to the server thread, consider switchinglockto a thread‑safe structure or serializing access via a mutex.- A
try/finallyaround the coroutine body that always doeslock -= player.uuidwould make the lock resilient to unexpected exceptions during the teleport or anchor handling.These are optional hardening steps and not specific to the mappings migration itself.
src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (1)
80-129: Per-player Mutex lock for join is a solid improvement; only minor lifecycle nits.Using
ConcurrentHashMap<ServerPlayer, Mutex>withgetOrPut(...).tryLock()cleanly prevents overlapping/joinoperations per player and avoids the cross-threadHashSetmutation pattern used in other commands. The progress listener updates and messages forQueue,Creating,Starting,Started, andFailedstages are straightforward, and you correctly unlock + remove the mutex both on success/failure and after the 5‑minute timeout.Two small follow-ups you might consider:
- The
CoroutineScope by CoroutineScope(Dispatchers.Default + SupervisorJob())means you’re running this logic off the main server thread; ifProxyResponseEventanddisplayClientMessageare assumed to be main‑thread only, you may want to use the server’s coroutine dispatcher (or explicitly hop back to it) for the parts that touchServerPlayer.- In the suggestions provider (Lines 52–71), the
TODO check leakstill applies: if noSearchPlayer.Resultever arrives, theProxyResponseEventlistener stays registered forever because the future never completes. You could add a timeout or failure path that completes the future and unregisters the listener.Overall, the new locking and feedback flow look good; the above are minor hardening tweaks.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java(2 hunks)src/main/kotlin/one/oktw/galaxy/block/CustomBlockHelper.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt(6 hunks)src/main/kotlin/one/oktw/galaxy/block/event/AngelBlock.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt(3 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/admin/Creative.kt(4 hunks)src/main/kotlin/one/oktw/galaxy/command/commands/admin/RegisterBlock.kt(3 hunks)src/main/kotlin/one/oktw/galaxy/gui/GUISBackStackManager.kt(1 hunks)src/main/kotlin/one/oktw/galaxy/item/CustomItem.kt(3 hunks)src/main/kotlin/one/oktw/galaxy/item/Gui.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/kotlin/one/oktw/galaxy/command/commands/admin/RegisterBlock.kt
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java:35-37
Timestamp: 2025-11-18T04:07:14.594Z
Learning: In Minecraft 1.21 mapping migration from Yarn to Mojang mappings, the method FilledMapItem.updateColors in Yarn maps to MapItem.update in Mojang mappings. This is part of the standard Yarn → Mojang mapping migration in the MapItem class.
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java:33-33
Timestamp: 2025-11-18T04:06:50.553Z
Learning: In Minecraft 1.21 mapping migration from Yarn to Mojang mappings, the class FilledMapItem in Yarn (net.minecraft.item.FilledMapItem) maps to MapItem in Mojang mappings (net.minecraft.world.item.MapItem). This is part of the standard Yarn → Mojang mapping migration.
📚 Learning: 2025-11-18T04:22:38.273Z
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/kotlin/one/oktw/galaxy/block/entity/TrashcanBlockEntity.kt:65-77
Timestamp: 2025-11-18T04:22:38.273Z
Learning: In the Galaxy GUI system (one.oktw.galaxy.gui.GUI), the `editInventory` method is designed to only modify default slots that are not overlaid by explicit `addSlot` bindings. When slots are bound to a specific container via `addSlot`, those slots remain independent of the GUI's internal inventory that `editInventory` operates on.
Applied to files:
src/main/kotlin/one/oktw/galaxy/command/commands/admin/Creative.ktsrc/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt
📚 Learning: 2025-11-18T04:07:14.594Z
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java:35-37
Timestamp: 2025-11-18T04:07:14.594Z
Learning: In Minecraft 1.21 mapping migration from Yarn to Mojang mappings, the method FilledMapItem.updateColors in Yarn maps to MapItem.update in Mojang mappings. This is part of the standard Yarn → Mojang mapping migration in the MapItem class.
Applied to files:
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.javasrc/main/kotlin/one/oktw/galaxy/item/CustomItem.kt
📚 Learning: 2025-11-18T04:06:50.553Z
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java:33-33
Timestamp: 2025-11-18T04:06:50.553Z
Learning: In Minecraft 1.21 mapping migration from Yarn to Mojang mappings, the class FilledMapItem in Yarn (net.minecraft.item.FilledMapItem) maps to MapItem in Mojang mappings (net.minecraft.world.item.MapItem). This is part of the standard Yarn → Mojang mapping migration.
Applied to files:
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.javasrc/main/kotlin/one/oktw/galaxy/item/CustomItem.kt
📚 Learning: 2025-11-18T03:28:36.223Z
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinFixBeacon_BeaconBlockEntity.java:60-62
Timestamp: 2025-11-18T03:28:36.223Z
Learning: In Minecraft mapping migration from Yarn to Mojang mappings, the method `getNonSpectatingEntities` in Yarn maps directly to `getEntitiesOfClass` in Mojang mappings. Both methods automatically filter out spectator players, so no additional manual spectator filtering is needed when migrating between these mapping systems.
Applied to files:
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java
🧬 Code graph analysis (5)
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java (1)
src/main/java/org/spongepowered/common/mixin/realtime/server/MinecraftServerMixin_RealTime.java (1)
Mixin(53-71)
src/main/kotlin/one/oktw/galaxy/gui/GUISBackStackManager.kt (1)
src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (1)
CoroutineScope(43-131)
src/main/kotlin/one/oktw/galaxy/command/commands/Join.kt (2)
src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt (1)
execute(52-121)src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt (1)
execute(50-109)
src/main/kotlin/one/oktw/galaxy/command/commands/Home.kt (1)
src/main/kotlin/one/oktw/galaxy/command/commands/Spawn.kt (1)
execute(50-109)
src/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt (1)
src/main/kotlin/one/oktw/galaxy/gui/GUI.kt (3)
addBinding(105-109)addBinding(111-115)addBinding(117-123)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Docker-image
- GitHub Check: Docker-image
- GitHub Check: Summary
🔇 Additional comments (30)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java (3)
21-28: LGTM! Import statements correctly updated for Mojang mappings.The imports have been properly migrated to Mojang mapping package structure (e.g.,
net.minecraft.world.item.MapItem,net.minecraft.world.level.*), consistent with the PR's migration objective.Based on learnings
33-37: LGTM! Mixin target and redirect correctly migrated to Mojang mappings.The changes are correct for Yarn → Mojang mapping migration:
@Mixin(MapItem.class)correctly targets the Mojang-mapped class- Redirect method updated to target
Level.getChunk(...)andMapItem.update- Method signature properly updated from
World/WorldChunktoLevel/LevelChunkBased on learnings
43-43: LGTM! EmptyLevelChunk construction correctly updated for Mojang API.The fallback chunk construction properly uses the new Mojang mapping API:
EmptyLevelChunkreplaces the YarnEmptyChunk- Registry access via
registryAccess().lookupOrThrow(Registries.BIOME)- Reasonable PLAINS biome default for the fallback chunk
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownManagerMixin_RealTime.java (4)
56-56: Previous review comments successfully addressed.Both issues from the previous review have been fixed:
- The
lastTickNanosfield is now an instance field (removedstatic), eliminating the race condition across multipleItemCooldownsinstances.- The method name has been corrected from
realTimeImpl$adjustForRealTimeExperiencePickUpDelaytorealTimeImpl$adjustForRealTimeItemCooldown, accurately reflecting its purpose.Also applies to: 63-63
3-3: LGTM!Copyright year update is appropriate.
45-45: LGTM! Mapping migrations are consistent.All Yarn → Mojang mapping migrations are correctly applied:
ItemCooldownManager→ItemCooldownstickfield →tickCount- Redirect method target updated accordingly
The logic remains unchanged; only identifiers have been updated per the mapping migration.
Based on learnings
Also applies to: 53-53, 59-59, 61-62, 63-63, 67-67
55-56: LGTM! Good use of @unique annotation.The
@Uniqueannotation is best practice for Mixin-injected fields, preventing conflicts with other mixins and clearly marking the field as mixin-specific.src/main/kotlin/one/oktw/galaxy/block/event/AngelBlock.kt (3)
22-25: LGTM! Standard mapping migration.The import changes correctly reflect the Yarn → Mojang mapping migration:
ServerboundPlayerActionPacket,ServerPlayer,BlockPlaceContext, andBlockHitResultare the appropriate equivalents.
36-37: LGTM! Correct type migration.The field types have been correctly updated from
ServerPlayerEntitytoServerPlayer, consistent with the Mojang mapping migration.
69-73: LGTM! Correct API migration.The mapping changes are correct:
ServerboundPlayerActionPacket.Action.START_DESTROY_BLOCKandlevel()are the appropriate Mojang equivalents. The previous sound event issue has been properly addressed.src/main/kotlin/one/oktw/galaxy/block/CustomBlockHelper.kt (1)
54-56: LGTM! Resource leak fixed.The
ProblemReporter.ScopedCollectoris now properly closed using.use { }, addressing the previous review concern. This is the correct Kotlin idiom forAutoCloseableresources.src/main/kotlin/one/oktw/galaxy/item/CustomItem.kt (4)
22-32: LGTM: Import updates align with Mojang mapping migration.The import updates correctly replace Yarn mappings with Mojang equivalents (DataComponents, CompoundTag, Component, ResourceLocation, etc.).
36-43: LGTM: Constructor signature correctly updated for Mojang mappings.Parameter types properly migrated:
Identifier→ResourceLocation,CustomModelDataComponent→CustomModelData.
61-68: LGTM: NBT methods correctly migrated toCompoundTag.Method signatures properly updated from
NbtCompoundtoCompoundTag. ThegetStringOrusage at line 66 is valid in Minecraft 1.21.10 (as confirmed in past reviews).Based on learnings
74-100: Verify migrated DataComponents API usage.The migration to
DataComponentsAPI introduces several new patterns:
- Line 77:
ItemAttributeModifiers(emptyList())- Lines 82-87:
TooltipDisplayconstructor andwithHiddenchaining- Lines 95-99:
update(DataComponents.CUSTOM_DATA, CustomData.EMPTY)patternThese APIs are part of the Mojang mapping migration and should be verified against the target Minecraft version to ensure correct constructor signatures and method chaining.
src/main/kotlin/one/oktw/galaxy/item/Gui.kt (3)
21-24: LGTM: Import updates correctly migrated to Mojang mappings.Imports properly updated to use
Component,ResourceLocation, andCustomModelData.
26-33: LGTM: Constructor correctly migrated to useResourceLocation.The
ResourceLocation.fromNamespaceAndPathfactory method is used correctly for both the identifier and itemModel. The conditionalhideTooltip = name == nulllogic is appropriate.
35-35: LGTM:getName()implementation provides non-null guarantee.This implementation correctly overrides the abstract
Component?return type with a non-nullComponent, usingComponent.empty()as a safe fallback. This is valid Kotlin variance and avoids potential null-safety issues.src/main/kotlin/one/oktw/galaxy/command/commands/admin/Creative.kt (1)
131-136: Command registration migration is clean and consistent.Switching the
commandproperty toLiteralArgumentBuilder<CommandSourceStack>withCommands.literal("creative")andplayerOrExceptioncleanly matches the new command API and integrates withGUISBackStackManager.openGUIwithout behavioral changes.src/main/kotlin/one/oktw/galaxy/gui/GUISBackStackManager.kt (4)
32-34: API migration looks correct.The constructor and dispatcher changes align with the Mojang mapping migration (ServerPlayerEntity→ServerPlayer, level() accessor). The WeakReference wrapping of the player appears redundant given that
managersalready uses weak keys viaMapMaker().weakKeys()(line 38), but it's harmless defensive coding.
40-46: LGTM!The method signatures and API calls have been correctly updated to use
ServerPlayerandcloseContainer()for the Mojang mapping migration.
50-60: LGTM!The method correctly handles the WeakReference null case and updates the thread-checking and GUI-opening APIs for Mojang mappings. The async launch on line 56 is intentional per the comment to delay GUI opening by 1 tick.
62-74: LGTM!The callback correctly updates to the
Playerparameter type and usesopenMenu()with properOptionalhandling for the Mojang mapping migration.src/main/kotlin/one/oktw/galaxy/block/entity/TestGuiBlockEntity.kt (7)
21-53: LGTM!The imports and class declaration have been correctly migrated to Mojang mappings, with
ContainerreplacingInventoryand all package paths updated appropriately.
54-93: LGTM!The field declarations and GUI initialization correctly migrate to Mojang mappings:
Component.literalfor textNonNullListfor item listsMenuType.GENERIC_9x6for the menu typeResourceLocation.fromNamespaceAndPathfor resource identifiersClickType.PICKUPfor click actions
95-126: LGTM!The additional GUI initializations correctly use the updated Mojang mapping types (
MenuType.GENERIC_9x4,MenuType.ANVIL,Component.literal,ResourceLocation.fromNamespaceAndPath).
128-151: LGTM!The data persistence methods have been correctly migrated to Mojang mappings:
saveAdditionalreplaceswriteDatacollectImplicitComponentsreplacesaddComponentsapplyImplicitComponentsreplacesreadComponentsremoveComponentsFromTagreplacesremoveFromCopiedStackData- All use the appropriate helper classes (
ContainerHelper,ItemContainerContents)
153-156: LGTM!The
onClickhandler correctly updates to the new signature withPlayer/InteractionHand/BlockHitResultparameters and returnsInteractionResult.SUCCESS_SERVERper Mojang mappings.
158-177: LGTM!The Container interface methods are correctly implemented with Mojang mapping names:
clearContent()(wasclear())getContainerSize()(wassize())getItem()(wasgetStack())removeItem()(wasremoveStack())removeItemNoUpdate()(wasremoveStack()without amount)setChanged()(wasmarkDirty())
178-188: LGTM!The remaining Container interface methods are correctly implemented:
setItem()(wassetStack())stillValid()with proper use ofContainer.stillValidBlockEntity()helper
| world.setBlockState(pos, Blocks.AIR.defaultState) | ||
| Block.dropStack(world, pos, CustomBlock.registry.get(blockEntity.getId())!!.toItem()!!.createItemStack()) | ||
| world.setBlockAndUpdate(pos, Blocks.AIR.defaultBlockState()) | ||
| Block.popResource(world, pos, CustomBlock.registry.get(blockEntity.getId())!!.toItem()!!.createItemStack()) |
There was a problem hiding this comment.
Unsafe null assertions could cause crashes.
The chained !! operators assume both registry.get() and toItem() are non-null. If the registry is missing the block ID or toItem() returns null (e.g., during configuration changes or mod updates), this will crash with NPE.
Consider safer null handling:
- Block.popResource(world, pos, CustomBlock.registry.get(blockEntity.getId())!!.toItem()!!.createItemStack())
+ val item = CustomBlock.registry.get(blockEntity.getId())?.toItem()
+ if (item != null) {
+ Block.popResource(world, pos, item.createItemStack())
+ }Or, if these are guaranteed non-null by design, add a comment explaining why and use checkNotNull for clearer error messages.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Block.popResource(world, pos, CustomBlock.registry.get(blockEntity.getId())!!.toItem()!!.createItemStack()) | |
| val item = CustomBlock.registry.get(blockEntity.getId())?.toItem() | |
| if (item != null) { | |
| Block.popResource(world, pos, item.createItemStack()) | |
| } |
🤖 Prompt for AI Agents
In src/main/kotlin/one/oktw/galaxy/block/CustomBlockHelper.kt around line 76,
the expression uses chained non-null assertions (registry.get(... )!! and
toItem()!!) which can throw NPEs; replace these with safe null handling: fetch
the registry entry into a val, check it (or use checkNotNull with a clear
message if non-null is guaranteed), then call toItem() and guard that result too
(use ?.let { ... } or an elvis that logs/warns and returns early) before calling
createItemStack and Block.popResource so the code won't crash if either is
missing.
| val item = player.getItemInHand(event.packet.hand) | ||
| if (CustomItemHelper.getItem(item) == CustomBlockItem.ANGEL_BLOCK) { | ||
| val blockHit = player.raycast(3.0, 1.0f, false) as BlockHitResult | ||
| val placeContext = ItemPlacementContext(player, event.packet.hand, item, blockHit) | ||
| val blockHit = player.pick(3.0, 1.0f, false) as BlockHitResult | ||
| val placeContext = BlockPlaceContext(player, event.packet.hand, item, blockHit) |
There was a problem hiding this comment.
Use safe cast for raycast result.
The API migrations are correct (getItemInHand, pick, BlockPlaceContext), but the unsafe cast as BlockHitResult on line 57 could throw a ClassCastException if the raycast doesn't hit a block (e.g., returns a MISS type).
Apply this diff to use a safe cast and handle the null case:
- val blockHit = player.pick(3.0, 1.0f, false) as BlockHitResult
- val placeContext = BlockPlaceContext(player, event.packet.hand, item, blockHit)
- if (CustomBlockHelper.place(placeContext)) event.swing = true
+ val blockHit = player.pick(3.0, 1.0f, false) as? BlockHitResult ?: return
+ val placeContext = BlockPlaceContext(player, event.packet.hand, item, blockHit)
+ if (CustomBlockHelper.place(placeContext)) event.swing = true📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| val item = player.getItemInHand(event.packet.hand) | |
| if (CustomItemHelper.getItem(item) == CustomBlockItem.ANGEL_BLOCK) { | |
| val blockHit = player.raycast(3.0, 1.0f, false) as BlockHitResult | |
| val placeContext = ItemPlacementContext(player, event.packet.hand, item, blockHit) | |
| val blockHit = player.pick(3.0, 1.0f, false) as BlockHitResult | |
| val placeContext = BlockPlaceContext(player, event.packet.hand, item, blockHit) | |
| val item = player.getItemInHand(event.packet.hand) | |
| if (CustomItemHelper.getItem(item) == CustomBlockItem.ANGEL_BLOCK) { | |
| val blockHit = player.pick(3.0, 1.0f, false) as? BlockHitResult ?: return | |
| val placeContext = BlockPlaceContext(player, event.packet.hand, item, blockHit) |
🤖 Prompt for AI Agents
In src/main/kotlin/one/oktw/galaxy/block/event/AngelBlock.kt around lines 55 to
58, replace the unsafe cast of the raycast result with a safe cast (use `as?
BlockHitResult`) and handle the null/miss case before creating the
BlockPlaceContext; if the cast yields null or the hit is not a block,
return/exit early so you only construct BlockPlaceContext with a valid
BlockHitResult.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionFileStorage.java (1)
37-80: Lock/unlock injections can permanently deadlock RegionFileStorage on exceptions
getRegionFile,close, andflushall acquire the sameReentrantLockat@At("HEAD")and release it only at@At("RETURN"). If any of these methods throws (e.g., I/O error inflush()or while opening a region file), the@At("RETURN")injector never runs andlockstays held forever, blocking all subsequent region operations on this storage instance.This is the same structural issue previously flagged on the earlier RegionBasedStorage mixin; it has effectively been carried over to the new RegionFileStorage mapping.
Consider guaranteeing unlock on all exit paths, e.g. by adding matching
@At("THROW")injections for each method, or otherwise emulating a try–finally:@@ - @Inject(method = "getRegionFile", at = @At("HEAD")) - private void readLock(ChunkPos pos, CallbackInfoReturnable<RegionFile> cir) { - lock.lock(); - } - - @Inject(method = "getRegionFile", at = @At("RETURN")) - private void readUnlock(ChunkPos pos, CallbackInfoReturnable<RegionFile> cir) { - lock.unlock(); - } + @Inject(method = "getRegionFile", at = @At("HEAD")) + private void readLock(ChunkPos pos, CallbackInfoReturnable<RegionFile> cir) { + lock.lock(); + } + + @Inject(method = "getRegionFile", at = @At("RETURN")) + private void readUnlockReturn(ChunkPos pos, CallbackInfoReturnable<RegionFile> cir) { + lock.unlock(); + } + + @Inject(method = "getRegionFile", at = @At("THROW")) + private void readUnlockThrow(ChunkPos pos, CallbackInfoReturnable<RegionFile> cir) { + lock.unlock(); + } @@ - @Inject(method = "close", at = @At("HEAD")) - private void closeLock(CallbackInfo ci) { - lock.lock(); - } - - @Inject(method = "close", at = @At("RETURN")) - private void closeUnlock(CallbackInfo ci) { - lock.unlock(); - } + @Inject(method = "close", at = @At("HEAD")) + private void closeLock(CallbackInfo ci) { + lock.lock(); + } + + @Inject(method = "close", at = @At("RETURN")) + private void closeUnlockReturn(CallbackInfo ci) { + lock.unlock(); + } + + @Inject(method = "close", at = @At("THROW")) + private void closeUnlockThrow(CallbackInfo ci) { + lock.unlock(); + } @@ - @Inject(method = "flush", at = @At("HEAD")) - private void syncLock(CallbackInfo ci) { - lock.lock(); - } - - @Inject(method = "flush", at = @At("RETURN")) - private void syncUnlock(CallbackInfo ci) { - lock.unlock(); - } + @Inject(method = "flush", at = @At("HEAD")) + private void syncLock(CallbackInfo ci) { + lock.lock(); + } + + @Inject(method = "flush", at = @At("RETURN")) + private void syncUnlockReturn(CallbackInfo ci) { + lock.unlock(); + } + + @Inject(method = "flush", at = @At("THROW")) + private void syncUnlockThrow(CallbackInfo ci) { + lock.unlock(); + }The redirects for
read/scanChunktoRegionFileInputStream.galaxy$getChunkInputStreamNoSynclook consistent with the async I/O design; just ensureRegionFileis always mixed to implementRegionFileInputStreamso the cast cannot fail.You may want to double-check the Mixin docs for
@At("THROW")in your Mixin library version to confirm the exact callback signature and behavior before applying this pattern.src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/ZombieVillagerMixin_RealTime.java (1)
45-69: Conversion progress redirect and rate scaling remain correct after mapping migration.Targeting
ZombieVillagerdirectly, shadowinggetConversionProgress(), and returninggetConversionProgress() * ticksusingself.level()keeps the real‑time boost behavior consistent with the originalvillagerConversionTime - getConversionProgress()countdown logic that we already validated together earlier.
🧹 Nitpick comments (7)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_Projectile.java (1)
31-38: Mixin behavior is sound; consider small encapsulation/generics tweaksThe mixin correctly targets
Projectile, extendsEntity, and injects intotickto discard long‑lived projectiles; no functional issues stand out.Two minor, optional cleanups:
- Encapsulate the counter field:
- @unique
- int maxAge = 0;
- @unique
- private int maxAge = 0;
This keeps the added state internal to the projectile class after mixin application. 2. Tighten the constructor generic for better type clarity: ```diff - public MixinThrownCountdown_Projectile(EntityType<?> type, Level world) { + public MixinThrownCountdown_Projectile(EntityType<? extends Projectile> type, Level world) {Both changes are compile‑time niceties and should not affect runtime behavior.
Please confirm they compile cleanly with your current Minecraft/Mixin versions before adopting.
Also applies to: 40-44
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_MapItem.java (1)
39-39: Address the TODO comment.The TODO comment indicates uncertainty about the
isPositionTickingcheck. Please verify this logic is correct or clarify what needs to be checked.Do you want me to help investigate the correct usage of
isPositionTickingor open a new issue to track this task?src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_ServerGamePacketListener.java (1)
21-44: Player action cancel handling is correct; consider aligning with other packet-listener mixinsThe migration to
ServerGamePacketListenerImpl,ServerPlayer, andServerboundPlayerActionPacketlooks sound, and cancelling athandlePlayerAction’s head while resending aClientboundBlockUpdatePacketpluscontainerMenu.sendAllDataToRemote()should preserve the previous client resync behavior on cancel. For consistency with your otherServerGamePacketListenerImplmixins, you might optionally extendServerCommonPacketListenerImplhere too and use itssend(...)helper instead of going throughplayer.connection.send(...)directly.Please double-check against the decompiled
handlePlayerActionin your current mappings to ensure the method name/signature hasn’t changed and that emitting/cancelling at the head still matches your intended semantics for all action types.src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java (1)
38-55: Chat redirect preserves behavior; consider usingsenderfor clarityThe redirect cleanly wraps chat broadcasts with
PlayerChatEvent, maintaining the original broadcast when not canceled and logging canceled messages via the existing server logger. Functionally this aligns with the EmoteCommands mixin.If you want to tighten coupling to the actual sender passed at the call site, you could construct
PlayerChatEventfrom thesenderparameter instead of the shadowedplayerfield; in this context they should always be the same, so this is purely a clarity/style tweak.src/main/java/one/oktw/galaxy/mixin/tweak/MixinGlobalDataPack_ServerPacksSource.java (1)
28-33: Global datapack path override is consistent; be aware it’s relative to the server working dirThe
@ModifyArgcorrectly targetsServerPacksSource.createPackRepository(LevelStorageAccess)and rewrites thePathargument toPath.of("datapacks"), effectively redirecting datapack loading to a globaldatapacksdirectory. This is fine if you intend a shared/global datapack folder; just note that this path is relative to the server’s working directory, not the world folder.src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_IOWorker.java (2)
86-99: Region‑batched storePendingChunk logic is sound but can loop redundantly under contention.The overwrite:
- Groups
pendingWritesby region and schedules per‑region FOREGROUND tasks that callrunStorefor each(ChunkPos, PendingStore).- Uses an
AtomicBooleanwriteLockto ensure only one batch scheduling is active.- Schedules a BACKGROUND task to clear
writeLockand re‑invokestorePendingChunk().Because
runStoreis also guarded byremoveResults, duplicate scheduling caused by the BACKGROUND task racing ahead ofrunStoreis functionally harmless but can result in extra cancelledrunStoreinvocations.If you want to reduce that overhead, consider only scheduling the BACKGROUND follow‑up after the current batch has finished (e.g., tracking outstanding region tasks) rather than unconditionally rescheduling.
138-170: Parallel region scan is correct but may be more aggressive than necessary.
createOldDataForRegion:
- Runs in
Util.backgroundExecutor()and then usesChunkPos.rangeClosed(...).parallel()plusscanChunk(...).join()per chunk.- Synchronizes only around
BitSetmutation and collects minimal fields viaCollectFields.The logic for computing indices and calling
isOldChunklooks right, but nested parallelism (background executor + parallel stream) can oversubscribe threads on busy servers.If profiling ever shows high CPU usage here, consider:
- Dropping
.parallel()and relying on the background executor, or- Using a fixed‑size pool explicitly sized to available cores for the inner loop.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (34)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_ServerGamePacketListener.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_EmoteCommands.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_ServerGamePacketListener.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_ServerGamePacketListener.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_ServerGamePacketListener.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_IOWorker.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionFileStorage.java(3 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_ServerGamePacketListener.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinCustomBlockEntity_ClientboundLevelChunkPacketData.java(3 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinCustomBlockEntity_StructureTemplate.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinGUI_ServerGamePacketListener.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinGlobalDataPack_ServerPacksSource.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_MapItem.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinOptimizeArmorStand_ArmorStand.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_GenericThread.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_RconClient.java(3 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_Projectile.java(2 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_ThrowableProjectile.java(1 hunks)src/main/java/one/oktw/galaxy/mixin/tweak/SignStyle_ServerGamePacketListener.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/ExperienceOrbMixin_RealTime.java(1 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/CreeperMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/MobMixin_RealTime.java(1 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/ZombieVillagerMixin_RealTime.java(1 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/passive/AgeableMobMixin_RealTime.java(1 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownsMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/entity/player/PlayerMixin_RealTime.java(3 hunks)src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerGamePacketListenerMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerPlayerGameModeMixin_RealTime.java(2 hunks)src/main/java/org/spongepowered/common/mixin/realtime/world/ServerLevelMixin_RealTime.java(4 hunks)src/main/java/org/spongepowered/common/mixin/realtime/world/dimension/PortalProcessorMixin_RealTime.java(3 hunks)src/main/resources/galaxy.mixin.json(1 hunks)src/main/resources/sponge.realtime.mixin.json(1 hunks)src/main/resources/tweak.mixin.json(1 hunks)
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java:35-37
Timestamp: 2025-11-18T04:07:14.594Z
Learning: In Minecraft 1.21 mapping migration from Yarn to Mojang mappings, the method FilledMapItem.updateColors in Yarn maps to MapItem.update in Mojang mappings. This is part of the standard Yarn → Mojang mapping migration in the MapItem class.
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java:33-33
Timestamp: 2025-11-18T04:06:50.553Z
Learning: In Minecraft 1.21 mapping migration from Yarn to Mojang mappings, the class FilledMapItem in Yarn (net.minecraft.item.FilledMapItem) maps to MapItem in Mojang mappings (net.minecraft.world.item.MapItem). This is part of the standard Yarn → Mojang mapping migration.
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinFixBeacon_BeaconBlockEntity.java:60-62
Timestamp: 2025-11-18T03:28:36.223Z
Learning: In Minecraft mapping migration from Yarn to Mojang mappings, the method `getNonSpectatingEntities` in Yarn maps directly to `getEntitiesOfClass` in Mojang mappings. Both methods automatically filter out spectator players, so no additional manual spectator filtering is needed when migrating between these mapping systems.
📚 Learning: 2025-11-18T04:07:14.594Z
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java:35-37
Timestamp: 2025-11-18T04:07:14.594Z
Learning: In Minecraft 1.21 mapping migration from Yarn to Mojang mappings, the method FilledMapItem.updateColors in Yarn maps to MapItem.update in Mojang mappings. This is part of the standard Yarn → Mojang mapping migration in the MapItem class.
Applied to files:
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_MapItem.javasrc/main/resources/tweak.mixin.json
📚 Learning: 2025-11-18T04:06:50.553Z
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_FilledMapItem.java:33-33
Timestamp: 2025-11-18T04:06:50.553Z
Learning: In Minecraft 1.21 mapping migration from Yarn to Mojang mappings, the class FilledMapItem in Yarn (net.minecraft.item.FilledMapItem) maps to MapItem in Mojang mappings (net.minecraft.world.item.MapItem). This is part of the standard Yarn → Mojang mapping migration.
Applied to files:
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_MapItem.java
📚 Learning: 2025-11-18T03:28:36.223Z
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/java/one/oktw/galaxy/mixin/tweak/MixinFixBeacon_BeaconBlockEntity.java:60-62
Timestamp: 2025-11-18T03:28:36.223Z
Learning: In Minecraft mapping migration from Yarn to Mojang mappings, the method `getNonSpectatingEntities` in Yarn maps directly to `getEntitiesOfClass` in Mojang mappings. Both methods automatically filter out spectator players, so no additional manual spectator filtering is needed when migrating between these mapping systems.
Applied to files:
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_MapItem.java
📚 Learning: 2025-11-18T04:22:38.273Z
Learnt from: james58899
Repo: OKTW-Network/Galaxy PR: 474
File: src/main/kotlin/one/oktw/galaxy/block/entity/TrashcanBlockEntity.kt:65-77
Timestamp: 2025-11-18T04:22:38.273Z
Learning: In the Galaxy GUI system (one.oktw.galaxy.gui.GUI), the `editInventory` method is designed to only modify default slots that are not overlaid by explicit `addSlot` bindings. When slots are bound to a specific container via `addSlot`, those slots remain independent of the GUI's internal inventory that `editInventory` operates on.
Applied to files:
src/main/java/one/oktw/galaxy/mixin/tweak/MixinGUI_ServerGamePacketListener.java
🧬 Code graph analysis (15)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_MapItem.java (1)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_ServerGamePacketListener.java (1)
Mixin(38-70)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_GenericThread.java (1)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_RconClient.java (1)
Mixin(32-43)
src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerPlayerGameModeMixin_RealTime.java (2)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_ServerGamePacketListener.java (1)
Mixin(42-70)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_ServerGamePacketListener.java (1)
Mixin(39-62)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_IOWorker.java (2)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionFileStorage.java (1)
Mixin(37-81)src/main/kotlin/one/oktw/galaxy/util/KotlinCoroutineTaskExecutor.kt (1)
schedule(42-45)
src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerGamePacketListenerMixin_RealTime.java (5)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_ServerGamePacketListener.java (1)
Mixin(33-46)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java (1)
Mixin(38-56)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_ServerGamePacketListener.java (1)
Mixin(34-52)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_ServerGamePacketListener.java (1)
Mixin(42-70)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_ServerGamePacketListener.java (1)
Mixin(39-62)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_ServerGamePacketListener.java (5)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java (1)
Mixin(38-56)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_ServerGamePacketListener.java (1)
Mixin(34-52)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_ServerGamePacketListener.java (1)
Mixin(42-70)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_ServerGamePacketListener.java (1)
Mixin(39-62)src/main/java/one/oktw/galaxy/mixin/tweak/MixinGUI_ServerGamePacketListener.java (1)
Mixin(31-42)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_ServerGamePacketListener.java (4)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_ServerGamePacketListener.java (1)
Mixin(33-46)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_ServerGamePacketListener.java (1)
Mixin(39-62)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)src/main/kotlin/one/oktw/galaxy/event/type/CancelableEvent.kt (1)
cancel(21-26)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinGUI_ServerGamePacketListener.java (2)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java (1)
Mixin(38-56)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_ServerGamePacketListener.java (1)
Mixin(34-52)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java (2)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_EmoteCommands.java (1)
Mixin(34-51)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)
src/main/java/one/oktw/galaxy/mixin/tweak/SignStyle_ServerGamePacketListener.java (4)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_ServerGamePacketListener.java (1)
Mixin(33-46)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java (1)
Mixin(38-56)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_ServerGamePacketListener.java (1)
Mixin(34-52)src/main/java/one/oktw/galaxy/mixin/tweak/MixinGUI_ServerGamePacketListener.java (1)
Mixin(31-42)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_ServerGamePacketListener.java (4)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_ServerGamePacketListener.java (1)
Mixin(33-46)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_ServerGamePacketListener.java (1)
Mixin(42-70)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)src/main/kotlin/one/oktw/galaxy/event/type/CancelableEvent.kt (1)
cancel(21-26)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_RegionFileStorage.java (1)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_IOWorker.java (1)
Mixin(48-171)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_EmoteCommands.java (2)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java (1)
Mixin(38-56)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_ServerGamePacketListener.java (3)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_ServerGamePacketListener.java (1)
Mixin(33-46)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java (1)
Mixin(38-56)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_ServerGamePacketListener.java (1)
Mixin(34-52)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_ServerGamePacketListener.java (7)
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerAction_ServerGamePacketListener.java (1)
Mixin(33-46)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_ServerGamePacketListener.java (1)
Mixin(38-56)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_ServerGamePacketListener.java (1)
Mixin(42-70)src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_ServerGamePacketListener.java (1)
Mixin(39-62)src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_ServerGamePacketListener.java (1)
Mixin(38-70)src/main/java/one/oktw/galaxy/mixin/tweak/MixinGUI_ServerGamePacketListener.java (1)
Mixin(31-42)src/main/kotlin/one/oktw/galaxy/event/EventManager.kt (1)
safeEmit(44-45)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Docker-image
- GitHub Check: Docker-image
- GitHub Check: Summary
🔇 Additional comments (52)
src/main/java/one/oktw/galaxy/mixin/tweak/MixinOptimizeArmorStand_ArmorStand.java (1)
21-21: Verify thattickCrammingcorrectly maps topushEntitiesin Mojang mappings for Minecraft 1.21.The mapping migration changes appear consistent overall (import path, mixin target, class name are all updated correctly from Yarn to Mojang mappings). However, web searches did not confirm that
tickCramming(Yarn) maps topushEntitiesin official Mojang mappings. The initial search suggested these are community-sourced deobfuscation names that map to obfuscated Mojang method IDs, not direct method name equivalents.Please verify this method mapping against your build configuration, mapping files, or by testing the compiled code to ensure
pushEntitiesis the correct target method in your Minecraft 1.21 Mojang mapping version.src/main/java/org/spongepowered/common/mixin/realtime/entity/passive/AgeableMobMixin_RealTime.java (2)
45-45: LGTM: Import and mixin target migration.The import, mixin target, and class name have been consistently updated from
PassiveEntitytoAgeableMobas part of the Yarn to Mojang mapping migration.Also applies to: 53-54
58-62: Verify the level() method implementation and type compatibility.The mapping change from
tickMovementtoaiStepis correct—both map to the same intermediary method. However, the code relies onthis.level()returning an object compatible with casting toRealTimeTrackingBridge. Confirm thatlevel()exists on the entity and that the level class properly implements the bridge interface.src/main/java/org/spongepowered/common/mixin/realtime/entity/ExperienceOrbMixin_RealTime.java (4)
45-45: LGTM! Import correctly updated for Mojang mappings.The import has been properly migrated from Yarn (
ExperienceOrbEntity) to Mojang mappings (ExperienceOrb) with the correct package path.
53-54: LGTM! Mixin target and class name correctly updated.The mixin target and class name have been properly aligned with the Mojang mapping (
ExperienceOrb), maintaining consistency with the import changes.
56-56: Shadow field correctly mapped to ExperienceOrb.age.The shadow field declaration
private int age;is verified as correct. The@Redirectannotation at line 58 explicitly referencesLnet/minecraft/world/entity/ExperienceOrb;age:I, confirming thatageis the proper Mojang field name in theExperienceOrbclass with typeint.
58-61: Redirect logic appears correctly implemented, but requires manual verification of Entity.level() mapping.The code changes are internally consistent and properly migrated:
- Redirect target descriptor correctly formatted:
Lnet/minecraft/world/entity/ExperienceOrb;age:I- Method parameter type and field references align with Mojang mappings
- Real-time tick adjustment logic preserved
However, the key dependency—that
level()is the correct Mojang mapping for Yarn'sgetEntityWorld()—could not be verified through available documentation. The migration pattern is consistent with confirmed Yarn→Mojang migrations in your learnings, but specific validation of this Entity method requires manual confirmation against official Minecraft Mojang mappings or existing codebase usage.src/main/java/org/spongepowered/common/mixin/realtime/world/ServerLevelMixin_RealTime.java (4)
1-17: License header year update looks fineExtending the copyright range to 2018–2025 is consistent with the project’s current timeline; no issues here.
45-72: ServerLevel migration (imports, mixin target, shadowed data field, constructor) looks consistent; please confirm signatures vs target MC versionThe switch to Mojang-mapped types (
ServerLevel,Level,Holder<DimensionType>,RegistryAccess,WritableLevelData,ServerLevelData) and the@Mixin(ServerLevel.class)target, plus extendingLevel, all look coherent. The shadowedServerLevelData serverLevelDataand the synthetic constructor delegating tosuper(...)also match what Level typically expects in Mojang mappings.Since mixin correctness is very sensitive to exact field and constructor signatures, please double-check against your target Minecraft/Sponge version that:
ServerLevelreally has aprivate final ServerLevelData serverLevelData(same name and type), andLevel’s constructor signature still matches the one used here (WritableLevelData, ResourceKey<Level>, RegistryAccess, Holder<DimensionType>, boolean, boolean, long, int).If those match, this part should be safe.
74-80: ShadowedsetDayTime(long)method nameShadowing
setDayTime(long)instead of the oldersetTimeOfDay(long)aligns with Mojang mappings, and using it from the injected logic is appropriate.Please just confirm that in your mapped
ServerLevelthe mutator is indeed namedsetDayTime(long)(and not, e.g., a renamed or differently-typed variant), otherwise the mixin will fail to apply.
81-90: Real-time tick injection: logic preserved, game rule and time accessors look correctThe
tickTime@InjectatHEADkeeps the existing real-time adjustment logic:
- Daylight-cycle check migrated to
this.serverLevelData.getGameRules().getBoolean(GameRules.RULE_DAYLIGHT), which matches Mojang’sRULE_DAYLIGHTkey.- Time delta computation (
realTimeBridge$getRealTimeTicks() - 1) and thediff > 0guard preserve the original behavior of only advancing when real time is ahead, avoiding redundant updates for listeners.- Using
this.levelData.getDayTime()as the base when callingthis.setDayTime(...)is consistent with Mojang’sLeveldata field.Functionally, this should behave the same as the pre-migration version while using the new types. The only thing to verify is that:
GameRules.RULE_DAYLIGHTis indeed the key corresponding to the old daylight-cycle rule in your mapped version, andlevelData.getDayTime()is the correct replacement for any previousgetTimeOfDay()accessor.If both align, this mixin should keep real-time daylight syncing intact.
src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_Projectile.java (2)
1-17: License header year update is fineThe copyright year range including 2025 is consistent with the rest of the header and needs no further changes.
21-27: Mojang-mapping imports and@Uniqueusage look correctThe remapped imports to
net.minecraft.world.entity.*,net.minecraft.world.entity.projectile.Projectile, andnet.minecraft.world.level.Levelalign with Mojang mappings, and adding@UniquetomaxAgeis the right approach to avoid potential field-name collisions in the targetProjectileclass.If you want extra safety, double‑check against your Gradle‑mapped sources that
Projectile’s constructor and package paths match these imports in the exact Minecraft version you build against.Also applies to: 33-34
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/PlayerMixin_RealTime.java (3)
45-56: LGTM! Systematic mapping migration from Yarn to Mojang.The class-level changes correctly migrate from
PlayerEntitytoPlayer, updating the import, mixin target, and class references consistently.
62-91: Mapping migration applied correctly for experience and sleep timers.The redirect methods properly update field targets, parameter types, and world access from
getEntityWorld()tolevel(). The shadowed fields are correctly referenced.
93-114: Wake timer redirect correctly migrated.The redirect properly updates the slice boundaries and field references for the Mojang mapping migration.
src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerPlayerGameModeMixin_RealTime.java (2)
45-55: Mixin target migration looks correct.The migration from
ServerPlayerInteractionManager(Yarn) toServerPlayerGameMode(Mojang) is consistent with the Minecraft 1.21 mapping migration, and the class name has been updated accordingly.
57-73: All Mojang mapping names verified as correct.The web search confirms that ServerPlayerGameMode has a protected field
levelof type ServerLevel, a private fieldgameTicksof type int, and atick()method. The mixin code correctly references these names in both the@Shadowdeclarations and the redirect target annotation.src/main/java/org/spongepowered/common/mixin/realtime/server/network/ServerGamePacketListenerMixin_RealTime.java (2)
45-65: Mixin target and constructor migration is consistent with the codebase.The migration to
ServerGamePacketListenerImplwith the updated constructor signature matches the pattern used in other migrated mixins (MixinPlayerChat_ServerGamePacketListener,MixinPlayerInteractBlock_ServerGamePacketListener, etc.).
67-79: Based on my verification:
The Yarn 1.21.5 mappings contain
Cooldownclass innet.minecraft.utilwith atick()method.The first web search confirmed that in Mojang 1.21 mappings,
ServerGamePacketListenerImplcontains twoTickThrottler-typed fields used for throttling chat and dropped-item spam.The retrieved learning from a prior mapping migration confirms that in Minecraft 1.21, the Yarn
Cooldownclass maps to the MojangTickThrottlerclass—consistent with this standard Yarn → Mojang migration.The redirect correctly targets
TickThrottler.tick(), preserving the throttling logic while properly updating to the Mojang mapping. The implementation is accurate.
src/main/java/org/spongepowered/common/mixin/realtime/world/dimension/PortalProcessorMixin_RealTime.java (2)
43-47: Real-time portal decay adjustment correctly updated fromticksInPortaltoportalTimeThe decay injection now operates on
portalTimeinstead ofticksInPortalwhile preserving the prior logic (ticks > 0thenportalTime = max(0, portalTime - ticks * 4)), so behavior should remain consistent after the mapping rename. No functional regressions stand out here.
21-38: Mixin target, imports, and injection signature confirmed correctAll mapped names and signatures for
net.minecraft.world.entity.PortalProcessorare confirmed in the Mojang 1.21 mappings: fieldint portalTime, methodboolean processPortalTeleportation(ServerLevel, Entity, boolean), and methodvoid decayTick(). The imports, field shadowing, and@Atdescriptors are all accurate for this build.src/main/java/one/oktw/galaxy/mixin/tweak/MixinMapExistingChunk_MapItem.java (4)
33-34: LGTM! Mixin target is correct for Mojang mappings.The
@Mixin(MapItem.class)correctly targets the Mojang mapping equivalent of Yarn'sFilledMapItem.Based on learnings
35-35: LGTM! Redirect target is correct for Mojang mappings.The method name
updateis the correct Mojang mapping for Yarn'supdateColorsmethod.Based on learnings
36-44: No duplicate mixins found—migration is clean.Verification confirms:
- Old
MixinMapExistingChunk_FilledMapItem.javahas been removed- Only
MixinMapExistingChunk_MapItem.javaexists with correct@Mixin(MapItem.class)annotation targeting the new Mojang mapping- No lingering Yarn mapping references to FilledMapItem remain
37-37: The unsafe cast concern is based on an incorrect assumption.MapItem.update executes server-side only—map data mutations occur server-side, and the client only renders updates via
MapItemRendererwhen receiving packets. Since the mixin interceptsLevel.getChunk(II)within this server-side-only method, theLevelparameter will always be aServerLevel. The cast on line 37 is safe and will not throwClassCastException.Note: The TODO comment on line 39 remains a separate concern unrelated to cast safety.
Likely an incorrect or invalid review comment.
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerChat_EmoteCommands.java (3)
21-27: LGTM! Import migration follows standard Mojang mappings.The imports have been correctly updated from Yarn to Mojang mappings, consistent with the standard naming conventions.
41-50: API signature verified ✓ — No issues found.The method call at line 46 matches the Mojang mappings exactly:
broadcastChatMessage(message, source, messageType)correctly uses the overloadbroadcastChatMessage(PlayerChatMessage message, CommandSourceStack sender, ChatType.Bound boundChatType).All prior observations remain valid: null-safety is sound, API migrations are correct, and the logic is well-structured.
34-38: Verify the synthetic method mapping during build/test.The mixin structure and class targeting are correct for Minecraft 1.21.10. The
EmoteCommandsclass exists and is properly imported. However, the synthetic method namemethod_43645cannot be definitively verified without decompiling the Minecraft 1.21.10 jar, as synthetic methods (lambdas, anonymous classes) are compiler-generated and not exposed in public API mappings.The handler method signature correctly matches the redirect target. To confirm
method_43645is accurate:
- Build the mod and verify the mixin applies successfully at runtime
- If the mixin fails to apply, the method name will need to be updated for this Minecraft version
src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/MobMixin_RealTime.java (1)
45-65: Real-time despawn tracking migration toMob.noActionTimelooks correctTargeting
Moband redirecting thenoActionTimefield write inserverAiStepis consistent with the Mojang mappings migration, and usingself.level()for theRealTimeTrackingBridgecast preserves the intended real-time tick scaling while keeping the logic local to the mob instance. The unusedmodifierparameter is expected for a@Redirectof aPUTFIELDand doesn’t affect behavior here.It’s worth re-running your realtime/despawn tests (or enabling Mixin debug logging) to confirm that in the current Minecraft version there’s still a single
PUTFIELD Mob.noActionTimeinserverAiStepthat this redirect cleanly replaces.src/main/java/org/spongepowered/common/mixin/realtime/entity/player/ItemCooldownsMixin_RealTime.java (1)
45-68: Real-time cooldown tick redirect matches the newItemCooldownsAPIRedirecting the first
tickCountwrite inItemCooldowns.tick()and advancingtickCountbased on elapsedSystem.nanoTime()(with a minimum of 1 tick) preserves the realtime cooldown behavior while correctly targeting the Mojang-mappedItemCooldownsclass and itstickCountfield. The unusedmodifieris expected for a field@Redirectand does not affect correctness in this pattern.Since this relies on
ordinal = 0for thetickCountPUTFIELD, keep an eye on the decompiledItemCooldowns.tick()when you bump Minecraft versions again to ensure the first assignment is still the right interception point and no additionaltickCountwrites were introduced before it.src/main/java/one/oktw/galaxy/mixin/tweak/MixinCustomBlockEntity_StructureTemplate.java (1)
1-52: Barrier workaround hook is correct but relies on a brittle local signature/ordinalInjecting into
StructureTemplate.placeInWorldand replacingBlocks.BARRIERwithBlocks.AIRusingBlock.UPDATE_INVISIBLE | Block.UPDATE_KNOWN_SHAPEis a clean, minimal fix for the structure-barrier issue and aligns with the newStructureTemplate/ServerLevelAccessorAPIs. The only caveat is that the very long local parameter list plusordinal = 1on thesetBlock(...)call tightly couples this mixin to the current decompiled structure ofplaceInWorld, so it may require adjustment if Mojang changes that method’s body or local layout.When upgrading Minecraft again, it would be good to re-verify that
placeInWorldstill has the same secondsetBlockcall and compatible locals, or consider narrowing the injection to a more stable pattern (e.g., a@Redirecton the specificsetBlockwrite) to reduce maintenance.src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractBlock_ServerGamePacketListener.java (1)
19-69: Block-interact cancel hook and client resync sequence look solidHooking
handleUseItemOnjust beforeServerPlayerGameMode.useItemOn(...)and, on cancel, sendingUSE_ITEM_COMPLETE, health, and two block updates pluscontainerMenu.sendAllDataToRemote()matches the established pattern in your other interaction mixins and should keep the client in sync when aPlayerInteractBlockEventis cancelled (with optional swing handling). ExtendingServerCommonPacketListenerImpland using itssend(...)API is also consistent with the rest of the ServerGamePacketListener mixins in this PR.As this relies on the exact
handleUseItemOnname and theServerPlayerGameMode.useItemOncall being present, re-check the decompiledServerGamePacketListenerImplwhen you upgrade Minecraft to ensure the injection point and descriptor are still accurate.src/main/resources/galaxy.mixin.json (1)
6-12: ServerGamePacketListener mixin list matches the new event mixin classesThe mixin entries line up with the new
ServerGamePacketListenerImpl-based implementations and the EmoteCommands chat hook; the config structure looks correct.src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_RconClient.java (1)
21-37: RCON local‑client detection and logging suppression remain consistentSwitching to
ServerInterfaceand sharing theisLocalflag viaMixinRCON_GenericThreadpreserves the previous behavior: loopback RCON clients are marked local in the constructor and bothstartandrunlogs respect that flag. The injection/redirect signatures look coherent with that design.src/main/java/one/oktw/galaxy/mixin/tweak/MixinRCON_GenericThread.java (1)
21-36: SharedisLocalflag and start‑log redirect are wired correctlyTargeting
GenericThreadwith a@Unique isLocalflag and redirecting thestartlogger call means only threads explicitly marked local (via the RconClient mixin) suppress their startup log; other threads continue logging normally. The access level and placement ofisLocalwork well for this pattern.src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInteractItem_ServerGamePacketListener.java (1)
39-61: Item‑use event hook and cancelation resync look soundThe injection point, event emission, cancel handling (including USE_ITEM_COMPLETE + health + inventory resync), and optional swing behavior are all consistent with the existing block‑interaction mixin patterns and should behave correctly for item uses.
src/main/java/one/oktw/galaxy/mixin/tweak/MixinCustomBlockEntity_ClientboundLevelChunkPacketData.java (1)
33-38: ClientboundLevelChunkPacketData redirect looks correct after mapping migrationTargeting
ClientboundLevelChunkPacketData’s LevelChunk constructor and filtering outCustomBlockEntityinstances via theentrySet()redirect preserves the original behavior with the new mappings; usingCollectors.toSet()is acceptable here as long as the vanilla code only iterates the set (which matches the usual usage in this constructor).src/main/java/one/oktw/galaxy/mixin/tweak/SignStyle_ServerGamePacketListener.java (1)
31-39: Sign style redirect correctly updated to ServerGamePacketListenerImplThe mixin target, method name (
handleSignUpdate), and imports are adjusted to the new mappings, and the existing style-code replacement logic is preserved. Marking thestylesPatternas@Uniqueavoids collisions with the target class’ fields.src/main/java/one/oktw/galaxy/mixin/tweak/MixinGUI_ServerGamePacketListener.java (1)
31-39: GUI rename hook matches new packet/listener mappingsTargeting
ServerGamePacketListenerImpl.handleRenameItem(ServerboundRenameItemPacket)and usingplayer.containerMenuto locateGUI.GuiContainerkeeps the previous behavior under the Mojang mappings. Usingpacket.getName()as the source forupdateInputTextis consistent with the packet API.src/main/java/org/spongepowered/common/mixin/realtime/entity/mob/CreeperMixin_RealTime.java (1)
55-94: Real-time creeper fuse adjustment correctly remapped toCreeperfieldsThe mixin now shadows
swell/maxSwelland usesgetSwellDir()plusRealTimeTrackingBridge.realTimeBridge$getRealTimeTicks()to scale fuse progression, redirecting the firstCreeper.swellwrite inside thetickfuse slice. The delay logic atmaxSwellis preserved, and the cast toself.level()asRealTimeTrackingBridgematches the broader RealTime world mixin pattern.src/main/java/one/oktw/galaxy/mixin/tweak/MixinThrownCountdown_ThrowableProjectile.java (1)
35-51: ThrowableProjectile redirect and extra water hit detection look soundThe redirect correctly wraps
ProjectileUtil.getHitResultOnMoveVectorintick, preserves the vanilla hit result, and only adds an extra block raycast when the original result isMISSandgalaxy$getIntoWater() > 10, using the newposition()/getDeltaMovement()andClipContextAPIs. This keeps behavior compatible with the updated mappings while extending it for the thrown-countdown logic; just ensureThrowableProjectile(or its super) is consistently mixed to implementIThrownCountdown_Entityso the cast onthiscannot fail.If you want to double-check, search the repo for where
IThrownCountdown_Entityis implemented and confirm it’s applied to all throwable projectiles that can reach this code path.src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_ServerGamePacketListener.java (3)
38-49: Shadow declarations look consistent with ServerGamePacketListenerImpl API.
player,clampHorizontal(double)andteleport(double, double, double, float, float)match the expected fields/methods onServerGamePacketListenerImpl, and using a dummy body for the static shadow is standard Mixin practice.
51-62: Movement cancellation when entering non‑ticking chunks is logically sound.The
handleMovePlayerinjection correctly:
- Ignores rotation‑only packets (
!packet.hasPosition()).- Computes target section using
clampHorizontal(packet.getX/Z(...))andSectionPos.posToSectionCoord.- Cancels processing when
isPositionTickingis false, zeroing delta motion and teleporting back to the current position/rotation.This avoids blocking on chunk loads while keeping the player in a valid, ticking area.
64-69: Post‑teleport chunkSource.move hook is minimal and safe.Running
world.getChunkSource().move(player)afterteleportSetPosition(and only if the player is still present inworld.players()) is a reasonable way to ensure the chunk source is in sync after server‑side teleports.src/main/java/one/oktw/galaxy/mixin/tweak/MixinAsyncChunk_IOWorker.java (4)
76-80: Constructor injection cleanly swaps executor and pendingWrites implementation.Reassigning
pendingWritesto aConcurrentSkipListMapandconsecutiveExecutortoKotlinCoroutineTaskExecutorin<init>’sRETURNhook is safe: the base constructor has finished, no writes can have been queued yet, and the new data structures are concurrency‑friendly.
105-108: tellStorePending now directly delegates to storePendingChunk, which matches the new batching strategy.Overwriting
tellStorePending()to just callstorePendingChunk()fits the custom executor model and avoids queuing an extra indirection task; thewriteLockstill prevents redundant batch scheduling.
110-125: fastRead/fastScan correctly short‑circuit to in‑memory pendingWrites.Both injections:
- Look up
IOWorker.PendingStorefrom the sharedpendingWritesmap.- For reads, return a completed future with
result.copyData()wrapped inOptional.- For scans, feed
result.datato the providedStreamTagVisitorand return a completed future.This gives you a consistent view of yet‑to‑be‑flushed chunks and avoids unnecessary disk I/O, while preserving the asynchronous API contract.
127-132: runStore head injection enforces single‑write semantics per chunk.
pendingWrites.remove(pos, result)at the head ofrunStoreensures:
- Only the current
PendingStoreinstance for thatChunkPosis written.- Any stale or duplicate
runStorecalls are cancelled cheaply.This pairs well with the batched scheduling and avoids double‑writes when newer data replaces older entries.
src/main/java/one/oktw/galaxy/mixin/event/MixinPlayerInput_ServerGamePacketListener.java (1)
21-51: Input handling migration to ServerGamePacketListenerImpl preserves event semantics.Hooking
handlePlayerInputat thesetLastClientInputcall and comparingpacket.input()againstplayer.getLastClientInput()correctly detects changes in sneak/jump state; the emitted events mirror the previous behavior under the new Mojang packet and API types.src/main/resources/sponge.realtime.mixin.json (1)
11-24: Realtime mixin manifest renames look consistent; just ensure classes exist for all entries.The updated mixin list (e.g.,
entity.ExperienceOrbMixin_RealTime,entity.mob.ZombieVillagerMixin_RealTime,server.network.ServerGamePacketListenerMixin_RealTime,world.ServerLevelMixin_RealTime) matches the new naming scheme; as long as the corresponding classes are present underorg.spongepowered.common.mixin.realtime, the manifest should load correctly.src/main/resources/tweak.mixin.json (1)
6-25: All mixin class files are present and correctly updated to use Mojang mappings.Verification confirms that all 20 mixin class files referenced in
tweak.mixin.jsonexist insrc/main/java/one/oktw/galaxy/mixin/tweak/, and spot-checking reveals that the mixin implementations correctly reference new Mojang-mapped class names (e.g.,MapItem,ServerGamePacketListenerImpl) with no remaining old Yarn mapping references. The configuration changes are properly aligned with the Java implementations.
...in/java/org/spongepowered/common/mixin/realtime/entity/passive/AgeableMobMixin_RealTime.java
Show resolved
Hide resolved
src/main/java/org/spongepowered/common/mixin/realtime/entity/player/PlayerMixin_RealTime.java
Show resolved
Hide resolved
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant undertaking, migrating the project to Mojang mappings and updating numerous dependencies. The changes are extensive and touch almost every part of the codebase, from build scripts to game logic and data generation. The migration appears to be thorough, correctly adapting to new class names, method signatures, and API patterns across Minecraft, Fabric, and other libraries. The updates to data generation, mixins, and event handlers are all consistent with the new mappings. I've identified a couple of potential null pointer exceptions in ModelCustomBlockEntity.kt due to unsafe non-null assertions on a nullable property, which should be addressed. Overall, this is a great step forward for the project's maintenance and compatibility.
| field = direction | ||
| (world as? ServerWorld)?.getEntity(entityUUID)?.yaw = direction.positiveHorizontalDegrees | ||
| markDirty() | ||
| (level as? ServerLevel)?.getEntity(entityUUID!!)?.yRot = direction.toYRot() |
There was a problem hiding this comment.
The non-null assertion !! on the nullable property entityUUID could lead to a NullPointerException if the facing property is set before entityUUID has been initialized. This can happen if the model entity hasn't spawned yet. It's safer to use a safe call (?.let) to handle the nullable entityUUID.
| (world as ServerWorld).getEntity(entityUUID)?.kill(world as ServerWorld) | ||
| override fun setRemoved() { | ||
| super.setRemoved() | ||
| (level as ServerLevel).getEntity(entityUUID!!)?.kill(level as ServerLevel) |
There was a problem hiding this comment.
Similar to the previous comment, using a non-null assertion !! on the nullable entityUUID here is unsafe. If setRemoved() is called on a ModelCustomBlockEntity where the model entity has not yet been spawned (and thus entityUUID is null), this will cause a NullPointerException. Please use a null-safe approach, for example with a safe call (?.let), to prevent this potential crash.
jimchen5209
added
type: enhancement
Merge Queue Status✅ The pull request has been merged at 5b37c04 This pull request spent 5 seconds in the queue, with no time running CI. Required conditions to merge
|
Summary by CodeRabbit
Chores
Refactor