[ARCHIVED, DEMO] New native webgl2 renderer#90
Conversation
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-hi3evau9d-zaro.vercel.app |
undo viewer changes before merge
|
/deploy |
|
/deploy |
1 similar comment
|
/deploy |
|
/deploy |
|
/deploy |
|
/deploy |
|
/deploy |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-n42x4eick-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-otarp7naq-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-4nyholn80-zaro.vercel.app |
…ChunksLoaded method correctly!!
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements a complete WebGL2 rendering backend for the Minecraft viewer, enabling GPU-accelerated instanced geometry rendering via Web Workers. The change splits chunk geometry pipeline into deferred block-side recording (texture index, tint, transparency) and worker-based instanced mesh generation, abstracts the renderer architecture into common/Three.js/WebGL implementations, and integrates canvas lifecycle management through ViewerWrapper. Supporting work includes shader files, animated texture atlas generation, playground example with simple controls, and E2E tests. ChangesWebGL Rendering Pipeline Implementation
🎯 4 (Complex) | ⏱️ ~60 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
@coderabbitai ignore |
|
/deploy |
|
/deploy |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cypress/e2e/index.spec.ts (1)
39-39:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winRemove
it.onlybefore merging.
it.onlyrestricts execution to just this test, so "Loads & renders singleplayer" and "Loads & renders zip world" will be silently skipped in CI. This looks like a debug leftover.🐛 Proposed fix
-it.only('Joins to server', () => { +it('Joins to server', () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypress/e2e/index.spec.ts` at line 39, Remove the accidental test-only filter by replacing the use of it.only in the test declaration (the line using it.only('Joins to server', ...)) with a normal it(...) so the other tests ("Loads & renders singleplayer" and "Loads & renders zip world") are not skipped; locate the test declaration in cypress/e2e/index.spec.ts and change the it.only usage back to it.prismarine-viewer/viewer/prepare/atlas.ts (1)
28-39:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
JsonAtlas['textures']to includetextureName
makeTextureAtlas()typestexturesIndexasJsonAtlas['textures'], but then assignstextureNamein each entry (texturesIndex[cleanName] = { ..., textureName: cleanName, ... }). SinceJsonAtlas['textures'][string]doesn’t declaretextureName, this is an excess-property type error.🛠️ Proposed type update
su?: number, sv?: number, + textureName?: string, animatedFrames?: number🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/prepare/atlas.ts` around lines 28 - 39, The JsonAtlas type's textures entries are missing the textureName property which makeTextureAtlas assigns into texturesIndex (texturesIndex[cleanName] = { ..., textureName: cleanName, ... }), causing an excess-property type error; update JsonAtlas so that the mapped value type (JsonAtlas['textures'][string]) includes textureName: string (optional or required as appropriate) and any other fields produced by makeTextureAtlas to match the actual shape used at runtime (refer to JsonAtlas, makeTextureAtlas, texturesIndex, and textureName when editing the type).
🟠 Major comments (25)
prismarine-viewer/examples/TouchControls2.tsx-18-18 (1)
18-18:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPerformance: avoid allocating new Vector3 every frame.
A new
THREE.Vector3is allocated every 33ms (30fps), creating unnecessary GC pressure. On mobile targets (per PR benchmarks on iPhone XS/12 Pro), this allocation overhead degrades performance.⚡ Proposed fix to reuse a single Vector3 instance
useEffect(() => { let vec3 = new Vec3(0, 0, 0) + const offset = new THREE.Vector3() const intervalId = setInterval(() => { - viewer.camera.position.add(new THREE.Vector3(vec3.x, vec3.y, vec3.z)) + offset.set(vec3.x, vec3.y, vec3.z) + viewer.camera.position.add(offset) }, 1000 / 30)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/examples/TouchControls2.tsx` at line 18, The code allocates a new THREE.Vector3 each frame in the expression viewer.camera.position.add(new THREE.Vector3(vec3.x, vec3.y, vec3.z)), causing GC pressure; instead create and reuse a single Vector3 instance (e.g., a module- or component-scoped temp Vector3) and set its x/y/z each frame then call viewer.camera.position.add(tempVec), updating or resetting the temp vector as needed; locate the allocation in TouchControls2 (the viewer.camera.position.add call) and replace it with reuse of that temp Vector3 to avoid per-frame allocations.prismarine-viewer/examples/TouchControls2.tsx-26-29 (1)
26-29:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDiagonal movement broken: vec3 reset on every coordinate update.
The
updateCoordcallback creates a newVec3(0, 0, 0)on line 27 before setting a single coordinate. This zeros out all other axes, so only the most recently updated axis will have a non-zero value. Diagonal movement (simultaneous x and z) will be impossible—only the last axis updated will move the camera.🐛 Proposed fix to update coordinate in place
updateCoord ([coord, state]) { - vec3 = new Vec3(0, 0, 0) vec3[coord] = state }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/examples/TouchControls2.tsx` around lines 26 - 29, The updateCoord implementation resets the movement vector each call by recreating vec3 (Vec3) so other axes get zeroed; instead, update the existing movement Vec3 in place—remove the new Vec3(0,0,0) allocation inside updateCoord and assign state to the specified component on the persistent vec3 (the outer-scope Vec3 instance used for movement), e.g., vec3[coord] = state on the existing object; ensure the persistent vec3 is declared once outside updateCoord (not reinitialized inside the function) so simultaneous axis updates (diagonal movement) accumulate correctly.buildWorkers.mjs-37-44 (1)
37-44:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCreate output dirs before writing to avoid ENOENT. The plugin writes into
distandprismarine-viewer/publicwithout ensuring they exist. Thebuildnpm script happens to createdistearlier, but standalone runs (e.g.watch-other-workersinrun-playground) can execute this beforedistexists, throwingENOENT. The siblingprismarine-viewer/buildWorker.mjsguards this withfs.mkdirSync(outDir, { recursive: true }).🛡️ Proposed fix
build.onEnd(({ outputFiles }) => { for (const file of outputFiles) { for (const dir of ['prismarine-viewer/public', 'dist']) { const baseName = path.basename(file.path) + fs.mkdirSync(dir, { recursive: true }) fs.writeFileSync(path.join(dir, baseName), file.contents) } } })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@buildWorkers.mjs` around lines 37 - 44, The build.onEnd handler writes outputFiles into 'prismarine-viewer/public' and 'dist' without ensuring those directories exist; update the build.onEnd callback (where build.onEnd is defined) to create each target directory before writing by calling fs.mkdirSync(dir, { recursive: true }) for each dir in the ['prismarine-viewer/public', 'dist'] loop, then proceed to compute baseName and call fs.writeFileSync as before so writes no longer throw ENOENT.package.json-11-11 (1)
11-11:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix Cypress perf spec path in
test:cypress:perfpackage.jsonpoints tocypress/e2e/perf.spec.ts, but the repo containscypress/e2e/performance.spec.tsand there is noperf.spec.ts—so the Cypress run will fail (“no specs found”). Also,--browser edgerequires Edge to be available in the runner/environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@package.json` at line 11, The npm script "test:cypress:perf" in package.json points to a non-existent spec; update the script value for "test:cypress:perf" to use the correct spec path "cypress/e2e/performance.spec.ts" and also remove or make the browser flag configurable (replace or omit "--browser edge") so the runner doesn't fail if Edge isn't available—locate the "test:cypress:perf" entry in package.json and change the spec path and/or browser flag accordingly.prismarine-viewer/viewer/lib/viewer.ts-201-213 (1)
201-213:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
render()is now a no-op.This stops the Three scene from ever being drawn, so
Entities,Primitives, and the FXAA path stop rendering for any caller that still drivesViewerthroughupdate()/render().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/viewer.ts` around lines 201 - 213, The render() method was left as a no-op which prevents the Three scene and post-processing from drawing; restore the original rendering logic in render() so the viewer actually draws each frame: inside Viewer.render() re-enable the composer branch (set this.renderPass.camera = this.camera and call this.composer.render() when this.composer exists) and otherwise call this.renderer.render(this.scene, this.camera), and then call this.entities.render() (and re-enable any FXAA/composer usage). Ensure update() still calls tweenJs.update() if needed so animations run.prismarine-viewer/viewer/lib/worldrendererCommon.ts-121-127 (1)
121-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
finishedChunkson reset.
resetWorld()keeps the previousfinishedChunksmap alive. After a version/world reset,allChunksLoaded()and the loaded-chunk counters can trip against stale completions from the previous world.Suggested fix
resetWorld () { this.active = false this.loadedChunks = {} + this.finishedChunks = {} this.sectionsOutstanding = new Map() for (const worker of this.workers) { worker.postMessage({ type: 'reset' }) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/worldrendererCommon.ts` around lines 121 - 127, resetWorld() currently leaves this.finishedChunks populated causing stale completion state; inside resetWorld (alongside this.active=false, this.loadedChunks={}, this.sectionsOutstanding=...) clear or reinitialize the finishedChunks collection (e.g., this.finishedChunks.clear() or this.finishedChunks = new Map()) so allChunksLoaded() and loaded-chunk counters start fresh after a reset; update the resetWorld method to perform this clear before posting reset to workers.prismarine-viewer/viewer/lib/worldrendererWebgl.ts-48-59 (1)
48-59:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDrop geometry for chunks that were already unloaded.
The loaded-chunk guard is commented out here, so a late worker reply can re-add a section after
removeColumn()already removed it. That leaves ghost chunks visible until some later reset.Suggested fix
handleWorkerMessage (data: any): void { if (data.type === 'geometry' && Object.keys(data.geometry.blocks).length) { const chunkCoords = data.key.split(',').map(Number) as [number, number, number] - if (/* !this.loadedChunks[chunkCoords[0] + ',' + chunkCoords[2]] || */ !this.active) return + const chunkKey = `${chunkCoords[0]},${chunkCoords[2]}` + if (!this.loadedChunks[chunkKey] || !this.active) return addBlocksSection(data.key, data.geometry) this.lastChunkDistance = Math.max(...this.getDistance(new Vec3(chunkCoords[0], 0, chunkCoords[2])))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/worldrendererWebgl.ts` around lines 48 - 59, Re-enable the loaded-chunk guard in handleWorkerMessage so late worker replies for already-unloaded columns are ignored: before calling addBlocksSection or updating lastChunkDistance/newChunks, check that this.active is true and that this.loadedChunks[chunkCoords[0] + ',' + chunkCoords[2]] exists (the same string-key used by removeColumn), and return early if not; only then call addBlocksSection, update lastChunkDistance (using getDistance and Vec3) and set this.newChunks[data.key].prismarine-viewer/viewer/lib/worldrendererThree.ts-29-37 (1)
29-37:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
updatePosDataChunk()until the section exists.
loadedChunksis marked before the worker sends geometry, soupdateViewerPosition()can call this path whilethis.sectionObjects[key]is still undefined. That makes normal camera movement during chunk meshing crash here.Suggested fix
updatePosDataChunk (key: string) { if (!this.viewerPosition) return + const object = this.sectionObjects[key] + if (!object) return const [x, y, z] = key.split(',').map(x => Math.floor(+x / 16)) const [xPlayer, yPlayer, zPlayer] = this.viewerPosition.toArray().map(x => Math.floor(x / 16)) // sum of distances: x + y + z const chunkDistance = Math.abs(x - xPlayer) + Math.abs(y - yPlayer) + Math.abs(z - zPlayer) - const section = this.sectionObjects[key].children.find(child => child.name === 'mesh')! + const section = object.children.find(child => child.name === 'mesh') + if (!section) return section.renderOrder = 500 - chunkDistance }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/worldrendererThree.ts` around lines 29 - 37, The updatePosDataChunk method assumes this.sectionObjects[key] and its children exist, which can be false when updateViewerPosition triggers while chunks are still being meshed; guard the method by returning early if this.sectionObjects[key] is falsy or if no child named 'mesh' is found, e.g., check existence of this.sectionObjects[key] and that children.find(child => child.name === 'mesh') returns a node before computing chunkDistance and setting section.renderOrder in updatePosDataChunk to avoid the crash.prismarine-viewer/viewer/lib/worldrendererWebgl.ts-62-66 (1)
62-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
fullResetinto the normal reset path.
chunksReset()is only a helper right now.Viewer.resetAll()andsetVersion()callresetWorld(), which resets workers but never tells the native WebGL renderer to drop its existing sections.Suggested fix
export class WorldRendererWebgl extends WorldRendererCommon { @@ chunksReset () { sendWorkerMessage({ type: 'fullReset' }) } + + resetWorld () { + super.resetWorld() + this.newChunks = {} + this.chunksReset() + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/worldrendererWebgl.ts` around lines 62 - 66, resetWorld() (invoked by Viewer.resetAll() and setVersion()) currently resets workers but does not tell the WebGL renderer to drop its existing sections; wire the existing chunksReset() helper into the normal reset path by invoking chunksReset() (or sending the same sendWorkerMessage({type: 'fullReset'})) from resetWorld() so the native renderer receives the 'fullReset' message and drops its sections; update resetWorld() to call chunksReset() before/after worker reset as appropriate to ensure both workers and the WebGL renderer are cleared.prismarine-viewer/viewer/lib/worldrendererThree.ts-169-175 (1)
169-175:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReset should dispose and clear renderer-owned state.
This removes meshes from the scene, but it leaves
sectionObjectsandchunkTexturespopulated and never disposes the cached sign textures. After reconnect/version switch, stale sections remain addressable and GPU resources leak.Suggested fix
resetWorld () { super.resetWorld() for (const mesh of Object.values(this.sectionObjects)) { this.scene.remove(mesh) + dispose3(mesh) } + this.sectionObjects = {} + for (const textures of this.chunkTextures.values()) { + for (const texture of Object.values(textures)) texture.dispose() + } + this.chunkTextures.clear() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/worldrendererThree.ts` around lines 169 - 175, resetWorld currently removes meshes from the scene but leaves renderer-owned state and GPU resources (sectionObjects, chunkTextures and cached sign textures) intact; modify resetWorld to iterate sectionObjects and for each mesh dispose its geometry and material (and material.map if present) before removing it, iterate chunkTextures and dispose each texture, dispose any cached sign textures (e.g. cachedSignTextures), then clear or reinitialize sectionObjects and chunkTextures (and the sign texture cache) so stale sections are not addressable and GPU memory is freed.prismarine-viewer/viewer/lib/worldDataEmitter.ts-39-42 (1)
39-42:⚠️ Potential issue | 🟠 Major | ⚡ Quick winChanging view distance must reconcile currently loaded chunks.
This only updates the number and emits
renderDistance; it never unloads chunks that moved out of range or loads the newly visible ring. The state stays at the old radius until the player moves.Suggested fix
- updateViewDistance (viewDistance: number) { + async updateViewDistance (viewDistance: number) { this.viewDistance = viewDistance this.emitter.emit('renderDistance', viewDistance) + await this.updatePosition(this.lastPos, true) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/worldDataEmitter.ts` around lines 39 - 42, updateViewDistance currently only sets this.viewDistance and emits 'renderDistance' but does not reconcile already-loaded chunks; update it to compute the new visible radius and then unload chunks that are now outside that radius and load the newly visible ring so state matches the new distance. Use the emitter/data structures already on the class (e.g. this.viewDistance, this.emitter, this.loadedChunks or whatever map/Set holds loaded chunks) and for each chunk decide: if now out of range, call the existing unload path (emit('unloadChunk', coord) or call the class's unloadChunk/unloadChunkAt method), and if newly in range, call the existing load path (emit('loadChunk', coord) or call loadChunk/loadChunkAt); keep emitting 'renderDistance' but ensure the load/unload work happens immediately inside updateViewDistance to reconcile state.prismarine-viewer/viewer/lib/world.ts-20-24 (1)
20-24:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
isCube()now treats every block as a full cube.That makes AO/occlusion logic misclassify slabs, stairs, panes, plants, and other partial shapes as solid cubes, so face culling and shading become wrong across the renderer pipeline.
Suggested fix
function isCube (shapes) { - return true if (!shapes || shapes.length !== 1) return false const shape = shapes[0] return shape[0] === 0 && shape[1] === 0 && shape[2] === 0 && shape[3] === 1 && shape[4] === 1 && shape[5] === 1 }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/world.ts` around lines 20 - 24, The isCube function currently always returns true, causing all blocks to be treated as full cubes; remove the stray "return true" and restore proper shape checks in isCube(shapes) so it only returns true when shapes exists, has length 1, and the single shape's coordinates match a full cube (e.g., shape[0]==0, shape[1]==0, shape[2]==0, shape[3]==1, shape[4]==1, shape[5]==1); otherwise return false. Ensure you reference and update the isCube function (and its shapes parameter handling) so partial shapes like slabs/stairs/panes correctly return false.prismarine-viewer/viewer/lib/worker.js-32-35 (1)
32-35:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't drop pending dirty counts when clearing a section.
value === falsedeletes the map entry and emits only onesectionFinished, even if that section was already queued multiple times.sectionsOutstandingon the main thread can then stay non-zero forever, which stallswaitForChunksToRender()andallChunksLoaded().Suggested fix
if (!value) { + const pending = dirtySections.get(key) || 0 dirtySections.delete(key) - postMessage({ type: 'sectionFinished', key }) + for (let i = 0; i < pending + 1; i++) { + postMessage({ type: 'sectionFinished', key }) + } return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/worker.js` around lines 32 - 35, The current code deletes a section from dirtySections and emits a single postMessage({ type: 'sectionFinished', key }) when value === false, which drops any pending counts and leaves sectionsOutstanding on the main thread incorrect; fix by preserving and sending the pending count when clearing a section: before calling dirtySections.delete(key) read the pending count (e.g. const pending = dirtySections.get(key) || 1), then either emit sectionFinished that many times or include the pending count in the message (postMessage({ type: 'sectionFinished', key, count: pending })), then delete the entry; ensure the main thread logic that updates sectionsOutstanding (used by waitForChunksToRender() and allChunksLoaded()) consumes the count field if you choose that approach.prismarine-viewer/examples/TextureAnimation.ts-23-38 (1)
23-38:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
frameImagesis hardcoded to1, ignoring theframesImagesconstructor argument.Line 29 sets
this.frameImages = 1instead of deriving it from theframesImagesparameter. As a result, whendata.framesis absent,framesToSwitchis always1(line 34), soframeIndex %= this.framesToSwitchkeeps the index pinned at0and the animation never advances. TheframesImagesconstructor arg is then unused. Confirm whether this should bethis.frameImages = framesImages.🐛 Proposed fix
- this.frameImages = 1; + this.frameImages = framesImages;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/examples/TextureAnimation.ts` around lines 23 - 38, The constructor currently ignores the framesImages parameter by hardcoding this.frameImages = 1; change this to assign the passed-in framesImages (this.frameImages = framesImages) so the instance uses the provided value; keep the existing logic that overrides framesToSwitch with data.frames.length when this.data.frames is present (symbols: constructor, framesImages parameter, this.frameImages, this.data.frames, this.framesToSwitch, frameIndex).cypress/e2e/performance.spec.ts-19-23 (1)
19-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
durationlogging and use the intendedworldLoadstart mark incypress/e2e/performance.spec.ts
const duration = cy.window()...its('duration')is a Cypress chainable;cy.log('Duration', duration)logs the chainable, not the numeric duration—resolve it via.then(duration => ...).- The
'worldLoad'mark is set butinvoke('measure', 'modalOpen')omitsstartMark, so the measurement starts from the performance timeline origin instead of'worldLoad'.🐛 Proposed fix
}).then(() => { - const duration = cy.window() - .its('performance') - .invoke('measure', 'modalOpen') - .its('duration') - cy.log('Duration', duration) + cy.window() + .its('performance') + .invoke('measure', 'modalOpen', 'worldLoad') + .its('duration') + .then(duration => { + cy.log('Duration', duration) + }) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cypress/e2e/performance.spec.ts` around lines 19 - 23, The current test stores a Cypress chainable in duration and logs the chainable instead of the numeric value and also calls performance.measure('modalOpen') without the intended start mark; update the chain to call performance.measure with the start mark 'worldLoad' (invoke('measure', 'modalOpen', 'worldLoad')) and then resolve the resulting entries to read the numeric duration (e.g., invoke/getEntriesByName and then .its('0.duration') or use .then(duration => ...) before calling cy.log) so cy.log receives a number rather than a chainable; locate the chain beginning at cy.window().its('performance').invoke('measure', 'modalOpen') and adjust as described.prismarine-viewer/examples/webglRendererWorker.ts-589-592 (1)
589-592:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
exportDatacrashes on sparseallSides.
allSidesholdsundefinedholes (set inremoveBlocksSectionLine 516 andresetHalfSceneLine 292). Destructuringconst [x, y, z, side] = allSides[i]throwsCannot destructure property '0' of undefinedonce any section has been removed before export.🛡️ Proposed fix
for (let i = 0; i < allSides.length; i++) { - const [x, y, z, side] = allSides[i]; + const entry = allSides[i]; + if (!entry) continue; + const [x, y, z, side] = entry; flatData.set([x, y, z, side.face, side.textureIndex], i * 5); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/examples/webglRendererWorker.ts` around lines 589 - 592, The loop in exportData assumes every entry in allSides is defined and destructures allSides[i] directly, which throws when allSides contains holes; fix by reading the row into a temporary (e.g., const row = allSides[i]), skip it if falsy, then destructure row when present and write into flatData; because skipping holes changes the output packing, use a separate writeIndex (incremented only when you write) for the flatData.set offset instead of using i. Reference symbols: allSides, exportData loop, flatData.set.prismarine-viewer/examples/webglRenderer.ts-161-179 (1)
161-179:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
exportFixturelistener isn't filtered by message type.The worker posts
{ type: 'fps' }every second (webglRendererWorker.tsLine 36). This handler reacts to the first message of any type, so it will frequently fire on anfpsmessage wheree.data.dataisundefined, then throw atreceivedData.sides.buffer(Line 179) and abort before the realexportDatareply arrives. Guard on the type.🐛 Proposed fix
worker.addEventListener('message', async (e) => { + if (e.data.type !== 'exportData') return const receivedData = e.data.data;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/examples/webglRenderer.ts` around lines 161 - 179, The message handler registered via worker.addEventListener('message', async (e) => { ... }) assumes e.data.data exists and immediately reads receivedData.sides.buffer; add a guard that checks e.data.type (e.g., only proceed when e.data.type === 'exportData' or the specific exportFixture/exportData type your worker sends) before accessing e.data.data/receivedData, so the handler ignores periodic {type:'fps'} messages and only runs the export logic (the section that creates objectURL via URL.createObjectURL and uses receivedData).prismarine-viewer/examples/webglRenderer.ts-128-132 (1)
128-132:⚠️ Potential issue | 🟠 Major | ⚡ Quick winResize height key mismatch — height never updates.
The worker reads
e.data.newHeight(webglRendererWorker.tsLine 470), but this message sends the key asheight. As a resultnewHeightstaysundefinedin the worker and the canvas height is never resized, distorting the projection on window resize.🐛 Proposed fix
sendWorkerMessage({ type: 'resize', newWidth: window.innerWidth * window.devicePixelRatio, - height: window.innerHeight * window.devicePixelRatio + newHeight: window.innerHeight * window.devicePixelRatio })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/examples/webglRenderer.ts` around lines 128 - 132, The resize message uses the wrong key ("height") so the worker's resize handler (reads e.data.newHeight in webglRendererWorker) never gets the new height; update the callsite that invokes sendWorkerMessage (the resize block in webglRenderer.ts) to send newHeight: window.innerHeight * window.devicePixelRatio instead of height so the worker receives e.data.newHeight and can correctly resize the canvas.prismarine-viewer/examples/webglRendererWorker.ts-113-118 (1)
113-118:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMulti-gigabyte upfront allocation will OOM on mobile.
With
NumberOfCube = 16_000_000,sidePositionsandsideBiomeColoreach allocate16e6 * 18 * 4 ≈ 1.15 GB, plus ~384 MB each for the index/texture arrays — several GB reserved before a single frame renders. Given the PR explicitly targets iOS/iPhone, this is very likely to crash. A dynamic-growth path already exists (Line 252-258), so start small and let it grow.🧮 Proposed fix
- let NumberOfCube = isPlayground ? 10_000 : 16_000_000 + let NumberOfCube = isPlayground ? 10_000 : 1_000_000🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/examples/webglRendererWorker.ts` around lines 113 - 118, The code preallocates huge buffers using NumberOfCube (16_000_000) causing multi-GB allocations for sidePositions, sideBiomeColor, sideTextureIndices and sideIndexes; change allocation to start with a small initial capacity (e.g. a few thousand or based on isPlayground) and rely on the existing dynamic-growth logic used later (the growth path around the growth/resize functions used at lines handling buffer expansion) to expand buffers as needed; update the creation of sidePositions, sideBiomeColor, sideTextureIndices and sideIndexes to use that initial capacity and ensure the growth/resize helpers are invoked when pushing beyond capacity so mobile/iOS devices don’t OOM.prismarine-viewer/viewer/lib/viewerWrapper.ts-50-65 (1)
50-65:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop the RAF loop and unregister window listeners in
dispose().
dispose()removes the canvas, but the RAF chain scheduled on Line 76 and thefocus/blurlisteners survive. Reconnecting will stack extra render loops and keep old wrapper instances alive.Suggested fix
export class ViewerWrapper { + private rafId: number | null = null + private onFocus = () => { + this.windowFocused = true + } + private onBlur = () => { + this.windowFocused = false + } trackWindowFocus () { - window.addEventListener('focus', () => { - this.windowFocused = true - }) - window.addEventListener('blur', () => { - this.windowFocused = false - }) + window.addEventListener('focus', this.onFocus) + window.addEventListener('blur', this.onBlur) } dispose () { if (!this.addedToPage) throw new Error('Not added to page') document.body.removeChild(this.canvas) this.renderer?.dispose() - // this.addedToPage = false + if (this.rafId !== null) this.globalObject.cancelAnimationFrame(this.rafId) + window.removeEventListener('focus', this.onFocus) + window.removeEventListener('blur', this.onBlur) clearInterval(this.fpsInterval) } - render (time: DOMHighResTimeStamp) { + render = (time: DOMHighResTimeStamp) => { if (this.globalObject.stopLoop) return for (const fn of beforeRenderFrame) fn() - this.globalObject.requestAnimationFrame(this.render.bind(this)) + this.rafId = this.globalObject.requestAnimationFrame(this.render) if (this.globalObject.stopRender || this.renderer?.xr.isPresenting || (this.stopRenderOnBlur && !this.windowFocused)) return ... } }Also applies to: 73-76
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/viewerWrapper.ts` around lines 50 - 65, The dispose() method currently only removes the canvas and clears fpsInterval but leaves the requestAnimationFrame loop and the focus/blur listeners active; update the wrapper to store the RAF id (e.g., this.rafId returned by requestAnimationFrame in the animation method) and the bound listener functions used by trackWindowFocus (e.g., this.onWindowFocus and this.onWindowBlur), then in dispose() call cancelAnimationFrame(this.rafId), removeEventListener('focus', this.onWindowFocus) and removeEventListener('blur', this.onWindowBlur), set this.addedToPage = false and null out/cleanup this.canvas and any other retained references so the instance can be garbage-collected (refer to trackWindowFocus, dispose, and the animation/RAF call sites).src/index.ts-386-387 (1)
386-387:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't overwrite the selected singleplayer version.
Line 386 forces every local server session to
1.14.4, so the version assembled fromoptions.localServerOptions,serverOverrides, andbotVersionis ignored.Suggested fix
- serverOptions.version = '1.14.4' + serverOptions.version ??= '1.14.4'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/index.ts` around lines 386 - 387, The code forcibly sets serverOptions.version = '1.14.4', which overwrites the version assembled from options.localServerOptions, serverOverrides, and connectOptions.botVersion; remove that hardcoded assignment (or change it to only set a default when serverOptions.version is undefined/null) so the existing resolution logic is preserved, and keep the downloadVersion calculation using connectOptions.botVersion || (singleplayer ? serverOptions.version : undefined) unchanged so it picks the already-determined serverOptions.version.prismarine-viewer/viewer/lib/models.ts-394-424 (1)
394-424:⚠️ Potential issue | 🟠 Major | ⚡ Quick winModule-level
textureNameis never reset, so stale values leak into side data.
getResultonly assignstextureNamewhenresult.textureNameis truthy (Line 420). When a face has notextureName, the module-level variable retains the value from a previously processed block/face, and that stale value is what gets recorded at Line 542. Reset it per call. Note also that the module-leveltintset ingetResult(Line 395/407-416) is dead — the side record at Line 543 uses the localtintcomputed inrenderElementNew.🐛 Proposed fix to reset per-call state
const getResult = (biome, block, side: string): number => { + textureName = undefined const facesOrTexture = findTextureInBlockStates(block.name);Also applies to: 538-544
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/models.ts` around lines 394 - 424, The module-level variable textureName is never reset inside getResult, causing stale texture names to leak between faces; update getResult to initialize and return a per-call textureName (or reset the module-level textureName at the start of getResult) so each invocation sets textureName only for that face, and remove or stop relying on the module-level tint set inside getResult (it’s dead), instead compute and return tint from getResult (or use the local tint in renderElementNew) so renderElementNew consumes the per-call textureName and tint returned by getResult rather than stale module-level values; look for the functions/variables named getResult, textureName, tint, and renderElementNew to implement this change.prismarine-viewer/viewer/prepare/atlas.ts-80-94 (1)
80-94:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAtlas size isn't reserved for animation frames, risking out-of-bounds placement.
texSize/imgSizeare computed fromtilesCount(defaults toinput.length, Lines 44/52), but animated textures consumeframestiles each and advanceoffsetbyframes - 1(Line 84). When animated frames push the runningpospasttexSize * texSize, the computedx/yexceedimgSize, so frames are drawn off-canvas and their texture indices become invalid. Consider accounting for total frame count when sizing the canvas.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/prepare/atlas.ts` around lines 80 - 94, The atlas allocation must account for animation frames so canvases don't overflow: compute a totalTiles count that includes extra frame slots (e.g., totalTiles = tilesCount + sum(frames - 1) for animated entries) and use that totalTiles when computing texSize and imgSize instead of using tilesCount or input.length; update the code that sets texSize/imgSize (where texSize/imgSize are calculated from tilesCount/input.length) to use totalTiles, keep the existing offset and pos/frame drawing logic (offset, pos, frames) but ensure offset increments still reflect consumed slots so x/y calculations cannot go out of bounds.prismarine-viewer/viewer/prepare/genItemsAtlas.ts-15-23 (1)
15-23:⚠️ Potential issue | 🟠 MajorFix latest MC assets version filter:
minecraft-datareturnsnull(doesn’t throw) for unsupported versions
minecraft-data’s Node loader returnsnullfor unsupported version strings rather than throwing, so this filter:const latestMcAssetsVersion = McAssets.versions.filter((v) => { try { minecraftDataLoader(v) return true } catch { return false } }).at(-1)!will treat unsupported versions as “supported” and effectively pick the last entry in
McAssets.versions(so the “latest supported” logic can be wrong). Also,.at(-1)assumesMcAssets.versionsis already sorted newest → oldest.Update the filter to explicitly reject
null/undefinedfromminecraftDataLoader(v)(and/or sortMcAssets.versionsbefore using.at(-1)).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/prepare/genItemsAtlas.ts` around lines 15 - 23, The current latestMcAssetsVersion computation treats loader failures as thrown exceptions, but minecraftDataLoader(v) returns null for unsupported versions; update the predicate in the filter that builds latestMcAssetsVersion to call minecraftDataLoader(v) and explicitly check for a non-null/undefined result (e.g., const data = minecraftDataLoader(v); return data != null) instead of relying on try/catch, and ensure McAssets.versions is sorted newest→oldest (or use .sort(...) before .at(-1)) so .at(-1) yields the newest supported assets; adjust the code around latestMcAssetsVersion, McAssets.versions, and minecraftDataLoader to implement these checks and sorting.prismarine-viewer/viewer/lib/models.ts-654-669 (1)
654-669:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftStop mutating shared water/lava model elements during rendering
In
prismarine-viewer/viewer/lib/models.ts(lines 654-669),getSectionGeometrydoes:if (block.name === 'water' || block.name === 'lava') { const textureParticle = block.variant![0].model.textures.particle const tex = { texture: textureParticle, cullface: true, tintindex: 0 }; block.variant![0].model.elements.push({ from: [0, 0, 0], to: [16, 16, 16], faces: { down: tex, up: tex, north: tex, south: tex, west: tex, east: tex } }) }But
block.variantis taken directly fromblockStatesbygetModelVariants(no cloning), so thispush()mutates the shared resolved model object for that blockstate. Since this code runs on every section render,model.elementswill keep growing across re-renders (duplicate entries), causing unbounded memory/CPU growth; additionally, later rendering uses onlyelements[0], so the extra appended entries may not even affect what gets drawn.Build a local element/geometry for this special-case instead of mutating
variant.model.elementsin-place, or guard so the append happens at most once per resolved model.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/models.ts` around lines 654 - 669, getSectionGeometry is mutating shared resolved models by pushing a water/lava element into block.variant![0].model.elements on each render, causing unbounded growth; fix by avoiding in-place mutation: either construct a local elements array for rendering (e.g., const elements = [...block.variant![0].model.elements, waterElement] and use that) or add a one-time guard on the resolved model (e.g., mark model._waterElementAdded) so you only append once. Update getSectionGeometry to use the local/cloned elements for geometry generation instead of mutating block.variant![0].model.elements.
🟡 Minor comments (11)
scripts/githubActions.mjs-9-9 (1)
9-9:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTrim tokens after splitting by
=to handle spaces around delimiter.The current implementation trims each line but doesn't trim the individual tokens after splitting by
=. IfALIASEScontains entries like"123 = my-alias", the resulting array would be['123 ', ' alias']with leading/trailing whitespace, which would fail to matchprNumberon line 13.🔧 Proposed fix
- const aliases = aliasesRaw.split('\n').map((x) => x.trim().split('=')) + const aliases = aliasesRaw.split('\n').map((x) => x.trim().split('=').map(t => t.trim()))🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/githubActions.mjs` at line 9, The split on '=' leaves surrounding whitespace on tokens so entries like "123 = my-alias" won't match prNumber; update the aliases parsing where aliases is created from aliasesRaw (the line creating aliases from aliasesRaw) to trim each token after splitting (i.e., split by '\n', trim the line, split by '=', then trim both key and value tokens) so comparisons against prNumber use trimmed values.src/react/CreateWorldProvider.tsx-55-55 (1)
55-55:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGame mode mapping will break if additional modes are enabled.
The ternary
gameMode === 'survival' ? 0 : 1correctly maps the current two game modes (survival→0, creative→1), but ifadventureorspectatorare uncommented ingameModes, they will incorrectly map to 1 instead of 2 and 3 respectively.🛡️ Proposed fix using explicit mapping
- gameMode: gameMode === 'survival' ? 0 : 1, + gameMode: { survival: 0, creative: 1, adventure: 2, spectator: 3 }[gameMode] ?? 0,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/react/CreateWorldProvider.tsx` at line 55, The current mapping in CreateWorldProvider.tsx uses a ternary gameMode === 'survival' ? 0 : 1 which only works for two modes; change it to an explicit mapping (e.g., a map/object or switch) that converts each gameMode string to its numeric code (e.g., 'survival'→0, 'creative'→1, 'adventure'→2, 'spectator'→3) and use that mapping where gameMode is set, so new modes in the gameModes list map correctly; update any usage of the gameMode value to read from this mapping instead of the ternary.src/optionsStorage.ts-64-66 (1)
64-66:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winInconsistent default game mode between UI and storage.
localServerOptions.gameModedefaults to1(creative), butcreatingWorldState.gameModeinCreateWorld.tsxdefaults togameModes[0]which is'survival'. This inconsistency could cause confusion or unexpected behavior iflocalServerOptions.gameModeis used elsewhere in the codebase.♻️ Proposed fix to align defaults
localServerOptions: { - gameMode: 1 + gameMode: 0 } as any,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/optionsStorage.ts` around lines 64 - 66, The default game mode is inconsistent: update the default in optionsStorage.ts so localServerOptions.gameMode matches CreateWorld.tsx's creatingWorldState.gameMode (which uses gameModes[0] => 'survival'); locate the localServerOptions object and change its gameMode default from 1 to the same index used by CreateWorld (or alternatively change CreateWorld to use gameModes[1] if you intend 'creative') so both places (localServerOptions.gameMode and creatingWorldState.gameMode in CreateWorld.tsx) use the same index/value.src/globals.d.ts-75-82 (1)
75-82:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMisleading variable name in shader module declarations.
The exported variable is named
pngin both shader module declarations, which appears to be copy-pasted from the*.pngmodule declaration. Consider renaming toshaderor a similar name for clarity.📝 Proposed fix
declare module '*.frag' { - const png: string - export default png + const shader: string + export default shader } declare module '*.vert' { - const png: string - export default png + const shader: string + export default shader }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/globals.d.ts` around lines 75 - 82, The module declarations for '*.frag' and '*.vert' export a misleadingly named const png; update both declarations to use a clearer identifier (e.g., shader) and export that instead so the symbols reflect shader content; modify the declarations that currently read "const png" to "const shader" (and update the corresponding default export if needed) in the '*.frag' and '*.vert' module blocks.prismarine-viewer/examples/_FragmentShader.frag-21-23 (1)
21-23:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winGrayscale tint detection likely needs
&&, not||.To apply biome tint only to grayscale textures, all three channel pairs must be near-equal. With
||, any single near-equal pair triggers tinting, so saturated colors (e.g. pure red(1,0,0)wherey≈z, or magenta wherex≈z) get incorrectly multiplied byBiomeColor.🎨 Proposed fix
- if (abs(t.x-t.y) <=0.010 || abs(t.x-t.z)<=0.010 ||abs(t.y-t.z) <=0.010) + if (abs(t.x-t.y) <= 0.010 && abs(t.x-t.z) <= 0.010 && abs(t.y-t.z) <= 0.010)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/examples/_FragmentShader.frag` around lines 21 - 23, The grayscale-detection condition using t (the sampled color) incorrectly uses logical ORs so any single near-equal channel pair triggers tinting; change the condition in the fragment shader where FragColor is set with BiomeColor to require all three comparisons be near-equal (use && between abs(t.x-t.y) <= 0.010, abs(t.x-t.z) <= 0.010, and abs(t.y-t.z) <= 0.010) so only near-grayscale texels are multiplied by BiomeColor.src/topRightStats.ts-42-42 (1)
42-42:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove the unconditional stats disable.
Line 42 makes
hideStatsalways truthy, soinitWithRenderer()never initializes the panels and the existing toggle becomes dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/topRightStats.ts` at line 42, The hideStats variable is being forced truthy by the trailing "|| true", preventing initWithRenderer() from initializing panels and breaking the toggle; remove the unconditional "|| true" and compute hideStats from the actual sources (e.g., respect localStorage.hideStats and isCypress()), ensuring hideStats is a proper boolean so initWithRenderer() and the toggle can run as intended (refer to the hideStats declaration, localStorage.hideStats usage, isCypress(), and initWithRenderer()).prismarine-viewer/examples/newStats.ts-5-20 (1)
5-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMake
addNewStat()idempotent and use a unique element id.This always appends a new node and hardcodes
pane.id = 'fps-counter', so reconnects or multiple stats create duplicate DOM ids and leave orphaned panes behind.Suggested fix
export const addNewStat = (id: string, width = 80, x = rightOffset, y = 0) => { - const pane = document.createElement('div') - pane.id = 'fps-counter' + const existing = stats[id] as HTMLDivElement | undefined + const pane = existing ?? document.createElement('div') + pane.id = `stat-${id}` pane.style.position = 'fixed' pane.style.top = `${y}px` pane.style.right = `${x}px` // gray bg pane.style.backgroundColor = 'rgba(0, 0, 0, 0.5)' @@ pane.style.fontSize = '12px' pane.style.zIndex = '10000' pane.style.pointerEvents = 'none' - document.body.appendChild(pane) + if (!existing) document.body.appendChild(pane) stats[id] = pane - if (y === 0) { // otherwise it's a custom position + if (!existing && y === 0) { // otherwise it's a custom position rightOffset += width }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/examples/newStats.ts` around lines 5 - 20, The addNewStat function currently always creates and appends a new DOM node with a hardcoded id 'fps-counter', causing duplicate ids and orphaned panes; change it to be idempotent by deriving a unique element id from the passed id (e.g., prefix with 'stat-' or similar) and first check document.getElementById for that id—if it exists reuse that element instead of creating a new one (update its styles/position), otherwise create and append a new div; ensure you still set stats[id] = pane (the reused element when present) and remove the hardcoded 'fps-counter' id usage so reconnects/multiple stats don’t produce duplicate DOM ids.prismarine-viewer/viewer/prepare/webglData.ts-9-27 (1)
9-27:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
readFileSyncwill throw if a.png.mcmetais missing for an animated texture.Animated-ness here is inferred from atlas slicing (
animatedFrames), but the corresponding.png.mcmetais not guaranteed to exist (e.g. procedurally generated/added textures). A missing or malformed file makesreadFileSync/JSON.parsethrow and aborts the whole generation run. Guard withfs.existsSync(and ideally a try/catch) before reading.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/prepare/webglData.ts` around lines 9 - 27, The map over Object.entries(atlas.textures) currently calls fs.readFileSync(JSON.parse(...)) for animated textures which will throw if the corresponding .png.mcmeta is missing or malformed; update the mapping in the Object.fromEntries callback (the block that references animatedFrames, blockTexturesDir, join, readFileSync and JSON.parse) to first check fs.existsSync(join(blockTexturesDir, texture + '.png.mcmeta')) and, if missing, skip/return null for that texture, and wrap the readFileSync+JSON.parse in a try/catch to handle parse errors (log or skip) so a missing or invalid mcmeta file doesn’t abort generation; keep the final .filter(Boolean) behavior to remove skipped entries.prismarine-viewer/viewer/prepare/generateTextures.ts-52-52 (1)
52-52:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
-loverwritessupportedVersions.jsonwith a single version.When
-lis passed,versionsis just the last asset version, so this write replaces the full supported-versions list with one entry. If-lis meant for fast/partial local rebuilds, confirm downstream consumers ofsupportedVersions.jsonwon't break when it no longer lists all versions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/prepare/generateTextures.ts` at line 52, The current write in generateTextures.ts (`fs.writeFileSync(path.join(publicPath, 'supportedVersions.json'), ... versions)`) overwrites the full list when `-l` produces a single-element `versions`; instead, when `versions` has only one entry (the `-l` case) read the existing supportedVersions.json (if present), merge the new version into that array, deduplicate (e.g., via a Set), and then write the merged array back to path.join(publicPath, 'supportedVersions.json'); otherwise keep the existing behavior of writing the full `versions` array. Ensure you reference the `versions` variable, `publicPath`, and the target filename `supportedVersions.json` so downstream consumers still see a complete list.prismarine-viewer/viewer/prepare/generateTextures.ts-24-25 (1)
24-25:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
generateItemsAtlases()is async but not awaited.The returned promise is neither awaited nor
void-marked, so a rejection becomes an unhandled promise rejection (silent build failure), and item-atlas work races with the textures loop /console.timeEnd. Await it (orvoidit intentionally with a.catch).♻️ Proposed fix
- generateItemsAtlases() + await generateItemsAtlases()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/prepare/generateTextures.ts` around lines 24 - 25, The call to generateItemsAtlases() inside the Promise.resolve().then(async () => { ... }) is not awaited, causing potential unhandled rejections and race conditions; update that block to await generateItemsAtlases() (or explicitly handle its promise with .catch if you intentionally want fire-and-forget) so the async work completes before proceeding and any errors are logged/handled — locate the anonymous async callback that calls generateItemsAtlases() and change the call to await generateItemsAtlases() (or append .catch(...) if choosing the explicit void pattern).prismarine-viewer/viewer/lib/models.ts-443-453 (1)
443-453:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDestructuring a possibly-undefined
elements, plus an unreachable guard.At Line 449,
const [element] = firstVar.model?.elementsthrowsTypeError: undefined is not iterablewhenmodelorelementsis undefined. Line 451'sif (!element)is also dead — Line 450 already returned whenelementis falsy.🛡️ Proposed guard
- const [element] = firstVar.model?.elements - if (!element) return firstVar.model?.textures?.particle - if (!element/* || !(element?.from.every(a => a === 0) && element?.to.every(a => a === 16)) */) return + const element = firstVar.model?.elements?.[0] + if (!element) return firstVar.model?.textures?.particle return element.facesAlso note this duplicates the now-exported
findTextureInBlockStatesfromsrc/playerWindows.ts; the two implementations have already diverged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prismarine-viewer/viewer/lib/models.ts` around lines 443 - 453, The function findTextureInBlockStates currently destructures firstVar.model?.elements without ensuring model/elements exist and contains an unreachable guard; update it to check that firstVar.model and firstVar.model.elements are defined and are an array before doing const [element] = ..., safely handle the case when elements is empty by returning firstVar.model?.textures?.particle or undefined, and remove the redundant if (!element) dead-check; ensure all early returns preserve the original fallback branch (multipart lookup) and align behavior with the exported implementation in src/playerWindows.ts to avoid divergence.
✅ Actions performedReviews paused. |
zardoy
changed the title
3 participants
Insane performance from the original author
Deployments History
https://prismarine-9qryejdgq-zaro.vercel.app(broken)Summary by CodeRabbit
New Features
Improvements
Build & Development