Repository navigation
fix: harden client cleanup, audio, and Pikafish lifecycle - #8
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| static void playUiSound(Minecraft minecraft, SoundEvent sound, float pitch, float volume) { | ||
| if (minecraft == null || sound == null) return; | ||
| int tick = minecraft.gui.getGuiTicks(); |
There was a problem hiding this comment.
WARNING: Potential NPE if minecraft.gui is null
minecraft.gui.getGuiTicks() is called without checking if gui is non-null. In early startup or edge cases, minecraft.gui could be null. Add a null check: if (minecraft.gui == null) return; before accessing getGuiTicks().
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (this.minecraft != null && this.minecraft.getSoundManager() != null) { | ||
| this.minecraft.getSoundManager().play(SimpleSoundInstance.forUI(SoundEvents.NOTE_BLOCK_BASS, 0.5f)); | ||
| } | ||
| GameSoundHelper.playUiSound(this.minecraft, SoundEvents.NOTE_BLOCK_BASS.value(), 0.5f, 0.25f); |
There was a problem hiding this comment.
WARNING: Inconsistent .value() usage on SoundEvents
SoundEvents.NOTE_BLOCK_BASS.value() is used here, but SoundEvents.EXPERIENCE_ORB_PICKUP (line 435) and SoundEvents.VILLAGER_NO (line 439) are used without .value(). In PianoTilesGameScreen, all SoundEvents use .value() (e.g., SoundEvents.NOTE_BLOCK_HARP.value()). This inconsistency suggests a potential compile error or runtime issue if SoundEvents fields are Holder<SoundEvent>.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if (this.minecraft != null && this.minecraft.getSoundManager() != null) { | ||
| this.minecraft.getSoundManager().play(SimpleSoundInstance.forUI(SoundEvents.EXPERIENCE_ORB_PICKUP, 1.0f + currentSequenceIndex * 0.1f)); | ||
| } | ||
| GameSoundHelper.playUiSound(this.minecraft, SoundEvents.EXPERIENCE_ORB_PICKUP, |
There was a problem hiding this comment.
WARNING: Missing .value() on SoundEvents
SoundEvents.EXPERIENCE_ORB_PICKUP and SoundEvents.VILLAGER_NO (lines 270, 274) are used without .value(), while PianoTilesGameScreen consistently uses .value() on all SoundEvents (e.g., SoundEvents.NOTE_BLOCK_HARP.value()). If SoundEvents fields are Holder<SoundEvent>, this will cause a type mismatch since GameSoundHelper.playUiSound expects SoundEvent.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| private void playSound(SoundEvent sound) { | ||
| if (minecraft != null && minecraft.level != null) { | ||
| minecraft.level.playLocalSound(0, 0, 0, sound, SoundSource.MASTER, 0.8f, 1.0f, false); | ||
| GameSoundHelper.playUiSound(minecraft, sound, 1.0f, 0.8f); |
There was a problem hiding this comment.
WARNING: GameSoundHelper called without gui null check
playSound checks minecraft != null && minecraft.level != null but GameSoundHelper.playUiSound accesses minecraft.gui.getGuiTicks() without verifying gui is non-null. If gui is null (early startup, headless), this will NPE. Consider adding && minecraft.gui != null to the condition.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| static void closeProcess(Process process, Closeable writer, Closeable reader) { | ||
| // 先终止进程解除读线程的阻塞，再关闭管道；反过来会在 BufferedReader.close() 卡住。 | ||
| if (process != null && process.isAlive()) { | ||
| java.util.List<ProcessHandle> descendants = process.descendants().toList(); |
There was a problem hiding this comment.
SUGGESTION: Redundant descendant destruction in closeProcess
Descendants are destroyed twice: first with destroy() (lines 259-261), then again with destroyForcibly() (lines 269-271). The second loop is redundant since destroyForcibly() on the parent process (line 267) should also terminate children. Consider removing the second descendant loop.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| */ | ||
| /** 客户端网络处理与断线清理，专服不会注册此订阅者。 */ | ||
| @OnlyIn(Dist.CLIENT) | ||
| @EventBusSubscriber(modid = ModMain.MODID, value = Dist.CLIENT) |
There was a problem hiding this comment.
WARNING: Changed from manual reflection to @EventBusSubscriber
Previously this class was explicitly NOT registered as an @EventBusSubscriber to avoid server-side class loading crashes, relying on ModNetworks reflection. Now it uses @EventBusSubscriber(modid = ModMain.MODID, value = Dist.CLIENT) with @OnlyIn(Dist.CLIENT). While this is the modern NeoForge pattern, verify that @OnlyIn stripping works correctly in your build pipeline to prevent the class from appearing in server JARs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
|
|
||
| final class GameSoundHelper { | ||
| private static final Set<ResourceLocation> PLAYED_THIS_TICK = new HashSet<>(); | ||
| private static int lastTick = Integer.MIN_VALUE; |
There was a problem hiding this comment.
SUGGESTION: lastTick initialization could be clearer
lastTick = Integer.MIN_VALUE works but is a magic value. Consider using a boolean firstTick = true flag or lastTick = -1 with a comment explaining it's a sentinel for "not yet initialized."
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Previous Issues ResolutionAll 7 issues from the previous review have been addressed:
Fix these issues in Kilo Cloud Previous Review Summary (commit 50d9d99)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 50d9d99)Status: 8 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (8 files)
Reviewed by free · Input: 212.7K · Output: 11.9K · Cached: 185.8K |
|
Review follow-up for commit
Verification after the follow-up: |
|
|
||
| final class GameSoundHelper { | ||
| private static final Set<ResourceLocation> PLAYED_THIS_TICK = new HashSet<>(); | ||
| private static int lastTick; |
There was a problem hiding this comment.
SUGGESTION: Fields lastTick and hasTick should be explicitly private
These static fields in a final class have package-private (default) access. Since they are only used internally within the class, they should be explicitly marked private for encapsulation.
| private static int lastTick; | |
| private static int lastTick; | |
| private static boolean hasTick; |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Summary
This PR hardens client lifecycle and external-engine handling in the upstream
game_consoleproject.Changes
Verification
./gradlew test build --offline --no-daemon --no-configuration-cache --console=plainuciokandreadyokboth terminate in about 15 seconds; normal handshake succeeds.The accelerated pressure test still reports 1,203 vanilla button-click sound-pool warnings because it injects thousands of button key events into one real client tick. A separate real-tick audio run has no pool warnings; the captured stack identifies the remaining accelerated warnings as vanilla
AbstractWidget.playDownSoundcalls.