New FAST renderer on clean WebGPU try: (webgpu.mcraft.fun) #120
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-pho51nlw6-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-6lrmuc5cl-zaro.vercel.app |
zardoy
changed the title
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-bc2ncs3uy-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-1x8yri3lq-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-gpwdotih8-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-90t7s5wjn-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-700h6sw1a-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-rfw8k4jpj-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-n3cdd3dup-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-h8wzgbvk8-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-arqd0oaby-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-qyfyijkk9-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-5aftdhvlw-zaro.vercel.app |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-b28fg7kj6-zaro.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
src/integratedServer/browserfsShared.ts (5)
18-18: Replace delete operator with undefined assignment for better performance.The delete operator can impact performance as indicated by the static analysis. Consider using undefined assignment instead.
- delete defaultMountablePoints['/world'] + defaultMountablePoints['/world'] = undefined🧰 Tools
🪛 Biome (1.9.4)
[error] 18-18: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
171-182: Improve error handling in mkdirRecursive function.The function catches errors but doesn't handle them appropriately. Some errors, like permission issues, should be propagated rather than silently ignored.
export const mkdirRecursive = async (path: string) => { const parts = path.split('/') let current = '' for (const part of parts) { current += part + '/' try { // eslint-disable-next-line no-await-in-loop await fs.promises.mkdir(current) } catch (err) { + // Ignore if directory already exists (EEXIST error) + if (err.code !== 'EEXIST') { + throw err + } } } }
322-322: Implement the TODO for handle cleanup.The comment indicates that handle cleanup needs to be implemented, which is important for resource management.
The TODO comment suggests that handle cleanup functionality is needed. Would you like me to help implement this feature? Having a clear cleanup strategy for handles is important to prevent resource leaks.
372-376: Improve error handling in fileExistsAsyncOptimized.The function has a commented-out error throw and a misleading ESLint disable comment. The error handling should be clearer and more consistent.
try { await fs.promises.readdir(path) } catch (err) { if (err.code === 'ENOTDIR') return true - // eslint-disable-next-line sonarjs/prefer-single-boolean-return if (err.code === 'ENOENT') return false - // throw err - return false + // Handle other potential errors + console.warn(`Unexpected error checking file existence: ${err.message || String(err)}`) + return false } return true
399-407: Improve error handling in the file copy operation.Consider adding more specific error handling for common file system errors like permission issues or disk space limitations.
// Copy file try { await fs.promises.writeFile(curPathDest, await fs.promises.readFile(curPathSrc) as any) console.debug('copied file', curPathSrc, curPathDest) } catch (err) { - console.error('Error copying file', curPathSrc, curPathDest, err) - throw err + console.error(`Error copying file from ${curPathSrc} to ${curPathDest}:`, err) + if (err.code === 'ENOSPC') { + throw new Error(`Not enough disk space to copy file: ${curPathSrc}`) + } else if (err.code === 'EACCES') { + throw new Error(`Permission denied while copying file: ${curPathSrc}`) + } else { + throw err + } } fileCopied?.(curPathDest)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/integratedServer/browserfsShared.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/integratedServer/browserfsShared.ts
[error] 18-18: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (2)
src/integratedServer/browserfsShared.ts (2)
186-203: Good job on addressing the async Promise executor issue.The Promise executor is correctly implemented without using async, which prevents potential unhandled rejections. This addresses the feedback from previous reviews.
104-108:❓ Verification inconclusive
Address the TODO comment about world corruption.
The comment indicates a critical issue with write operations causing world corruption on interactions like block placements. This needs to be addressed to ensure data integrity.
This appears to be a potential source of data corruption when blocks are placed while in read-only mode. Could you provide more information about this issue or how it manifests?
Action Required: Clarify and Resolve the World Corruption Risk
The code at
src/integratedServer/browserfsShared.tslines 104–108 currently shortcuts write operations in read-only mode by returning an empty buffer. However, the inline TODO note warns that this handling may lead to world corruption (e.g., during block placements). To proceed, please clarify the following:
- How exactly do these write operations lead to world corruption when the filesystem is in read-only mode?
- What is the expected behavior for write attempts in this state—should we throw an explicit error instead of silently returning an empty result?
- Do you have test cases or scenarios where this behavior has been observed, so we can better understand the risk?
Once we have more details on the manifestation of this issue and a clear strategy to prevent any data integrity problems, we can consider adjusting the implementation accordingly.
| // todo that's not correct | ||
| currentInternalFsState.openWriteOperations-- | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix inaccurate tracking of write operations.
The comment "todo that's not correct" indicates that the tracking of write operations is inaccurate, which could lead to incorrect state management.
Consider implementing proper operation tracking. Without more context on why this is incorrect, a thorough review of the operation counting logic is needed to ensure operations are properly tracked and closed.
| export async function removeFileRecursiveAsync (path) { | ||
| const errors = [] as Array<[string, Error]> | ||
| try { | ||
| const files = await fs.promises.readdir(path) | ||
|
|
||
| // Use Promise.all to parallelize file/directory removal | ||
| await Promise.all(files.map(async (file) => { | ||
| const curPath = join(path, file) | ||
| const stats = await fs.promises.stat(curPath) | ||
| if (stats.isDirectory()) { | ||
| // Recurse | ||
| await removeFileRecursiveAsync(curPath) | ||
| } else { | ||
| // Delete file | ||
| await fs.promises.unlink(curPath) | ||
| } | ||
| })) | ||
|
|
||
| // After removing all files/directories, remove the current directory | ||
| await fs.promises.rmdir(path) | ||
| } catch (error) { | ||
| errors.push([path, error]) | ||
| } | ||
|
|
||
| if (errors.length) { | ||
| setTimeout(() => { | ||
| console.error(errors) | ||
| throw new Error(`Error removing directories/files: ${errors.map(([path, err]) => `${path}: ${err.message}`).join(', ')}`) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor error handling in removeFileRecursiveAsync.
Using setTimeout to throw errors after the function has returned can lead to unhandled promise rejections and makes error handling unpredictable.
export async function removeFileRecursiveAsync (path) {
const errors = [] as Array<[string, Error]>
try {
const files = await fs.promises.readdir(path)
// Use Promise.all to parallelize file/directory removal
await Promise.all(files.map(async (file) => {
const curPath = join(path, file)
const stats = await fs.promises.stat(curPath)
if (stats.isDirectory()) {
// Recurse
await removeFileRecursiveAsync(curPath)
} else {
// Delete file
await fs.promises.unlink(curPath)
}
}))
// After removing all files/directories, remove the current directory
await fs.promises.rmdir(path)
} catch (error) {
errors.push([path, error])
}
if (errors.length) {
- setTimeout(() => {
- console.error(errors)
- throw new Error(`Error removing directories/files: ${errors.map(([path, err]) => `${path}: ${err.message}`).join(', ')}`)
- })
+ console.error(errors)
+ throw new Error(`Error removing directories/files: ${errors.map(([path, err]) => `${path}: ${err?.message || String(err)}`).join(', ')}`)
}
}📝 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.
| export async function removeFileRecursiveAsync (path) { | |
| const errors = [] as Array<[string, Error]> | |
| try { | |
| const files = await fs.promises.readdir(path) | |
| // Use Promise.all to parallelize file/directory removal | |
| await Promise.all(files.map(async (file) => { | |
| const curPath = join(path, file) | |
| const stats = await fs.promises.stat(curPath) | |
| if (stats.isDirectory()) { | |
| // Recurse | |
| await removeFileRecursiveAsync(curPath) | |
| } else { | |
| // Delete file | |
| await fs.promises.unlink(curPath) | |
| } | |
| })) | |
| // After removing all files/directories, remove the current directory | |
| await fs.promises.rmdir(path) | |
| } catch (error) { | |
| errors.push([path, error]) | |
| } | |
| if (errors.length) { | |
| setTimeout(() => { | |
| console.error(errors) | |
| throw new Error(`Error removing directories/files: ${errors.map(([path, err]) => `${path}: ${err.message}`).join(', ')}`) | |
| }) | |
| } | |
| export async function removeFileRecursiveAsync (path) { | |
| const errors = [] as Array<[string, Error]> | |
| try { | |
| const files = await fs.promises.readdir(path) | |
| // Use Promise.all to parallelize file/directory removal | |
| await Promise.all(files.map(async (file) => { | |
| const curPath = join(path, file) | |
| const stats = await fs.promises.stat(curPath) | |
| if (stats.isDirectory()) { | |
| // Recurse | |
| await removeFileRecursiveAsync(curPath) | |
| } else { | |
| // Delete file | |
| await fs.promises.unlink(curPath) | |
| } | |
| })) | |
| // After removing all files/directories, remove the current directory | |
| await fs.promises.rmdir(path) | |
| } catch (error) { | |
| errors.push([path, error]) | |
| } | |
| if (errors.length) { | |
| console.error(errors) | |
| throw new Error(`Error removing directories/files: ${errors.map(([path, err]) => `${path}: ${err?.message || String(err)}`).join(', ')}`) | |
| } | |
| } |
|
Deployed to Vercel Preview: https://prismarine-7cretdayd-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (25)
src/globals.js (1)
11-11: Consider the implications of setting such a high listener limit.Setting the maximum listeners to 10,000 prevents warnings but may indicate an architectural concern. A high number of listeners could:
- Lead to memory leaks if not properly cleaned up
- Impact performance when events are emitted
- Suggest a design where event listeners might be better managed differently
Consider these alternatives:
- Ensure listeners are properly removed when no longer needed
- Add comments explaining why this high limit is necessary
- Monitor actual listener count during development to determine a more appropriate limit
- Implement a more structured event management system if the WebGPU renderer truly requires thousands of listeners
src/core/timers.ts (2)
127-134: Consider adding explicit return type annotationThe function now returns a Promise, but lacks an explicit TypeScript return type annotation. For better type safety and code clarity:
-return async () => { +return async (): Promise<void> => {
127-138: Inconsistent error handling approachThere's inconsistency in error handling - the outer try-catch still uses
console.errorwhile the inner one now usesconsole.warn. Consider using a consistent approach to error logging based on severity.try { // ...existing code... return async (): Promise<void> => { try { oscillator.stop() await audioContext.close() } catch (err) { console.warn('Error stopping silent audio:', err) } } } catch (err) { - console.error('Error creating silent audio:', err) + console.warn('Error creating silent audio:', err) return () => {} }src/index.ts (2)
335-348: Avoid usinganytype for CustomClient.The
anytype bypasses TypeScript's type checking and could lead to runtime errors. Consider defining a proper interface or type for CustomClient.-let CustomClient: any +// Define a proper interface for the client +interface CustomClientInterface { + // Add relevant properties and methods +} +let CustomClient: CustomClientInterface
349-368: Clean up commented-out server initialization code.There's a significant block of commented-out code related to server initialization and loading status updates. If this functionality has been moved elsewhere or is no longer needed, consider removing it to improve code readability.
src/integratedServer/worker.ts (3)
8-8: Prefer defining a distinct interface foroptions.Currently,
serveris declared asFullServer & { options }without specifying the shape ofoptions. Consider defining a concrete interface or type foroptionsto improve readability and type safety.- let server: import('flying-squid/dist/index').FullServer & { options } + interface ServerOptions { + // add explicit fields here + } + let server: import('flying-squid/dist/index').FullServer & { options: ServerOptions }
41-41: Add a type forprocessDataGlobal.Storing a critical function or object in an untyped variable can introduce maintainability issues.
-let processDataGlobal +let processDataGlobal: (data: unknown) => void
63-63: Type theserverOptionsparameter for clarity.Consider defining an interface for
serverOptionsto ensure a consistent usage across the codebase.- const startServer = async (serverOptions) => { + interface ServerStartOptions { + // define relevant fields + } + const startServer = async (serverOptions: ServerStartOptions) => {src/integratedServer/main.ts (1)
36-48: Avoid duplicatingrestorePatchedDataDeeplogic.A similar function exists in
worker.ts. Extract it to a shared utility module to promote reusability and reduce code duplication.renderer/viewer/webgpu/worldrendererWebgpu.ts (9)
66-74: Replaceanytype with a proper type definition.Using
anybypasses TypeScript's type checking, which can lead to runtime errors. Consider defining a proper interface for the proxy.getPlaceholderChannel () { return new Proxy({}, { get: (target, p) => (...args) => { void this.readyWorkerPromise.then(() => { this.webgpuChannel[p](...args) }) } - }) as any // placeholder to avoid crashes + }) as typeof workerProxyType['__workerProxy'] // placeholder to avoid crashes }
87-94: Simplify camera vector preparation.The nested reduce functions make the code harder to read. Consider using a more straightforward approach with object literals.
sendCameraToWorker () { - const cameraVectors = ['rotation', 'position'].reduce((acc, key) => { - acc[key] = ['x', 'y', 'z'].reduce((acc2, key2) => { - acc2[key2] = this.camera[key][key2] - return acc2 - }, {}) - return acc - }, {}) as any + const cameraVectors = { + position: { + x: this.camera.position.x, + y: this.camera.position.y, + z: this.camera.position.z + }, + rotation: { + x: this.camera.rotation.x, + y: this.camera.rotation.y, + z: this.camera.rotation.z + } + } this.webgpuChannel.camera({ ...cameraVectors, fov: this.camera.fov }) }
181-182: Implement the empty updatePosDataChunk method.The method is empty but seems to be part of the renderer API. Either implement the required functionality or add a TODO comment explaining why it's empty.
updatePosDataChunk (key: string) { + // TODO: Implement position data chunk updates for WebGPU renderer }
200-202: Consider implementing updateShowChunksBorder.The method contains a TODO comment but doesn't provide any implementation. This affects feature completeness.
Would you like me to propose an implementation for this method based on how it might be implemented in other renderers?
240-242: Clarify playground setting.Line 240 is commented out, and line 241 sets
playgroundtofalsedirectly. This suggests a temporary change that may need revisiting.- // const playground = this.displayOptions.isPlayground - const playground = false + // Always false for now, but may need to be configurable in the future + const playground = false // this.displayOptions.isPlayground
245-247: Verify canvas element existence before removal.The code removes the existing canvas without checking if it exists, which could lead to errors if the element isn't found.
const existingCanvas = document.getElementById('viewer-canvas') -existingCanvas?.remove() +if (existingCanvas) { + existingCanvas.remove() +}
255-267: Consider using a more dynamic approach for worker path.The worker path is hardcoded, which might cause issues if the application structure changes. Consider using a configurable or relative path.
if (USE_WORKER) { - this.worker = new Worker('./webgpuRendererWorker.js') + // Use a configurable path or import.meta.url for relative paths + const workerPath = new URL('./webgpuRendererWorker.js', import.meta.url).href + this.worker = new Worker(workerPath) console.log('starting offscreen') } else if (globalThis.webgpuRendererChannel) {
280-285: Replace setTimeout(0) with a more explicit approach.Using setTimeout with a 0ms delay is a hacky way to force an event loop tick. Consider using a more explicit approach.
if (!USE_WORKER) { // wait for the .canvas() message to be processed (it's async since we still use message channel) - await new Promise(resolve => { - setTimeout(resolve, 0) - }) + // Use queueMicrotask or Promise.resolve() to defer execution to the next event loop tick + await Promise.resolve() }
398-398: Consider extracting build info to a configuration.Hardcoding build information directly in the UI code makes it harder to maintain. Consider extracting it to a configuration file.
- updateTextBuild(`WebGPU Renderer Demo by @SA2URAMI. Build: ${process.env.NODE_ENV === 'development' ? 'dev' : process.env.RELEASE_TAG}`) + const buildInfo = { + author: '@SA2URAMI', + version: process.env.NODE_ENV === 'development' ? 'dev' : process.env.RELEASE_TAG + } + updateTextBuild(`WebGPU Renderer Demo by ${buildInfo.author}. Build: ${buildInfo.version}`)src/browserfs.ts (7)
7-7: Consider renaminginstallResourcepackPackfor clarity.
The function name can be confusing. A more concise name (e.g.,installResourcePack) might be clearer.Suggested rename:
- import { installResourcepackPack, updateTexturePackInstalledState } from './resourcePack' + import { installResourcePack, updateTexturePackInstalledState } from './resourcePack'Also update its usage at line 226 accordingly:
- void installResourcepackPack(file, createFullScreenProgressReporter()).catch((err) => { + void installResourcePack(file, createFullScreenProgressReporter()).catch((err) => {
134-134: Consider handling potential failures frommountRemoteFsBackend.
If an error is thrown here, it won't be caught or communicated. A simple try/catch block and user-facing message could improve resilience.- await mountRemoteFsBackend(fsState) + try { + await mountRemoteFsBackend(fsState) + } catch (e) { + showNotification('Error mounting remote FS', e.message, true) + throw e + }
141-158: Remove or revise large commented block.
This entire ZipFS mounting logic is disabled; if no longer needed, removing it keeps the file clean. If you intend to re-enable soon, add a clarifying comment.
198-198: Revisit commented-out cleanup logic.
IfpossiblyCleanHandle()is obsolete, consider removing it. If necessary, re-enable it or provide an alternative resource cleanup mechanism.
251-273: Most of the resource-pack detection code is commented out.
Refactor or remove the code if the final approach is to let users install texture packs via the options menu. Keeping it commented might cause confusion.Would you like help investigating the mentioned “browserfs read errors”?
275-275: Unused constant.
const SUPPORT_WRITE = trueis unused now that the code referencing it is commented out.Remove it or reintroduce it together with the relevant logic.
277-321: Large commented-out block for directory-based world loading.
If you no longer plan to support direct directory handles, consider removing this block. Unused code can hinder readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
renderer/playground/webgpuRendererShared.ts(1 hunks)renderer/viewer/webgpu/worldrendererWebgpu.ts(1 hunks)src/browserfs.ts(5 hunks)src/core/timers.ts(1 hunks)src/globals.js(1 hunks)src/index.ts(10 hunks)src/integratedServer/main.ts(1 hunks)src/integratedServer/worker.ts(1 hunks)src/optionsStorage.ts(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/optionsStorage.ts
- renderer/playground/webgpuRendererShared.ts
🧰 Additional context used
🧬 Code Definitions (2)
renderer/viewer/webgpu/worldrendererWebgpu.ts (5)
renderer/playground/webgpuRendererShared.ts (2)
defaultWebgpuRendererParams(4-13)rendererParamsGui(15-25)renderer/playground/webgpuRendererWorker.ts (2)
workerProxyType(95-281)canvas(97-113)renderer/viewer/webgpu/graphicsBackendWebgpu.ts (1)
WebgpuInitOptions(5-7)src/appViewer.ts (1)
DisplayWorldOptions(61-68)renderer/playground/webgpuBlockModels.ts (1)
prepareCreateWebgpuBlocksModelsData(9-157)
src/integratedServer/worker.ts (5)
src/integratedServer/customServer.ts (1)
createCustomServerImpl(5-23)src/createLocalServer.ts (1)
startLocalServer(3-13)src/integratedServer/browserfsShared.ts (1)
localFsState(44-46)src/loadSave.ts (1)
fsState(19-19)src/integratedServer/browserfsServer.ts (1)
mountFsBackend(42-50)
🪛 Biome (1.9.4)
src/integratedServer/worker.ts
[error] 37-37: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
[error] 16-16: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 17-17: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
🔇 Additional comments (16)
src/core/timers.ts (2)
130-130: Properly awaiting AudioContext.close()Good improvement! Adding
awaitensures the audio context is fully closed before continuing.AudioContext.close()returns a Promise that should be properly awaited.
132-132: Changed error severity level from error to warningThe error handling has been changed from
console.errortoconsole.warn, indicating this is considered a less critical issue. This is reasonable since it's occurring during cleanup, but ensure this aligns with your error handling policy.src/index.ts (5)
94-95: Import refactoring reflects the new WebGPU renderer architecture.The imports have been updated to use the new integrated server main module, which aligns with the WebGPU renderer implementation mentioned in the PR objectives.
198-202: Improved server shutdown approach.The server shutdown logic has been refactored to use a dedicated method
destroyLocalServerMain(false)instead of manual reference clearing. This is a cleaner approach that ensures proper cleanup of resources.
708-708: Good use of async/await for world initialization.Adding the
awaitkeyword to theappViewer.startWorldcall ensures proper sequencing of world initialization, which prevents potential race conditions that could occur if the world isn't fully loaded before subsequent operations.
220-223:✅ Verification successful
Verify if file system cleanup is still needed.
The commented-out
possiblyCleanHandlecode suggests file system cleanup might no longer be implemented. Ensure this doesn't cause resource leaks.
🏁 Script executed:
#!/bin/bash # Check if possiblyCleanHandle is used elsewhere or has been replaced rg "possiblyCleanHandle" --type ts # Check for other file cleanup mechanisms that might have replaced it rg "cleanHandle|cleanFs|fileHandle" --type tsLength of output: 347
File System Cleanup Verified – Relying on cleanFs()
After verifying the shell script results, it's clear that file system cleanup is still being handled in the code. The commented-out
possiblyCleanHandleblock insrc/index.tshas been replaced by an active implementation using thecleanFs()function. The search also confirms that whilepossiblyCleanHandleis defined in other parts of the codebase (insrc/integratedServer/browserfsShared.ts), it isn’t used in the cleanup process insrc/index.ts.Actionable Points:
- Confirm that
cleanFs()insrc/index.tsmeets all file system cleanup requirements.- Consider removing the commented-out
possiblyCleanHandlecode fromsrc/index.tsto reduce confusion and maintain code clarity.
456-461:❓ Verification inconclusive
Ensure proper client initialization for all connection types.
The commented-out
CustomChannelClienton line 460 suggests a change in client initialization for replay sessions. Verify that replay functionality still works correctly with the new WebGPU renderer.
🏁 Script executed:
#!/bin/bash # Look for tests or usages of replay functionality rg "localReplaySession|startLocalReplayServer" --type ts -A 5 -B 5Length of output: 4922
Review Update: Verify Replay Session Client Initialization with WebGPU Renderer
- The grep results confirm that replay functionality is exercised (see usage in
src/packetsReplay/replayPackets.tsand the invocation ofstartLocalReplayServer).- In
src/index.ts(lines 456–461), whenlocalReplaySessionis active the configuration only defines an emptyconnect()function, and the intended client assignment (CustomChannelClient as any) remains commented out.- Please ensure that replay sessions are properly initialized for the new WebGPU renderer. If a custom client is still required for replay, update the initialization accordingly; if not, remove the commented code to avoid confusion.
- Additionally, verify that all replay functionality tests pass with the current renderer integration.
src/integratedServer/worker.ts (1)
16-17: Avoid using'{}'as a type
This usage was flagged in a previous review. Leaving it unresolved can lead to confusion, as'{}'matches any non-nullable value.export interface BackEvents { - ready: {} - quit: {} + ready: Record<string, never> + quit: Record<string, never> packet: any ... }🧰 Tools
🪛 Biome (1.9.4)
[error] 16-16: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 17-17: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
src/integratedServer/main.ts (2)
17-34:lastOptionsis never assigned.This was raised in a prior review. Returning a never-assigned variable can cause unexpected results. Consider setting
lastOptionsafter initializing or starting the server.+ lastOptions = serverOptions
56-56:❓ Verification inconclusive
Verify the availability of
'./integratedServer.js'.Ensure your build process or deployment correctly generates and serves
integratedServer.js. Otherwise, instantiating theWorkerwill fail.
🏁 Script executed:
#!/bin/bash # Search for 'integratedServer.js' generation in build scripts: rg "integratedServer.js"Length of output: 410
Attention Required: Confirm Worker Script Accessibility and Correct Path
In
src/integratedServer/main.ts(line 56), the Worker is instantiated with:worker = new Worker('./integratedServer.js')Our search shows that your build process generates
integratedServer.jsin thedistdirectory (see references inrenderer/rsbuildSharedConfig.tsandbuildWorkers.mjs). Please verify that at runtime the worker file is accessible by this relative path. If it isn’t—because the file is actually served from thedistfolder—you might need to adjust the Worker instantiation (e.g., usenew Worker('./dist/integratedServer.js')) or ensure that your deployment properly maps the generated file to the expected location.renderer/viewer/webgpu/worldrendererWebgpu.ts (1)
58-64: Add MessagePort cleanup in destroy method.When the worker is a MessagePort, it should be closed to avoid resource leaks. This was previously identified but not implemented.
override destroy () { this.abortController.abort() this.webgpuChannel.destroy() // still needed in case if running in the same thread if (this.worker instanceof Worker) { this.worker.terminate() + } else if (this.worker instanceof MessagePort) { + this.worker.close() } }src/browserfs.ts (5)
4-4: No issues with the added import.
This import forresetOptionsis straightforward and matches its usage below.
14-14: Looks good.
Importing functions frombrowserfsSharedensures a clearer separation of responsibilities.
131-133: State assignment appears correct.
StoringindexFileUrlandbaseUrlinfsStateshould simplify tracking the active remote backend.
95-121:✅ Verification successful
Validate new
indexFileUrlusage.
The logic for discovering and storingindexFileUrlappears sound. However, ensure that no additional calls rely on the old approach.
🏁 Script executed:
#!/bin/bash # Verify usage of the newly introduced indexFileUrl property. rg -A 3 'indexFileUrl'Length of output: 600
IndexFileUrl Integration Validated
The updated implementation in
src/browserfs.tscorrectly sets and routesindexFileUrlinto the file system state. The grep output confirms that:
indexFileUrlis assigned from the fetched URL.- It is subsequently stored in
fsState.usingIndexFileUrl.- No other parts of the codebase reference an older approach.
It appears that the new usage is isolated and fully integrated. Please double-check that any downstream dependencies (e.g., within
mountRemoteFsBackend) consistently referencefsState.usingIndexFileUrl.
1-1:❓ Verification inconclusive
Double-check Node’s
fsusage in a browser environment.
If this code is intended for the browser, relying on Node’sfscould lead to compatibility issues unless properly polyfilled or only used when running in Node-like environments.Please verify all references to
fswith the following script to confirm whether they’re guarded by environment checks or replaced in production builds:
🏁 Script executed:
#!/bin/bash # Search for usage of `fs` to ensure calls are safe for the browser environment. rg -A 5 $'import.*fs.*?from'Length of output: 21004
Action: Verify Conditional Usage of Node’s fs in Browser Context
In the file src/browserfs.ts, the import of Node’s
fsmodule remains unguarded:import * as fs from 'fs'Since this module also imports BrowserFS (which provides a browser-compatible filesystem polyfill), please ensure that any direct calls to Node’s native
fsare either conditionally executed (e.g., wrapped by environment checks) or properly replaced/aliased in the browser production build. This verification is especially important given the widespread usage offsacross the codebase, as shown by the search results.
- Confirm that the bundler or runtime environment correctly handles the unguarded
fsimport when running in a browser.- If the file is exclusively for Node environments, consider clarifying this by renaming the file or adding explicit comments to avoid confusion.
| oscillator.start() | ||
|
|
||
| return () => { | ||
| return async () => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Return type is now asynchronous - confirm consumer code compatibility
The function now returns an async function, which means it returns a Promise. This is a breaking change that might require updates to any code that calls this function.
🏁 Script executed:
#!/bin/bash
# Find all usages of preventThrottlingWithSound to verify if consumer code handles the async return value
rg "preventThrottlingWithSound" --type ts -A 3 -B 3Length of output: 975
Update consumer code to properly await the async function
The function now returns an asynchronous function (i.e. a Promise resolving to a cleanup callback). In its current usage (in src/mineflayer/timers.ts), the return value is directly assigned and passed to bot.on('end') without being awaited, which could lead to unexpected behavior. Please update the call site to handle the promise correctly, for example:
- Change from:
const unsub = preventThrottlingWithSound(); bot.on('end', unsub);
- To something like:
(async () => { const unsub = await preventThrottlingWithSound(); bot.on('end', unsub); })();
Review all consumer code paths for preventThrottlingWithSound to ensure they correctly await its asynchronous return, thereby maintaining compatibility and preventing runtime issues.
| globalThis.postMessage({ type, data }, ...args) | ||
| } catch (err) { | ||
| // eslint-disable-next-line no-debugger | ||
| debugger |
There was a problem hiding this comment.
Remove the leftover debugger statement.
Having debugger in production code can unintentionally pause execution in debugging tools.
- debugger📝 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.
| debugger |
🧰 Tools
🪛 Biome (1.9.4)
[error] 37-37: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
| const addWebgpuDebugUi = (worker, isPlayground, worldRenderer: WorldRendererWebgpu) => { | ||
| // todo destroy | ||
| const mobile = isMobile() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement UI cleanup in addWebgpuDebugUi.
The TODO comment indicates that cleanup for the debug UI is not implemented, which could lead to memory leaks.
The debug UI should be properly cleaned up when no longer needed. Consider implementing a cleanup function that:
- Removes event listeners
- Clears timers
- Removes DOM elements
This function should be called when the renderer is destroyed.
| const imageBlob = await fetch(image).then(async (res) => res.blob()) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for fetch operation.
The fetch operation could fail for various reasons (network issues, invalid URL, etc.), but there's no error handling.
- const imageBlob = await fetch(image).then(async (res) => res.blob())
+ let imageBlob: Blob;
+ try {
+ const response = await fetch(image);
+ if (!response.ok) {
+ throw new Error(`Failed to fetch image: ${response.status} ${response.statusText}`);
+ }
+ imageBlob = await response.blob();
+ } catch (error) {
+ console.error('Failed to fetch texture image:', error);
+ this.issueReporter.reportProblem(false, `Failed to load textures: ${error.message}`);
+ throw error;
+ }📝 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.
| const imageBlob = await fetch(image).then(async (res) => res.blob()) | |
| let imageBlob: Blob; | |
| try { | |
| const response = await fetch(image); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch image: ${response.status} ${response.statusText}`); | |
| } | |
| imageBlob = await response.blob(); | |
| } catch (error) { | |
| console.error('Failed to fetch texture image:', error); | |
| this.issueReporter.reportProblem(false, `Failed to load textures: ${error.message}`); | |
| throw error; | |
| } |
| searchParams.set(key, String(value)) | ||
| window.location.search = searchParams.toString() |
There was a problem hiding this comment.
Fix URL search parameter update.
The code sets the search parameter to the old value value instead of the new value newVal.
if (rendererParamsGui[key]?.qsReload) {
const searchParams = new URLSearchParams(window.location.search)
- searchParams.set(key, String(value))
+ searchParams.set(key, String(newVal))
window.location.search = searchParams.toString()
}📝 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.
| searchParams.set(key, String(value)) | |
| window.location.search = searchParams.toString() | |
| if (rendererParamsGui[key]?.qsReload) { | |
| const searchParams = new URLSearchParams(window.location.search) | |
| searchParams.set(key, String(newVal)) | |
| window.location.search = searchParams.toString() | |
| } |
| worker.addEventListener('message', (e: any) => { | ||
| if (e.data.type === 'fps') { | ||
| updateText(`FPS: ${e.data.fps}`) | ||
| if (prevTimeout) clearTimeout(prevTimeout) | ||
| prevTimeout = setTimeout(() => { | ||
| updateText('<hanging>') | ||
| }, 1002) | ||
| } | ||
| if (e.data.type === 'stats') { | ||
| updateTextGpuStats(e.data.stats) | ||
| // worldRenderer.rendererDevice = `${e.data.device} WebGL data: ${WorldRendererThree.getRendererInfo(new THREE.WebGLRenderer())}` | ||
| } | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for event listener in addWebgpuDebugUi.
The event listener is added but never removed, which could lead to memory leaks if the component is destroyed and recreated.
+ const messageListener = (e: any) => {
+ if (e.data.type === 'fps') {
+ updateText(`FPS: ${e.data.fps}`)
+ if (prevTimeout) clearTimeout(prevTimeout)
+ prevTimeout = setTimeout(() => {
+ updateText('<hanging>')
+ }, 1002)
+ }
+ if (e.data.type === 'stats') {
+ updateTextGpuStats(e.data.stats)
+ // worldRenderer.rendererDevice = `${e.data.device} WebGL data: ${WorldRendererThree.getRendererInfo(new THREE.WebGLRenderer())}`
+ }
+ }
+ worker.addEventListener('message', messageListener)
+
+ // Clean up on abort
+ worldRenderer.abortController.signal.addEventListener('abort', () => {
+ worker.removeEventListener('message', messageListener)
+ if (prevTimeout) clearTimeout(prevTimeout)
+ })
- worker.addEventListener('message', (e: any) => {
- if (e.data.type === 'fps') {
- updateText(`FPS: ${e.data.fps}`)
- if (prevTimeout) clearTimeout(prevTimeout)
- prevTimeout = setTimeout(() => {
- updateText('<hanging>')
- }, 1002)
- }
- if (e.data.type === 'stats') {
- updateTextGpuStats(e.data.stats)
- // worldRenderer.rendererDevice = `${e.data.device} WebGL data: ${WorldRendererThree.getRendererInfo(new THREE.WebGLRenderer())}`
- }
- })📝 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.
| worker.addEventListener('message', (e: any) => { | |
| if (e.data.type === 'fps') { | |
| updateText(`FPS: ${e.data.fps}`) | |
| if (prevTimeout) clearTimeout(prevTimeout) | |
| prevTimeout = setTimeout(() => { | |
| updateText('<hanging>') | |
| }, 1002) | |
| } | |
| if (e.data.type === 'stats') { | |
| updateTextGpuStats(e.data.stats) | |
| // worldRenderer.rendererDevice = `${e.data.device} WebGL data: ${WorldRendererThree.getRendererInfo(new THREE.WebGLRenderer())}` | |
| } | |
| }) | |
| const messageListener = (e: any) => { | |
| if (e.data.type === 'fps') { | |
| updateText(`FPS: ${e.data.fps}`) | |
| if (prevTimeout) clearTimeout(prevTimeout) | |
| prevTimeout = setTimeout(() => { | |
| updateText('<hanging>') | |
| }, 1002) | |
| } | |
| if (e.data.type === 'stats') { | |
| updateTextGpuStats(e.data.stats) | |
| // worldRenderer.rendererDevice = `${e.data.device} WebGL data: ${WorldRendererThree.getRendererInfo(new THREE.WebGLRenderer())}` | |
| } | |
| } | |
| worker.addEventListener('message', messageListener) | |
| // Clean up on abort | |
| worldRenderer.abortController.signal.addEventListener('abort', () => { | |
| worker.removeEventListener('message', messageListener) | |
| if (prevTimeout) clearTimeout(prevTimeout) | |
| }) |
|
Deployed to Vercel Preview: https://prismarine-5o52rrley-zaro.vercel.app |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
renderer/viewer/three/world/cursorBlock.ts (1)
93-105:⚠️ Potential issueMethod signature change may cause unintended mutation
The
updateBreakAnimationmethod has been refactored to work with separated position and shape data instead of a complete Block object. While this decoupling is good for the new renderer architecture, there's a potential issue with line 99.The line
position.add(blockPosition)mutates the originalpositionobject frommergedShape. IfmergedShape.positionis used elsewhere after this call, it will contain an incorrect position (with blockPosition already added). Consider using a clone of the position to avoid this side effect:- position.add(blockPosition) - this.blockBreakMesh.position.set(position.x, position.y, position.z) + const worldPosition = position.clone().add(blockPosition) + this.blockBreakMesh.position.set(worldPosition.x, worldPosition.y, worldPosition.z)
🧹 Nitpick comments (5)
renderer/viewer/lib/basePlayerState.ts (3)
17-18: Consider defining a stronger type for block shape fields.
Usinganyforpositionand other fields could lead to runtime surprises. A dedicated interface or a more specific type would improve clarity and maintainability.
45-51: Consolidate coordinate properties into a dedicated type or interface.
Reusing(x, y, z)across multiple structures may lead to duplication. A sharedVec3Liketype can improve consistency.
52-59: Apply the same coordinate-type approach for 'diggingBlock'.
Consider a shared interface or type to avoid repeating coordinate definitions.renderer/viewer/three/worldrendererThree.ts (1)
122-139: Introduce descriptive constants for fog distance values.
Replacing hard-coded values (e.g., 100 vs 20) with well-named constants could improve clarity. Overall, the reactive updates for player state are implemented cleanly.renderer/viewer/three/world/cursorBlock.ts (1)
95-95: Parameter validation inconsistencyThe
mergedShapeparameter is marked as optional in the method signature, but the early return condition treats it as required. This could lead to unexpected behavior if callers assume they can omit it. Consider either:
- Making the parameter required in the signature, or
- Handling the undefined case differently if it's a valid scenario
- updateBreakAnimation (blockPosition: { x: number, y: number, z: number } | undefined, stage: number | null, mergedShape?: BlockShape) { + updateBreakAnimation (blockPosition: { x: number, y: number, z: number } | undefined, stage: number | null, mergedShape: BlockShape) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
renderer/viewer/lib/basePlayerState.ts(3 hunks)renderer/viewer/lib/mesher/mesher.ts(4 hunks)renderer/viewer/lib/mesher/models.ts(10 hunks)renderer/viewer/three/graphicsBackend.ts(1 hunks)renderer/viewer/three/world/cursorBlock.ts(3 hunks)renderer/viewer/three/worldrendererThree.ts(1 hunks)renderer/viewer/webgpu/worldrendererWebgpu.ts(1 hunks)src/controls.ts(3 hunks)src/mineflayer/plugins/mouse.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- renderer/viewer/lib/mesher/mesher.ts
- renderer/viewer/three/graphicsBackend.ts
- renderer/viewer/webgpu/worldrendererWebgpu.ts
- renderer/viewer/lib/mesher/models.ts
🧰 Additional context used
🧬 Code Definitions (1)
src/mineflayer/plugins/mouse.ts (1)
src/mineflayer/playerState.ts (1)
playerState(206-206)
🪛 Biome (1.9.4)
src/controls.ts
[error] 950-950: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
🔇 Additional comments (11)
src/controls.ts (3)
420-422: Improved state check for URL locking.Good enhancement by adding the
fsState.inMemorySavecondition check. This prevents potential errors when attempting to accessinMemorySavePathwhen there is no in-memory save available.
948-951: Avoid leavingdebuggerstatements in production code.According to static analysis, the
debuggerstatement is flagged. Removing or wrapping it behind a dev build check can prevent accidental triggers in production.if (e.code === 'KeyK' && e.altKey && !e.shiftKey && !e.ctrlKey && !e.metaKey) { - // eslint-disable-next-line no-debugger - debugger + // TODO: Use console.log or dev-tools breakpoints in development only }🧰 Tools
🪛 Biome (1.9.4)
[error] 950-950: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
952-954:❓ Verification inconclusive
New keyboard shortcut to toggle renderer engines.
You've implemented a useful Alt+J keyboard shortcut to toggle between the WebGPU and ThreeJS renderers, which aligns with the main objective of this PR to add WebGPU as a rendering option.
🏁 Script executed:
#!/bin/bash # Check for the renderer options definition in optionsGuiScheme.tsx echo "Checking for renderer options definition..." rg -A 10 "activeRenderer" --include="*.tsx"Length of output: 582
Renderer Toggle Shortcut Functionality – Please Verify Active Renderer Definition
The Alt+J shortcut implementation in
src/controls.ts(lines 952–954) cleanly toggles between the WebGPU and ThreeJS renderers as intended. The logic correctly checks that only Alt (without Shift, Ctrl, or Meta) is pressed and then swaps theactiveRenderervalue.Before finalizing this change, please manually verify that the
activeRendererproperty is properly defined and used in the corresponding UI/options file (for example, inoptionsGuiScheme.tsxor the relevant component), as our initial search usingrgencountered flag issues.
- File:
src/controls.ts(lines 952–954) – toggling logic is correctly implemented.- Action Needed: Confirm that the
activeRendererproperty exists and is managed consistently across the codebase.renderer/viewer/lib/basePlayerState.ts (3)
5-5: Use of 'ref' from 'valtio' is clear and valid.
No issues noted.
40-40: Addition of 'waterBreathing' property looks consistent.
This property aligns well with existing booleans likeinWaterand improves readability.
67-68: Reactive array reference for 'backgroundColor' is appropriate.
Good usage ofrefto optimize reactivity.src/mineflayer/plugins/mouse.ts (3)
14-14: Clearing 'lookingAtBlock' on missing block data is straightforward.
This prevents stale or incorrect references.
19-26: Reactive assignment of 'lookingAtBlock' data.
Storing block details inplayerStateis a clean and maintainable approach.
32-39: Managing 'diggingBlock' reactively is well-structured.
Conditionally clearing data when the break stage is absent ensures correct state transitions.renderer/viewer/three/world/cursorBlock.ts (2)
6-6: Import updated to use new block shape typesThe import statement has been updated to use the new
BlockShapeandBlocksShapestypes from the basePlayerState module. This change aligns with the refactoring of the block rendering system to support the new WebGPU renderer.
121-121: Type signature improved for block shape handlingThe parameter type for
shapePositionshas been updated to use the more specificBlocksShapestype, which improves type safety and makes the expected data structure clearer.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
renderer/viewer/lib/worldrendererCommon.ts (1)
163-167:⚠️ Potential issuePotential memory leak from interval.
A
setIntervalis created but no reference is stored to clear it later. If multiple renderer instances are created or the renderer is destroyed and recreated, this may lead to memory leaks.Consider storing the interval ID as a class property and clearing it in the
destroy()method:+private geometryStatsInterval?: NodeJS.Timeout constructor(public readonly resourcesManager: ResourcesManager, public displayOptions: DisplayWorldOptions, public version: string) { // ... - setInterval(() => { + this.geometryStatsInterval = setInterval(() => { this.geometryReceiveCountPerSec = Object.values(this.geometryReceiveCount).reduce((acc, curr) => acc + curr, 0) this.geometryReceiveCount = {} this.updateChunksStats() }, 500) } destroy() { // ... + if (this.geometryStatsInterval) clearInterval(this.geometryStatsInterval) this.active = false // ... }
🧹 Nitpick comments (1)
src/loadSave.ts (1)
19-20: Well-structured fsState initialization with global exposureThe new initialization approach using
structuredClone(initialFsState)is a good practice as it:
- Creates a deep copy of the initial state to prevent reference issues
- Centralizes the state definition in one place
- Makes the state structure more predictable and maintainable
Making the state available globally with
globalThis.fsState = fsStateimproves accessibility across modules but consider documenting this global access point for other developers.Consider adding a comment explaining the purpose of making fsState global:
export const fsState = proxy(structuredClone(initialFsState)) +// Make fsState globally accessible for debugging and cross-module access globalThis.fsState = fsState
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (9)
renderer/viewer/lib/mesher/mesher.ts(4 hunks)renderer/viewer/lib/worldrendererCommon.ts(4 hunks)renderer/viewer/webgpu/worldrendererWebgpu.ts(1 hunks)src/controls.ts(2 hunks)src/loadSave.ts(3 hunks)src/optionsGuiScheme.tsx(2 hunks)src/optionsStorage.ts(3 hunks)src/utils.ts(1 hunks)src/watchOptions.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/watchOptions.ts
- src/optionsGuiScheme.tsx
- src/optionsStorage.ts
- src/utils.ts
- renderer/viewer/webgpu/worldrendererWebgpu.ts
- renderer/viewer/lib/mesher/mesher.ts
🧰 Additional context used
🧬 Code Definitions (2)
renderer/viewer/lib/worldrendererCommon.ts (1)
renderer/viewer/lib/ui/newStats.ts (2)
addNewStat(7-36)updateStatText(82-85)
src/loadSave.ts (1)
src/integratedServer/browserfsShared.ts (1)
initialFsState(31-43)
🔇 Additional comments (9)
src/controls.ts (2)
950-950: Avoid leavingdebuggerstatements in production code.According to static analysis, the
debuggerstatement is flagged. Removing or wrapping it behind a dev build check can prevent accidental triggers in production.if (e.code === 'KeyK' && e.altKey && !e.shiftKey && !e.ctrlKey && !e.metaKey) { - debugger + // TODO: Use console.log or dev-tools breakpoints in development only }🧰 Tools
🪛 Biome (1.9.4)
[error] 950-950: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
952-954: Good implementation of renderer toggle featureThe new Alt+J keyboard shortcut for toggling between WebGPU and ThreeJS renderers is a useful addition for testing and comparing the renderers. This aligns well with the PR objective of implementing a new WebGPU renderer.
src/loadSave.ts (3)
13-13: Good refactoring of imports from centralized moduleThe import statement has been updated to bring in utility functions and state from the
browserfsSharedmodule, which helps centralize file system related functionality. This is a good practice for maintainability.
107-107: Consistent use of encapsulated state propertiesUsing
fsState.forceCachedDataPathsinstead of a standalone variable improves code organization by properly encapsulating related state within the fsState object.
146-146: Consistent state property access patternUsing
fsState.forceRedirectPathsfollows the same pattern of accessing state properties through the fsState object, maintaining consistency throughout the codebase.renderer/viewer/lib/worldrendererCommon.ts (4)
159-160: Good addition of chunk download statistics UI element.Adding a dedicated stat for downloaded chunks improves visibility of the rendering process, which aligns well with the WebGPU rendering implementation focus of this PR.
167-167: Increased stats refresh rate from 1000ms to 500ms.Doubling the update frequency for geometry reception statistics will provide more responsive feedback during rendering. This is a good change for the WebGPU renderer where performance monitoring is critical.
528-528: Enhanced statistics display with queue information.The addition of message queue length information (
Q: ${this.messageQueue.length}) provides valuable insight into the rendering pipeline's current workload, which is helpful for debugging and performance monitoring.
579-582: Improved condition for resetting chunk loading metrics.The revised condition for resetting metrics when there are no finished chunks is more precise than the previous implementation that checked if all chunks were not finished.
|
|
||
| // viewer.world.chunksReset() // todo | ||
|
|
||
| // TODO! | ||
| // if (localServer) { | ||
| // //@ts-expect-error not sure why it is private... maybe revisit api? | ||
| // localServer.players[0].world.columns = {} | ||
| // } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider implementing the TODO for viewer.world.chunksReset()
The previous code that cleared localServer.players[0].world.columns has been commented out, with a TODO note for implementing viewer.world.chunksReset(). This change appears to be part of the refactoring for the new WebGPU renderer. Make sure to implement this method to properly handle chunk resets in the new rendering system.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (9)
renderer/viewer/webgpu/worldrendererWebgpu.ts (8)
157-159: Fix missing setTimeout delay parameter.The setTimeout call is missing a delay parameter, which may cause inconsistent behavior across browsers.
await new Promise(resolve => { - setTimeout(resolve,) + setTimeout(resolve, 0) })
180-180: Remove or explain commented code.There's commented-out code that might indicate an unresolved issue or incomplete implementation.
Either remove the commented code or add an explanatory comment about why it's commented out and if it needs to be addressed in the future.
- if (/* !this.loadedChunks[chunkCoords[0] + ',' + chunkCoords[2]] || */ !this.active) return + if (!this.active) return
230-232: Implement the updateShowChunksBorder method.The method contains only a TODO comment without any implementation, which means this functionality is not working.
updateShowChunksBorder (value: boolean) { - // todo + this.webgpuChannel.updateShowChunksBorder(value) }Would you like me to help implement this method or create an issue to track this task?
429-435: Optimize mainLoop in debug UI to use requestAnimationFrame's callback parameter.The current implementation creates a new variable to track updates, but requestAnimationFrame already provides a timestamp parameter that could be used to calculate FPS more accurately.
- let updates = 0 + let lastTime = performance.now(); + let frames = 0; const mainLoop = () => { - requestAnimationFrame(mainLoop) - updates++ + const currentTime = performance.now(); + frames++; + + // Update FPS counter once per second + if (currentTime - lastTime >= 1000) { + updateText2(`Main Loop: ${frames}`); + frames = 0; + lastTime = currentTime; + } + + requestAnimationFrame(mainLoop); } mainLoop() - setInterval(() => { - updateText2(`Main Loop: ${updates}`) - updates = 0 - }, 1000)
36-36: Add type safety to webgpuChannel placeholder.The current implementation uses
anytype which reduces type safety. Consider using a more specific type to ensure type checking during development.- webgpuChannel: typeof workerProxyType['__workerProxy'] = this.getPlaceholderChannel() + webgpuChannel: typeof workerProxyType['__workerProxy'] = this.getPlaceholderChannel<typeof workerProxyType['__workerProxy']>()Then update the
getPlaceholderChannelmethod:- getPlaceholderChannel () { + getPlaceholderChannel<T extends Record<string, (...args: any[]) => any>> (): T { return new Proxy({}, { get: (target, p) => (...args) => { void this.readyWorkerPromise.then(() => { - this.webgpuChannel[p](...args) + (this.webgpuChannel as any)[p as string](...args) }) } - }) as any // placeholder to avoid crashes + }) as T // placeholder to avoid crashes }
80-88: Improve type safety in getPlaceholderChannel method.The method uses
anytype which reduces type safety. Using generics can provide better type checking while maintaining the flexibility needed for this placeholder implementation.- getPlaceholderChannel () { - return new Proxy({}, { - get: (target, p) => (...args) => { + getPlaceholderChannel<T extends Record<string, (...args: any[]) => any>>(): T { + return new Proxy<Record<string, any>>({}, { + get: (target, p: string | symbol) => (...args: any[]) => { void this.readyWorkerPromise.then(() => { - this.webgpuChannel[p](...args) + (this.webgpuChannel as any)[p](...args) }) } - }) as any // placeholder to avoid crashes + }) as T // placeholder to avoid crashes }
101-113: Improve type safety in sendCameraToWorker method.The method uses
anytype which reduces type safety. Using proper type annotations would improve code quality and prevent potential type-related bugs.+ interface CameraVectors { + rotation: { x: number; y: number; z: number }; + position: { x: number; y: number; z: number }; + } sendCameraToWorker () { - const cameraVectors = ['rotation', 'position'].reduce((acc, key) => { - acc[key] = ['x', 'y', 'z'].reduce((acc2, key2) => { - acc2[key2] = this.camera[key][key2] + const cameraVectors = ['rotation', 'position'].reduce<Partial<CameraVectors>>((acc, key) => { + acc[key as keyof CameraVectors] = ['x', 'y', 'z'].reduce<{ x?: number; y?: number; z?: number }>((acc2, key2) => { + acc2[key2 as 'x' | 'y' | 'z'] = this.camera[key as 'rotation' | 'position'][key2 as 'x' | 'y' | 'z'] return acc2 - }, {}) + }, {}) as { x: number; y: number; z: number } return acc - }, {}) as any + }, {}) as CameraVectors this.webgpuChannel.camera({ ...cameraVectors, fov: this.displayOptions.inWorldRenderingConfig.fov }) }
117-121: Optimize event listener in addWebgpuListener method.The current implementation adds a new listener for every type, which is inefficient if multiple listeners for different types are added. Instead, a single listener could be used with type filtering.
export class WorldRendererWebgpu extends WorldRendererCommon { + private typeListeners: Map<string, Array<(data: any) => void>> = new Map(); + private messageHandler: ((e: MessageEvent) => void) | null = null; // ... addWebgpuListener (type: string, listener: (data: any) => void) { void this.readyWorkerPromise.then(() => { - this.worker!.addEventListener('message', (e: any) => { - if (e.data.type === type) { - listener(e.data) - } - }) + if (!this.typeListeners.has(type)) { + this.typeListeners.set(type, []); + } + this.typeListeners.get(type)!.push(listener); + + // Add the global handler only once + if (!this.messageHandler) { + this.messageHandler = (e: MessageEvent) => { + const messageType = e.data?.type; + if (messageType && this.typeListeners.has(messageType)) { + for (const typeListener of this.typeListeners.get(messageType)!) { + typeListener(e.data); + } + } + }; + this.worker!.addEventListener('message', this.messageHandler); + } }) } // Don't forget to clean up in destroy method override destroy () { // ... + if (this.worker && this.messageHandler) { + this.worker.removeEventListener('message', this.messageHandler); + this.messageHandler = null; + this.typeListeners.clear(); + } // ... } }src/index.ts (1)
347-367: Large block of commented-out code should be removedThis extensive block of commented-out code suggests incomplete refactoring. If this functionality has been replaced, the commented code should be removed to improve readability.
If the functionality has been completely replaced by the new implementation, consider removing this commented-out code. If not, add a comment explaining why it's being preserved.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
renderer/viewer/webgpu/graphicsBackendWebgpu.ts(1 hunks)renderer/viewer/webgpu/worldrendererWebgpu.ts(1 hunks)src/appViewer.ts(4 hunks)src/appViewerLoad.ts(3 hunks)src/index.ts(10 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- renderer/viewer/webgpu/graphicsBackendWebgpu.ts
🧰 Additional context used
🧬 Code Definitions (3)
src/index.ts (4)
src/integratedServer/main.ts (2)
destroyLocalServerMain(115-137)startLocalServerMain(55-97)src/globalState.ts (1)
miscUiState(109-129)src/optionsStorage.ts (1)
options(218-223)src/appViewer.ts (1)
appViewer(256-256)
renderer/viewer/webgpu/worldrendererWebgpu.ts (5)
renderer/playground/webgpuRendererShared.ts (2)
defaultWebgpuRendererParams(4-13)rendererParamsGui(15-25)renderer/playground/webgpuRendererWorker.ts (2)
workerProxyType(95-281)canvas(97-113)renderer/viewer/webgpu/graphicsBackendWebgpu.ts (1)
WebgpuInitOptions(5-7)src/appViewer.ts (1)
DisplayWorldOptions(61-68)renderer/playground/webgpuBlockModels.ts (1)
prepareCreateWebgpuBlocksModelsData(9-157)
src/appViewerLoad.ts (2)
src/appViewer.ts (2)
appViewer(256-256)loadBackend(117-156)src/optionsStorage.ts (1)
options(218-223)
🔇 Additional comments (15)
renderer/viewer/webgpu/worldrendererWebgpu.ts (5)
58-63: Add MessagePort cleanup in destroy method.The destroy method terminates the worker if it's a Worker instance but doesn't close the port if it's a MessagePort, which could lead to resource leaks.
Apply this diff to fix the issue:
if (this.worker instanceof Worker) { this.worker.terminate() +} else if (this.worker instanceof MessagePort) { + this.worker.close() }
273-274: Add error handling for fetch operation.The fetch operation could fail for various reasons (network issues, invalid URL, etc.), but there's no error handling.
- const imageBlob = await fetch(image).then(async (res) => res.blob()) + let imageBlob: Blob; + try { + const response = await fetch(image); + if (!response.ok) { + throw new Error(`Failed to fetch image: ${response.status} ${response.statusText}`); + } + imageBlob = await response.blob(); + } catch (error) { + console.error('Failed to fetch texture image:', error); + this.issueReporter.reportProblem(false, `Failed to load textures: ${error.message}`); + throw error; + }
402-404: Implement UI cleanup in addWebgpuDebugUi.The TODO comment indicates that cleanup for the debug UI is not implemented, which could lead to memory leaks.
The debug UI should be properly cleaned up when no longer needed. Consider implementing a cleanup function that:
- Removes event listeners
- Clears timers
- Removes DOM elements
This function should be called when the renderer is destroyed.
407-419: Add cleanup for event listener in addWebgpuDebugUi.The event listener is added but never removed, which could lead to memory leaks if the component is destroyed and recreated.
+ const messageListener = (e: any) => { + if (e.data.type === 'fps') { + updateText(`FPS: ${e.data.fps}`) + if (prevTimeout) clearTimeout(prevTimeout) + prevTimeout = setTimeout(() => { + updateText('<hanging>') + }, 1002) + } + if (e.data.type === 'stats') { + updateTextGpuStats(e.data.stats) + // worldRenderer.rendererDevice = `${e.data.device} WebGL data: ${WorldRendererThree.getRendererInfo(new THREE.WebGLRenderer())}` + } + } + worker.addEventListener('message', messageListener) + + // Clean up on abort + worldRenderer.abortController.signal.addEventListener('abort', () => { + worker.removeEventListener('message', messageListener) + if (prevTimeout) clearTimeout(prevTimeout) + }) - worker.addEventListener('message', (e: any) => { - if (e.data.type === 'fps') { - updateText(`FPS: ${e.data.fps}`) - if (prevTimeout) clearTimeout(prevTimeout) - prevTimeout = setTimeout(() => { - updateText('<hanging>') - }, 1002) - } - if (e.data.type === 'stats') { - updateTextGpuStats(e.data.stats) - // worldRenderer.rendererDevice = `${e.data.device} WebGL data: ${WorldRendererThree.getRendererInfo(new THREE.WebGLRenderer())}` - } - })
454-458: Fix URL search parameter update.The code sets the search parameter to the old value
valueinstead of the new valuenewVal.if (rendererParamsGui[key]?.qsReload) { const searchParams = new URLSearchParams(window.location.search) - searchParams.set(key, String(value)) + searchParams.set(key, String(newVal)) window.location.search = searchParams.toString() }src/appViewerLoad.ts (3)
2-2: Appropriate import for new WebGPU rendererThe import of the WebGPU backend matches the PR objective to implement a new rendering engine using WebGPU.
11-11: Correctly enables WebGPU backend loadingPreviously commented code is now active, properly loading the WebGPU renderer when selected in options.
37-45:❓ Verification inconclusive
Potential undefined variable in renderer change handler
The subscription handler references a global
botvariable that isn't defined or imported in this file. This could lead to runtime errors if the variable isn't available when the handler executes.
🏁 Script executed:
#!/bin/bash # Check if 'bot' is defined as a global variable elsewhere rg -A 2 -B 2 "window.bot\s*=|global.bot\s*=" --glob="*.{js,ts,jsx,tsx}"Length of output: 794
Attention: Global 'bot' Initialization Clarification Needed
The subscription handler in
src/appViewerLoad.tsreferencesbotwithout a local declaration. Our search shows thatbotis defined as a global variable in bothsrc/index.tsandsrc/globals.js(via assignments towindow.bot). Although this confirms that a globalbotis present, relying on an implicit global can be fragile if the initialization order isn’t strictly guaranteed.To strengthen the code:
- Explicit Global Reference: Consider referencing
window.botinstead ofbotdirectly to make the dependency clearer.- Documentation/Declaration: Add a global declaration or an inline comment that documents the source and expected initialization order of
bot.- Initialization Order Review: Verify that the code initializing
botalways runs before the subscription handler insrc/appViewerLoad.tsfires.src/index.ts (4)
94-94: Appropriate imports for server managementThe imports correctly include the new server management functions that replace the previous implementation.
200-200: Improved server termination with proper cleanupUsing
destroyLocalServerMain(false)is more robust than simply setting variables to undefined as it properly terminates the worker and cleans up resources.
346-346: Improved server initialization with better encapsulationObtaining the CustomClient from
startLocalServerMainpromotes better separation of concerns compared to the previous approach.
706-706: Improved encapsulation with new startWithBot methodUsing the new
appViewer.startWithBot()method encapsulates the renderer initialization logic, which is a good practice for maintainability.src/appViewer.ts (3)
194-194: Improved code organization in resetBackendThe change simplifies the method by delegating the cleanup logic to
disconnectBackend, which is good for code maintainability.
221-226: Good consolidation of cleanup logicUpdating the
disconnectBackendmethod to handle the state cleanup based on thecleanStateparameter consolidates related functionality in one place, which is a good practice.
235-235: Simplified renderer state initializationThe direct assignment using
proxy(getDefaultRendererState())is cleaner than the previous approach and ensures a fresh renderer state.
| const mainLoop = () => { | ||
| requestAnimationFrame(mainLoop) | ||
| updates++ | ||
| } | ||
| mainLoop() | ||
| setInterval(() => { | ||
| updateText2(`Main Loop: ${updates}`) | ||
| updates = 0 | ||
| }, 1000) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clean up interval timer in addWebgpuDebugUi.
The setInterval is created but never cleared, which could lead to memory leaks if the component is destroyed and recreated.
- setInterval(() => {
+ const statsInterval = setInterval(() => {
updateText2(`Main Loop: ${updates}`)
updates = 0
}, 1000)
+
+ // Clean up on abort
+ worldRenderer.abortController.signal.addEventListener('abort', () => {
+ clearInterval(statsInterval);
+ })📝 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.
| const mainLoop = () => { | |
| requestAnimationFrame(mainLoop) | |
| updates++ | |
| } | |
| mainLoop() | |
| setInterval(() => { | |
| updateText2(`Main Loop: ${updates}`) | |
| updates = 0 | |
| }, 1000) | |
| const mainLoop = () => { | |
| requestAnimationFrame(mainLoop) | |
| updates++ | |
| } | |
| mainLoop() | |
| const statsInterval = setInterval(() => { | |
| updateText2(`Main Loop: ${updates}`) | |
| updates = 0 | |
| }, 1000) | |
| // Clean up on abort | |
| worldRenderer.abortController.signal.addEventListener('abort', () => { | |
| clearInterval(statsInterval); | |
| }) |
| addWebgpuListener (type: string, listener: (data: any) => void) { | ||
| void this.readyWorkerPromise.then(() => { | ||
| this.worker!.addEventListener('message', (e: any) => { | ||
| if (e.data.type === type) { | ||
| listener(e.data) | ||
| } | ||
| }) | ||
| }) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve event listener management to prevent memory leaks.
The addWebgpuListener method adds event listeners without providing a way to remove them when the renderer is destroyed, which could lead to memory leaks if many listeners are added.
Create a storage for the listeners and clean them up in the destroy method:
export class WorldRendererWebgpu extends WorldRendererCommon {
// Add a new property to store listeners
+ private messageListeners: Array<{ type: string, handler: (e: any) => void }> = [];
// Modify the addWebgpuListener method
addWebgpuListener (type: string, listener: (data: any) => void) {
void this.readyWorkerPromise.then(() => {
- this.worker!.addEventListener('message', (e: any) => {
+ const handler = (e: any) => {
if (e.data.type === type) {
listener(e.data)
}
- })
+ };
+ this.worker!.addEventListener('message', handler);
+ this.messageListeners.push({ type, handler });
})
}
// Modify the destroy method to clean up listeners
override destroy () {
// Existing code...
+ // Clean up message listeners
+ if (this.worker) {
+ for (const { handler } of this.messageListeners) {
+ this.worker.removeEventListener('message', handler);
+ }
+ this.messageListeners = [];
+ }
// Rest of destroy method...
}
}📝 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.
| addWebgpuListener (type: string, listener: (data: any) => void) { | |
| void this.readyWorkerPromise.then(() => { | |
| this.worker!.addEventListener('message', (e: any) => { | |
| if (e.data.type === type) { | |
| listener(e.data) | |
| } | |
| }) | |
| }) | |
| } | |
| export class WorldRendererWebgpu extends WorldRendererCommon { | |
| // Existing properties... | |
| // Add a new property to store listeners | |
| private messageListeners: Array<{ type: string, handler: (e: any) => void }> = []; | |
| // Other class members... | |
| addWebgpuListener(type: string, listener: (data: any) => void) { | |
| void this.readyWorkerPromise.then(() => { | |
| const handler = (e: any) => { | |
| if (e.data.type === type) { | |
| listener(e.data); | |
| } | |
| }; | |
| this.worker!.addEventListener('message', handler); | |
| this.messageListeners.push({ type, handler }); | |
| }); | |
| } | |
| override destroy() { | |
| // Existing cleanup code... | |
| // Clean up message listeners | |
| if (this.worker) { | |
| for (const { handler } of this.messageListeners) { | |
| this.worker.removeEventListener('message', handler); | |
| } | |
| this.messageListeners = []; | |
| } | |
| // Rest of destroy method... | |
| } | |
| // Other class methods... | |
| } |
| const blob = await fetch(this.resourcesManager.currentResources!.blocksAtlasParser.latestImage).then(async (res) => res.blob()) | ||
| this.webgpuChannel.updateTexture(blob) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for texture update fetch operation.
Similar to the issue at line 273, this fetch operation lacks error handling which could lead to unhandled promise rejections.
- const blob = await fetch(this.resourcesManager.currentResources!.blocksAtlasParser.latestImage).then(async (res) => res.blob())
- this.webgpuChannel.updateTexture(blob)
+ try {
+ const response = await fetch(this.resourcesManager.currentResources!.blocksAtlasParser.latestImage);
+ if (!response.ok) {
+ throw new Error(`Failed to fetch texture: ${response.status} ${response.statusText}`);
+ }
+ const blob = await response.blob();
+ this.webgpuChannel.updateTexture(blob);
+ } catch (error) {
+ console.error('Failed to update texture:', error);
+ this.issueReporter.reportProblem(false, `Failed to update textures: ${error.message}`);
+ }📝 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.
| const blob = await fetch(this.resourcesManager.currentResources!.blocksAtlasParser.latestImage).then(async (res) => res.blob()) | |
| this.webgpuChannel.updateTexture(blob) | |
| } | |
| try { | |
| const response = await fetch(this.resourcesManager.currentResources!.blocksAtlasParser.latestImage); | |
| if (!response.ok) { | |
| throw new Error(`Failed to fetch texture: ${response.status} ${response.statusText}`); | |
| } | |
| const blob = await response.blob(); | |
| this.webgpuChannel.updateTexture(blob); | |
| } catch (error) { | |
| console.error('Failed to update texture:', error); | |
| this.issueReporter.reportProblem(false, `Failed to update textures: ${error.message}`); | |
| } | |
| } |
| subscribeKey(options, 'activeRenderer', () => { | ||
| if (appViewer.currentDisplay === 'world' && bot) { | ||
| appViewer.resetBackend(true) | ||
| loadBackend() | ||
| void appViewer.startWithBot() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for asynchronous operation
The code calls appViewer.startWithBot() with void which swallows any errors that might occur during this asynchronous operation.
Consider adding proper error handling:
- void appViewer.startWithBot()
+ try {
+ void appViewer.startWithBot()
+ } catch (error) {
+ console.error('Error restarting with new renderer:', error)
+ }Committable suggestion skipped: line range outside the PR's diff.
| if (singleplayer && !fsState.inMemorySave) { | ||
| possiblyCleanHandle(() => { | ||
| // todo: this is not enough, we need to wait for all async operations to finish | ||
| }) | ||
| // possiblyCleanHandle(() => { | ||
| // // todo: this is not enough, we need to wait for all async operations to finish | ||
| // }) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Commented out cleanup code may lead to resource leaks
The previous cleanup logic using possiblyCleanHandle has been commented out without a clear replacement. This could potentially lead to resource leaks.
🏁 Script executed:
#!/bin/bash
# Check what possiblyCleanHandle was doing and if its functionality is replaced
rg -A 10 "possiblyCleanHandle\s*=" --glob="*.{js,ts,jsx,tsx}"Length of output: 788
Critical: Address the removal of cleanup logic in src/index.ts
The previously active cleanup via possiblyCleanHandle is now commented out (lines 217–220 in src/index.ts), and there’s no clear replacement. Since possiblyCleanHandle (defined in src/integratedServer/browserfsShared.ts) handles asynchronous cleanup via browserfs.configure, eliminating its invocation risks leaving async operations unfinished and may create resource leaks.
- Action items:
- Reinstate and adjust cleanup: Re-enable
possiblyCleanHandlewith proper synchronization to ensure all async operations complete. - Or remove safely: If the cleanup logic is deemed unnecessary, remove the commented-out code entirely and update documentation accordingly to reflect the change.
- Reinstate and adjust cleanup: Re-enable
| async startWithBot () { | ||
| const renderDistance = miscUiState.singleplayer ? options.renderDistance : options.multiplayerRenderDistance | ||
| await this.startWorld(bot.world, renderDistance) | ||
| this.worldView!.listenToBot(bot) | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
New method lacks error handling and uses undeclared global variable
The new startWithBot method references a global bot variable that isn't imported in this file. Additionally, it lacks error handling for potential issues during world initialization.
Consider adding proper variable declaration and error handling:
async startWithBot () {
+ if (!window.bot) {
+ throw new Error('Bot is not initialized')
+ }
const renderDistance = miscUiState.singleplayer ? options.renderDistance : options.multiplayerRenderDistance
- await this.startWorld(bot.world, renderDistance)
- this.worldView!.listenToBot(bot)
+ try {
+ await this.startWorld(window.bot.world, renderDistance)
+ this.worldView!.listenToBot(window.bot)
+ } catch (error) {
+ console.error('Error starting world with bot:', error)
+ throw error
+ }
}🏁 Script executed:
#!/bin/bash
# Check how bot is defined and accessed elsewhere in the codebase
rg -A 2 -B 2 "window.bot\s*=|global.bot\s*=" --glob="*.{js,ts,jsx,tsx}"Length of output: 794
Attention: Global Variable Reference and Error Handling in startWithBot
After verifying the codebase, it’s clear that the bot instance is defined on the global scope as window.bot (see references in src/index.ts and src/globals.js), so direct usage of bot in this method is incorrect. Additionally, the lack of error handling around the asynchronous call to startWorld may lead to unhandled exceptions if issues occur during initialization.
Please update the startWithBot method to:
- Explicitly reference
window.botinstead of the undeclaredbot. - Add an initialization check (e.g., verify that
window.botexists). - Wrap the asynchronous calls in a try/catch block to capture and log any errors.
Below is a suggested diff:
async startWithBot () {
+ if (!window.bot) {
+ throw new Error('Bot is not initialized.');
+ }
const renderDistance = miscUiState.singleplayer ? options.renderDistance : options.multiplayerRenderDistance
- await this.startWorld(bot.world, renderDistance)
- this.worldView!.listenToBot(bot)
+ try {
+ await this.startWorld(window.bot.world, renderDistance)
+ this.worldView!.listenToBot(window.bot)
+ } catch (error) {
+ console.error('Error starting world with bot:', error)
+ throw error
+ }
}📝 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.
| async startWithBot () { | |
| const renderDistance = miscUiState.singleplayer ? options.renderDistance : options.multiplayerRenderDistance | |
| await this.startWorld(bot.world, renderDistance) | |
| this.worldView!.listenToBot(bot) | |
| } | |
| async startWithBot () { | |
| if (!window.bot) { | |
| throw new Error('Bot is not initialized.'); | |
| } | |
| const renderDistance = miscUiState.singleplayer ? options.renderDistance : options.multiplayerRenderDistance | |
| try { | |
| await this.startWorld(window.bot.world, renderDistance) | |
| this.worldView!.listenToBot(window.bot) | |
| } catch (error) { | |
| console.error('Error starting world with bot:', error) | |
| throw error | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
renderer/viewer/lib/worldrendererCommon.ts (3)
469-485: Unify out-of-range time checks with error throwing logic.
Here, the code gracefully handlestimeOfDay < 0 || timeOfDay > 24000, while line 720 explicitly throws an error for the same condition. Consider consolidating into a single approach (error or fallback) to prevent confusion.
542-542: Minor formatting nitpick in stat text.
String formatting includes both “ms” and “ss” units, which might be slightly unclear. Consider clarifying or unifying the display for better readability.
593-593: Consolidate repeated logic for settingallChunksFinished.
This line reassignsthis.allChunksFinished, whilecheckAllFinished()and other lines (e.g. line 703) also manage it. Centralizing the logic incheckAllFinished()(or a single function) can reduce confusion and maintain consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
renderer/viewer/lib/mesher/mesher.ts(5 hunks)renderer/viewer/lib/worldrendererCommon.ts(7 hunks)renderer/viewer/webgpu/graphicsBackendWebgpu.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- renderer/viewer/lib/mesher/mesher.ts
- renderer/viewer/webgpu/graphicsBackendWebgpu.ts
🧰 Additional context used
🧬 Code Definitions (1)
renderer/viewer/lib/worldrendererCommon.ts (1)
renderer/viewer/lib/ui/newStats.ts (2)
addNewStat(7-36)updateStatText(82-85)
🔇 Additional comments (5)
renderer/viewer/lib/worldrendererCommon.ts (5)
19-19: Adopting new UI stat utilities looks good.
No issues found with the expanded import ofaddNewStat,removeAllStats,removeStat, andupdateStatText. This aligns with the overall stats-tracking approach in the codebase.
56-56: Initialization of day time indicator.
DefiningtimeOfTheDay = 0is a straightforward and clear default.
160-161: Stat creation for downloaded chunks.
UsingaddNewStatto visualize the "downloaded-chunks" metric is consistent with your existing stat-based UI.
164-168: Potential memory leak from setInterval.
AsetIntervalis created but never cleared, which may cause a memory leak if multiple renderer instances are constructed. This issue was previously flagged in an older review. Consider storing the interval ID as a class property and callingclearIntervalindestroy()to avoid leaks.Example fix:
+ private geometryStatsInterval?: number constructor(...) { ... - setInterval(() => { + this.geometryStatsInterval = setInterval(() => { this.geometryReceiveCountPerSec = Object.values(this.geometryReceiveCount).reduce((acc, curr) => acc + curr, 0) this.geometryReceiveCount = {} this.updateChunksStats() }, 500) } destroy() { ... + if (this.geometryStatsInterval) clearInterval(this.geometryStatsInterval) }
725-725: Verify redundant out-of-range checks.
Because line 720 already throws an error iftimeOfDayis out of range, ensure you truly need this additional assignment logic for negative or beyond 24000 values.Would you like to confirm whether any code path sets
timeOfTheDayto an invalid range, or should we remove the no-op block ingetMesherConfig()?
zardoy
changed the title
|
@coderabbitai pause |
✅ Actions performedReviews paused. |
|
Deployed to Vercel Preview: https://prismarine-axi4apbgu-zaro.vercel.app |
|
Deployed to Vercel Preview: https://prismarine-p1admyy31-zaro.vercel.app |
|
Is their a download fo the modified minecraft web client of this site: https://prismarine-pho51nlw6-zaro.vercel.app/ |
You can do it yourself by checking out the branch and running build-single-file npm script. I might add it deploy workflow as well so you can just download a single html file |
|
Deployed to Vercel Preview: https://prismarine-7s5ojgyk2-zaro.vercel.app |
zardoy
changed the title
3 participants
WORKING PLAYGROUND DEMO - PERF CHUNKS
LATEST MAP DEMO (Future City)
tasks (branch is archived now):
@zardoy
Summary by CodeRabbit
New Features
Controlscomponent for better touch interaction.TextureAnimationclass for managing texture animations.MessageChannelReplacementclass for improved message handling.Enhancements
Bug Fixes
These updates deliver improved graphics, responsiveness, and a modernized user interface for a superior viewing experience.