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: 40
🔭 Outside diff range comments (5)
src/packetsReplay/replayPackets.ts (1)
59-63: 🛠️ Refactor suggestion
⚠️ Potential issueResolve dangling reference to LocalServer.
ThecreateServercall is still passingLocalServer as any(line 60) even though the import has been commented out (and presumably theLocalServerclass has been removed). Update the server creation logic to remove or replace this reference to avoid runtime issues. For example, if theServerproperty is now obsolete or should be assigned a different value, update accordingly.Here’s a possible diff if the property should simply be removed:
- const server = createServer({ - Server: LocalServer as any, - version: header.minecraftVersion, - 'online-mode': false - }) + const server = createServer({ + version: header.minecraftVersion, + 'online-mode': false + })src/react/ButtonWithTooltip.tsx (1)
29-40: 🛠️ Refactor suggestionPotential issue with tooltip timeout behavior
The current implementation may hide tooltips after timeout even when
alwaysTooltipis specified, due to the hide logic being tied only tolocalStorageKey.- if (showTooltips && localStorageKey) { + if (showTooltips && localStorageKey && !alwaysTooltip) { // todo wait for user interaction mouseoe timeout = setTimeout(() => { hide() }, 10_000) }function hide () { - if (!localStorageKey) return + if (!localStorageKey || alwaysTooltip) return localStorage[localStorageKey] = 'false' setShowTooltips(false) }renderer/viewer/lib/mesher/modelsGeometryCommon.ts (1)
141-153:⚠️ Potential issueFix inconsistent side value for south face
The
southface hasside: 0which is the same as theupface. This inconsistency could lead to rendering issues.south: { - side: 0, + side: 5, webgpuSide: 2, dir: [0, 0, 1], mask1: [1, 0, 1], mask2: [0, 1, 1], corners: [ [0, 0, 1, 0, 1], [1, 0, 1, 1, 1], [0, 1, 1, 0, 0], [1, 1, 1, 1, 0] ] }renderer/playground/TextureAnimation.ts (1)
15-70:⚠️ Potential issueHandle undefined
this.data.framesbefore mapping.
At line 50, callingthis.data.frames.map(...)can throw a runtime error ifthis.data.framesisundefined. Perform a null check or default to an empty array prior to mapping.let frames = this.data.frames ? this.data.frames.map(frame => typeof frame === 'number' ? { index: frame, time: this.data.frametime } : frame ) : [] if (frames.length) { const frame = frames[this.frameIndex] ...renderer/viewer/three/worldrendererThree.ts (1)
607-609: 💡 Verification agent🧩 Analysis chain
Missing cleanup in the destroy method
The
destroymethod has been modified to remove calls toremoveAllStats()andthis.media.onWorldGone(), but doesn't seem to clean up other resources that might need to be freed.Ensure that all resources are properly cleaned up in the
destroymethod:
🏁 Script executed:
#!/bin/bash # Verify if the cleanup of resources is appropriate and consistent with other classes echo "Checking for resource cleanup patterns in related classes..." # Check for similar destroy patterns in other renderer classes rg -A 10 'destroy\s*\(\s*\).*{' --type ts | grep -v 'super\.destroy' # Look for potential resource cleanup patterns that might be needed rg "\b(dispose|clean|free|release)\b" --type ts | grep -A 2 -B 2 "renderer"Length of output: 5064
Attention: Incomplete Resource Cleanup in destroy() Method
The current implementation in
renderer/viewer/three/worldrendererThree.ts—wheredestroy()only callssuper.destroy()—appears to be missing cleanup calls that are otherwise common in similar renderer classes. Previously, cleanup functions such asremoveAllStats()andthis.media.onWorldGone()were present, and a review of disposal patterns (e.g., calls to.dispose()for textures, geometry, and materials in related files) shows that other parts of the code actively release resources.
- Confirm that resources allocated in this class (like
oldTexture,oldItemsTexture, textures referenced in thetexturesarray, andthis.pointsrelated objects) are either cleaned up elsewhere or need explicit disposal in this method.- If the cleanup is intended to rely solely on the base class implementation, please add a clarifying comment; otherwise, incorporate the necessary disposal calls directly in the
destroy()method.
🧹 Nitpick comments (124)
renderer/viewer/lib/workerProxy.ts (2)
3-3: Refine the event parameter type for stronger type safety.The event is currently typed as
any. Declaring it asMessageEventprovides improved type checking and clarity in code.- target.addEventListener('message', (event: any) => { + target.addEventListener('message', (event: MessageEvent) => {
48-48: Consider replacingas any[]withas Transferable[]for stricter type checking.Casting to
any[]forfeits type safety and can obscure potential issues around transferring objects. Using a more explicit type can help prevent unexpected errors.- }, transfer as any[]) + }, transfer as Transferable[])renderer/package.json (1)
17-20: Duplicatelive-serverDependency Declaration
The package"live-server": "^1.2.2"appears in both thedependencies(line 19) anddevDependencies(line 44). Typically, if the package is only used for development (e.g. serving the app locally), it should reside solely indevDependenciesto avoid bloating production installations. Please review and consolidate these entries accordingly.renderer/playground.html (1)
37-39: Remove Redundant Universal Selector
A similar universal selector is declared again at lines 37-39, which duplicates the rules already defined in lines 20-23. To keep the CSS DRY and maintainable, consider removing the redundant block (or merging differences if vendor prefixes are needed).renderer/playground/messageChannel.ts (2)
1-28: Good implementation of MessageChannel replacement with minor style issuesThis class creates a simplified implementation of the standard
MessageChannelAPI for in-thread communication. It correctly simulates the core functionality with port listeners and message passing.Fix the styling issues with space before function parentheses:
- start() {} + start () {}- start() {} + start () {}🧰 Tools
🪛 ESLint
[error] 14-14: Missing space before function parentheses.
(@stylistic/space-before-function-paren)
[error] 26-26: Missing space before function parentheses.
(@stylistic/space-before-function-paren)
15-15: Consider adding proper typings instead of using 'any'The type assertions to
anyfor both port objects bypass TypeScript's type checking, which could lead to undetected errors.Consider creating an explicit interface that matches the subset of MessagePort API you're implementing:
interface SimpleMessagePort { addEventListener(type: string, listener: (e: MessageEvent) => void): void; postMessage(data: any): void; start(): void; }Then use it instead of
any:- } as any + } as SimpleMessagePortAlso applies to: 27-27
src/globals.d.ts (1)
36-47: Correct shader file type declarations with misnamed constantsThe module declarations for shader files are correctly implemented, but the constant name
pngis misleading for shader files.declare module '*.frag' { - const png: string + const content: string export default png } declare module '*.vert' { - const png: string + const content: string export default png } declare module '*.wgsl' { - const png: string + const content: string export default png }Also update the exports to use the new constant name:
- export default png + export default contentrenderer/viewer/common/webglData.ts (2)
7-7: Address the TODO comment.The
// todoremark indicates that this function is incomplete or pending further development. Consider adding a clear description of the task to be completed or implementing a placeholder logic to ensure no disruptions down the line.Do you want help implementing the missing functionality for texture data handling?
8-26: Clarify partial entries in the returned object.After filtering out missing or invalid textures, the returned object may have fewer entries than
atlas.textures. If that's intended, consider documenting or logging the exclusions for debugging.renderer/rsbuildSharedConfig.ts (1)
8-9: Consider typing forreleaseTagandreleaseChangelog.These variables are declared without explicit types, which might impede clarity and maintainability. If these values are known to always be strings, consider:
-let releaseTag -let releaseChangelog +let releaseTag: string | undefined +let releaseChangelog: string | undefinedbuildWorkers.mjs (2)
45-55: Consider parallel file writes.Writing output files to multiple directories synchronously may have performance impacts. If build times become an issue, consider an asynchronous approach with batching, or confirm that the overhead is acceptable.
123-277: Ensure consistent approach for integrated server build.The integrated server build shares a similar bundling approach but includes custom JSON resolution logic and a banner that sets
globalThis.global = globalThis. Confirm that the node environment emulation is sufficient for all needed features and that no hidden breaks occur in a browser environment.Would you like help verifying that the entire integrated server worker logic runs correctly in the browser under typical usage scenarios?
renderer/viewer/lib/worldDataEmitter.ts (1)
45-47: Consider grouping the new configuration properties into a dedicated object.
Currently, these properties are scattered as individual class fields. For better maintainability and readability, consider creating a single configuration object or constructor-based injection to clearly define their usage and default values.renderer/playground/workerProxy.ts (2)
23-52: Improve error and concurrency handling inuseWorkerProxy.
While the proxy-based approach is flexible, it lacks mechanisms for handling unexpected errors or queuing messages when the worker is busy. Consider adding error handling or a queue to manage concurrent calls gracefully.
54-61: Remove or refactor commented-out usage examples.
Maintaining commented-out code can clutter the repository. Move these examples to documentation or a separate code sample if they are needed for reference.src/resourcesManager.ts (2)
53-53: Remove or revive commented property.
IfnoBlockstatesModelsis no longer required, remove it; otherwise, uncomment or document why it remains commented.
170-205: Consider decoupling time-based texture logic.
Embedding holiday logic (christmasPack) directly inside atlas recreation can quickly become cumbersome. Extract these seasonal overrides to a separate function or configuration for maintainability and clarity.renderer/playground/shared.ts (1)
6-7: Clarify the rationale for commenting outtextureIndexandmodelId.These properties were previously part of
BlockFaceTypebut are now commented out. If they are obsolete, consider removing them entirely. Otherwise, add a comment explaining why they are disabled and if/when they may be reintroduced.renderer/playground/webgpuRendererWorker.ts (5)
10-11: Consider scoping global references likechunksStorage.Attaching
chunksStoragetoglobalThiscan introduce difficulty in testing, mocking, or debugging. If feasible, encapsulatechunksStoragein a dedicated module with controlled access.
16-17: Be cautious with globalcamerausage.Setting
globalThis.cameracan lead to naming collisions and complicates refactoring. Prefer exportingcamerafrom a module or passing it in as a parameter where needed.
29-38: Clear the FPS tracking interval on teardown to avoid potential memory leaks.This interval never appears to be cleared, so if you re-initialize the worker or the renderer multiple times, intervals may accumulate. Consider clearing it in
destroy()or when the renderer is reset.
40-46: Similarly clear the stats interval.Like the FPS interval, ensure the stats interval is also cleared to avoid resource leaks and unintended repeated logic.
211-237: Refine the camera update logic.
- Using
camera.rotation.set(..., 'ZYX')is unusual; confirm that your rotation order matches the rest of the engine’s logic.- The code in lines 228–233 updates
lastCameraUpdateDiff, but be sure this data is actually consumed.package.json (1)
58-58: Verify new or updated dependencies.
@rsbuild/plugin-basic-ssl(line 58) may introduce HTTPS overhead. Confirm it’s intended.@webgpu/types(line 64) is for the experimental WebGPU. Confirm version compatibility.flying-squidis pinned to a GitHub branch (line 82). This can cause potential breakage if the branch changes or is deleted.twgl.js(line 117) is added; confirm it doesn't conflict with existing 3D or GPU libs.mc-assetsversion bump (line 158) could have minor changes; ensure no regressions.Also applies to: 64-64, 82-82, 117-117, 158-158
src/shims/empty.ts (1)
2-2:❓ Verification inconclusive
Added default export alongside named export
Adding a default export to this module provides more flexibility for importing it elsewhere in the codebase. However, having both an empty named export and an empty default export in the same file is unusual.
Consider documenting the purpose of these exports with a comment to clarify their intended use, especially since empty exports can be confusing to other developers.
export { } + // This file ensures TypeScript treats this as a module while providing both named and default exports export default {}
🏁 Script executed:
#!/bin/bash # Search for imports of this module to understand how it's being used rg -A 2 "from ['\"].*src/shims/empty['\"]" --type ts --type tsxLength of output: 97
Below is an updated verification step. The original search command returned an error because the file type "tsx" wasn't recognized. To address this, please run the revised shell script that explicitly defines TSX files:
Once you've confirmed how (or if) this module is imported, please verify whether documenting its dual exports is required for clarity. If you decide to document it, consider adding a comment clarifying why both an empty named export and a default export are present. For example, you might update the file as follows:
export { } // This file ensures TypeScript treats this as a module while providing both named and default exports export default {}Action required: Verify the module’s usage with the above command and, based on the findings, add documentation as needed.
🏁 Script executed:
#!/bin/bash # Search for imports of src/shims/empty module in both TS and TSX files by defining the TSX file type. rg -A 2 "from ['\"].*src/shims/empty['\"]" --type-add 'tsx:*.tsx' --type ts --type tsxLength of output: 214
Review Note: Verify the Use of Dual Exports in
src/shims/empty.tsThe file now provides both a default and an empty named export. While this approach can ensure that TypeScript treats the file as a module and offer flexible import options, using both export types together is uncommon. There was difficulty confirming whether this module is actively imported elsewhere (our recent search returned no output). Before finalizing this pattern, please:
- Manually verify if
src/shims/empty.tsis imported in any part of the codebase.- If it is used, consider adding an inline comment to explain why both exports are present. For example:
export { } + // This file is deliberately set up with both an empty named export and a default export + // to ensure TypeScript treats it as a module and to provide flexible import options. export default {}- If the module isn’t used, re-evaluate the necessity of maintaining its dual exports.
renderer/viewer/three/graphicsBackend.ts (1)
120-120: Consider the redundancy of setting the ID propertyThis line sets
id = 'threejs'directly on the function, but the backend object returned by this function already has the same ID property set at line 94. This redundancy might be intentional if the code needs to identify the backend type before instantiation, but it could be confusing for maintainers.Consider adding a comment explaining why the ID needs to be set both on the function and the returned object, or remove one of them if redundant.
renderer/viewer/three/holdingBlock.ts (1)
914-914: Consider avoiding the use of 'any' type castUsing
as anybypasses TypeScript's type checking system, which can lead to potential runtime errors that would have been caught during compilation. While it might be necessary in some cases, especially during refactoring or when dealing with third-party libraries, it's generally better to use more specific types.Consider using a more specific type or interface that matches the expected structure of the transformed model. If this is a temporary solution during the WebGPU implementation, add a TODO comment explaining the rationale and when it might be addressed.
- }) as any + }) as TransformedBlockModel // Replace with the actual typeIf the exact type is unknown or complex, you could create a temporary type/interface that captures the essential properties needed by the downstream code.
src/react/AppStatusProvider.tsx (1)
83-104: Good enhancement: Robust element detection for animationAdding a MutationObserver ensures the animation works even when the target element isn't immediately available in the DOM. This is a good defensive programming approach.
I appreciate the proper cleanup implementation that disconnects the observer when the component unmounts or after finding the element, preventing memory leaks and unnecessary processing.
For optimal performance, consider limiting the mutation observer scope to a more specific parent element if possible, rather than observing the entire document body:
- observer.observe(document.body, { + observer.observe(document.getElementById('your-parent-container-id') || document.body, {This would reduce the number of mutations the observer needs to process, especially in a complex application.
src/localServerMultiplayer.ts (1)
71-71:❓ Verification inconclusive
Consider completing the refactoring pattern
Line 71 still directly accesses
localServer._server, while other server option accesses have been refactored to use the centralized function.- localServer._server.emit('connection', client) + // Potentially refactor to use a getter function for server instance + // If getLocalServerInstance() function exists or can be created + getLocalServerInstance()._server.emit('connection', client)
🏁 Script executed:
#!/bin/bash # Check if there's an existing getter for the server instance rg -A 2 "getLocalServer(?!Options)" src/Length of output: 285
Consistent Access to Server Instance
- The code on line 71 still directly accesses
localServer._servervia an immediate emit call.- A preliminary search using regex (with PCRE2 enabled) and a plain string search did not reveal an existing getter function (e.g.
getLocalServerInstance) anywhere in thesrc/directory.- To maintain consistency with other parts of the codebase that use abstraction for server instance access, please verify whether a getter function should exist:
- If a standardized getter is part of the refactoring pattern, create or integrate it (e.g., implement
getLocalServerInstance()), and update the call accordingly.- Otherwise, if no such abstraction is planned, consider whether the current direct access is acceptable.
- localServer._server.emit('connection', client) + // Consider refactoring to use a centralized getter for the server instance. + // For example, if a getLocalServerInstance() function is available or can be implemented: + // getLocalServerInstance()._server.emit('connection', client)renderer/viewer/lib/basePlayerState.ts (2)
37-37: New waterBreathing state propertyThe addition of the
waterBreathingproperty enhances the player state model, likely to implement underwater breathing mechanics.Consider adding documentation to clarify the purpose of this property and how it differs from the existing
inWaterproperty.
49-50: Proper initialization of new property and ref optimizationThe
waterBreathingproperty is correctly initialized tofalseand usingreffor thebackgroundColorarray is an appropriate optimization for in-place array updates.Consider applying the
ref()optimization to other arrays or objects in the state that might be updated in-place but not fully replaced, such as position vectors.src/integratedServer/workerMcData.mjs (1)
1-8: Good organization and helper function implementationThe TypeScript checking directive and helper function for version manipulation are well-implemented. The
toMajorVersionfunction provides a clean way to extract major version numbers.The
toMajorVersionfunction could be more robust by adding validation for the input format:const toMajorVersion = version => { + if (!version) return '1.0' // Default fallback version const [a, b] = (String(version)).split('.') + // Ensure we have valid version components + if (!a || !b) return '1.0' // Default fallback version return `${a}.${b}` }src/integratedServer/customServer.ts (1)
5-23: Check redundant EventEmitter reference and empty close method.The implementation effectively creates a custom server that emits a connection event with a new client instance. However, there are two potential issues:
- The code uses
EventEmitter.EventEmitteron line 7, but you've already importedEventEmitterdirectly, so just usingextends EventEmitterwould be cleaner.- The
close()method is empty, which could potentially lead to resource leaks if the server needs cleanup when closed.-export const createCustomServerImpl = (...args: Parameters<typeof createLocalServerClientImpl>) => { +export const createCustomServerImpl = (...args: Parameters<typeof createLocalServerClientImpl>) => { const CustomChannelClient = createLocalServerClientImpl(...args) - return class LocalServer extends EventEmitter.EventEmitter { + return class LocalServer extends EventEmitter { socketServer = null cipher = null decipher = null clients = {} constructor (public version, public customPackets, public hideErrors = false) { super() } listen () { this.emit('connection', new CustomChannelClient(true, this.version)) } - close () { } + close () { + // TODO: Properly clean up resources if needed + this.emit('close') + } } }src/watchOptions.ts (1)
6-6: Unused imports detected.You've added imports for
WorldRendererWebgpuandupdateLocalServerSettingsthat aren't used in this file. This could indicate dead code or incomplete implementation.Either remove the unused imports or add the missing implementation that uses them:
-import { WorldRendererWebgpu } from 'renderer/viewer/webgpu/worldrendererWebgpu' -import { updateLocalServerSettings } from './integratedServer/main'Also applies to: 11-11
cypress/e2e/performance.spec.ts (1)
9-9: Consider extracting timeout value to a constant.The 20,000ms timeout is used twice in this file. For better maintainability, consider extracting it to a named constant.
+const WORLD_LOAD_TIMEOUT = 20_000 -cy.get('.initial-loader', { timeout: 20_000 }).should('not.exist') +cy.get('.initial-loader', { timeout: WORLD_LOAD_TIMEOUT }).should('not.exist')renderer/playground/scenes/floorStoneWorld.ts (3)
19-24: Consider removing or documenting commented code.The commented code appears to be an alternative implementation using
webgpuChannel.generateRandom. Either remove it if it's no longer needed or add a comment explaining why it's kept and when it might be used.
26-35: Add comments explaining the block filtering logic.The filtering logic for
fullBlockschecks for specific shape dimensions but lacks comments explaining what these dimensions represent or why they're being filtered this way. Consider adding explanatory comments.const fullBlocks = loadedData.blocksArray.filter(block => { const b = this.Block.fromStateId(block.defaultState, 0) + // Only use blocks with a single shape that fills an entire block space (1x1x1 cube) if (b.shapes?.length !== 1) return false const shape = b.shapes[0] return shape[0] === 0 && shape[1] === 0 && shape[2] === 0 && shape[3] === 1 && shape[4] === 1 && shape[5] === 1 })
37-42: Consider more descriptive condition for block placement.The variable name
isEvendoesn't accurately describe the conditionx === z. A more precise name would improve readability.for (let x = -squareSize; x <= squareSize; x++) { for (let z = -squareSize; z <= squareSize; z++) { - const isEven = x === z + const isOnDiagonal = x === z - worldView!.world.setBlockStateId(this.targetPos.offset(x, 0, z), isEven ? 1 : 2) + worldView!.world.setBlockStateId(this.targetPos.offset(x, 0, z), isOnDiagonal ? 1 : 2) } }renderer/playground/scenes/layers.ts (2)
23-28: Consider extracting the block filtering logicThis logic for filtering full blocks based on shapes is complex and would benefit from being extracted into a separate method with descriptive name for better readability.
- const fullBlocks = loadedData.blocksArray.filter(block => { - const b = this.Block.fromStateId(block.defaultState, 0) - if (b.shapes?.length !== 1) return false - const shape = b.shapes[0] - return shape[0] === 0 && shape[1] === 0 && shape[2] === 0 && shape[3] === 1 && shape[4] === 1 && shape[5] === 1 - }) + const fullBlocks = this.getFullCubeBlocks(loadedData.blocksArray)And add this method:
private getFullCubeBlocks(blocksArray) { return blocksArray.filter(block => { const b = this.Block.fromStateId(block.defaultState, 0) if (b.shapes?.length !== 1) return false const shape = b.shapes[0] return shape[0] === 0 && shape[1] === 0 && shape[2] === 0 && shape[3] === 1 && shape[4] === 1 && shape[5] === 1 }) }
37-37: Unused variableisEvenThe variable
isEvenis defined but never used in the code.- const isEven = x === zrenderer/playground/TouchControls2.tsx (4)
12-16: Prevent default for all touch eventsThis code prevents the default behavior for all touch events, which could interfere with other touch interactions on the page. Consider being more selective about which events to prevent.
- window.addEventListener('touchstart', (e) => { - e.preventDefault() - }) + // Only prevent default for touch events within our control areas + const preventDefaultForControlAreas = (e: TouchEvent) => { + const target = e.target as HTMLElement; + if (target.closest('.touch-control-area')) { + e.preventDefault() + } + } + window.addEventListener('touchstart', preventDefaultForControlAreas, { passive: false }) + + return () => { + window.removeEventListener('touchstart', preventDefaultForControlAreas) + }
27-32: Simplify coordinate-to-key mappingThe series of if statements for determining which key to press based on vector components could be simplified using a mapping.
- let key: string | undefined - if (vec3.z < 0) key = 'KeyW' - if (vec3.z > 0) key = 'KeyS' - if (vec3.y > 0) key = 'Space' - if (vec3.y < 0) key = 'ShiftLeft' - if (vec3.x < 0) key = 'KeyA' - if (vec3.x > 0) key = 'KeyD' + const keyMap = { + 'z': { '-1': 'KeyW', '1': 'KeyS' }, + 'y': { '-1': 'ShiftLeft', '1': 'Space' }, + 'x': { '-1': 'KeyA', '1': 'KeyD' } + } + + let key: string | undefined + for (const [axis, mapping] of Object.entries(keyMap)) { + const value = vec3[axis as 'x' | 'y' | 'z'] + if (value !== 0) { + key = mapping[value > 0 ? '1' : '-1'] + break // Take the first non-zero value + } + }
62-66: Add class for control areas stylingTo support the selective prevention of default behavior suggested earlier, add a class to identify control areas.
pointer-events: none; touch-action: none; & > div { + class-name: touch-control-area; pointer-events: auto; }
77-77: Commented code should be removed or explainedThere's commented code without explanation. Either remove it or add a comment explaining why it's kept.
- // selector: 'body',src/react/MainMenuRenderApp.tsx (1)
115-115: Consider adding a comment explaining the URL redirectionThe condition for setting the 'to' parameter for non-mcraft.fun origins isn't immediately clear. Adding a comment would help future developers understand the purpose.
if (mapsProviderUrl && location.origin !== 'https://mcraft.fun') { + // Set redirection parameter when accessing from a domain other than the main site mapsProviderUrl.searchParams.set('to', location.href) }renderer/viewer/lib/mesher/getPreflatBlock.ts (3)
1-21: Add TypeScript type annotations for better type safetyThe function lacks proper TypeScript type annotations for parameters and return values, which makes it harder to understand the expected input and output types and could lead to runtime errors.
-export const getPreflatBlock = (block, reportIssue?: () => void) => { +export const getPreflatBlock = (block: { type: string; metadata: string; name?: string; _properties: Record<string, any> }, reportIssue?: () => void) => {Additionally, the return type should be specified to clarify what the function returns.
7-7: Better error handling for missing block typesThe fallback logic only triggers if no match is found for the exact block ID and metadata, but there's no clear indication to the user when this happens beyond the optional
reportIssuecallback.Consider adding more robust error handling with more descriptive error information, such as which block type wasn't found:
- const namePropsStr = legacyJson.blocks[b.type + ':' + b.metadata] || findClosestLegacyBlockFallback(b.type, b.metadata, reportIssue) + const blockKey = b.type + ':' + b.metadata; + const namePropsStr = legacyJson.blocks[blockKey] || + (reportIssue && reportIssue(`Block not found: ${blockKey}`), + findClosestLegacyBlockFallback(b.type, b.metadata, reportIssue))
12-17: Add error handling for malformed property stringsThe property parsing logic doesn't handle edge cases when the property string is malformed.
Add more robust error handling when parsing properties:
- const newProperties = Object.fromEntries(propsStr.join('').split(',').map(x => { - let [key, val] = x.split('=') - if (!isNaN(val)) val = parseInt(val, 10) - return [key, val] - })) + const newProperties = Object.fromEntries(propsStr.join('').split(',') + .filter(x => x && x.includes('=')) // Filter out invalid entries + .map(x => { + const parts = x.split('='); + const key = parts[0]?.trim(); + let val = parts[1]; + if (val !== undefined && !isNaN(val as any)) val = parseInt(val, 10); + return [key, val]; + }))renderer/viewer/lib/mesher/modelsGeometryCommon.ts (1)
75-154: Add documentation for the side and webgpuSide propertiesThe newly added
sideandwebgpuSideproperties lack documentation explaining their purpose and usage in the WebGPU rendering context.Add a comment at the top of the
elemFacesobject explaining what these properties represent:+/** + * Element faces with their directional properties for rendering. + * - side: Index used for traditional rendering (0-5) + * - webgpuSide: Index used for WebGPU rendering pipeline (0-5) + * - dir: Direction vector + * - mask1/mask2: Masking vectors for face calculations + * - corners: Corner coordinates for the face + */ export const elemFaces = { // ... rest of the coderenderer/playground/Cube.frag.wgsl (2)
21-24: Fix double semicolon and hardcoded depth valuesThere's a double semicolon on line 23, and the depth linearization uses hardcoded values that should be parameters.
var output: FragmentOutput; - output.depth = linearize_depth_ndc(Position.z, 0.05, 10000) / 10000; - output.color = vec4f(pixelColor.rgb, 1.0) * ColorBlend;; + // Use uniform values for near and far planes rather than hardcoded values + output.depth = linearize_depth_ndc(Position.z, 0.05, 10000) / 10000; + output.color = vec4f(pixelColor.rgb, 1.0) * ColorBlend;It would be even better to provide these depth values as uniform parameters rather than hardcoded values:
@group(0) @binding(3) var<uniform> depthParams: vec2f; // x: near, y: far // Then use: output.depth = linearize_depth_ndc(Position.z, depthParams.x, depthParams.y) / depthParams.y;
4-7: Improve struct formatting for better readabilityThe indentation in the
FragmentOutputstruct is inconsistent, which affects readability.- struct FragmentOutput { - @builtin(frag_depth) depth: f32, - @location(0) color: vec4f - } + struct FragmentOutput { + @builtin(frag_depth) depth: f32, + @location(0) color: vec4f + }renderer/playground/scenes/cubesHouse.ts (3)
1-16: Rename file or class to maintain consistencyThe class name
RailsCobwebScenedoesn't match the filenamecubesHouse.ts, which can cause confusion.Either rename the file to match the class name, or rename the class to match the file name:
-export default class RailsCobwebScene extends BasePlaygroundScene { +export default class CubesHouseScene extends BasePlaygroundScene {
4-9: Provide more descriptive property for webgpuRendererParamsThe
webgpuRendererParamsproperty is set totrue, which suggests it's a boolean flag rather than containing parameters for the WebGPU renderer.- webgpuRendererParams = true + webgpuRendererParams = { + enabled: true, + // Add other parameters that the WebGPU renderer might need + tileSize: 16, + maxTiles: 256 + }
30-39: Remove or properly document commented-out codeThere's a large block of commented-out code that should either be removed or properly documented as to why it's kept.
Either remove the commented code or add a comment explaining why it's preserved:
const squareSize = chunkDistance * 16 + // Previous implementation using block state IDs - kept for reference + // in case we need to revert to this approach for testing // for (let y = 0; y < squareSize; y += 2) { // for (let x = 0; x < squareSize; x++) { // for (let z = 0; z < squareSize; z++) { // const isEven = x === z // if (y > 400) continue // worldView!.world.setBlockStateId(this.targetPos.offset(x, y, z), isEven ? 1 : 2) // } // } // }src/integratedServer/customClient.js (3)
6-18: Commented code should be removed or documentedThis commented-out block appears to be an earlier implementation or example of custom communication using window objects. Consider either removing it if it's no longer needed or adding a comment explaining why it's kept.
53-56: Improve end reason documentationThe end method stores and emits the reason, but the comment suggests it only works on the server side. This behavior should be clearly documented to prevent confusion.
- end(reason) { - this._endReason = reason - this.emit('end', this._endReason) // still emits on server side only, doesn't send anything to our client - } + /** + * Handles connection termination. + * @param {string} reason - Reason for termination + * @note This only emits on server side and doesn't send anything to the client + */ + end(reason) { + this._endReason = reason + this.emit('end', this._endReason) + }
68-74: Empty method stubs need implementation or documentationThe writeBundle and writeRaw methods are marked as no-op but don't explain why they exist. If they're required by an interface or for compatibility, this should be documented.
- writeBundle(packets) { - // no-op - } - - writeRaw(buffer) { - // no-op - } + /** + * Required by the protocol interface but not implemented for this client. + * @param {Array} packets - Packet bundle to write + */ + writeBundle(packets) { + // no-op - not needed for this implementation + } + + /** + * Required by the protocol interface but not implemented for this client. + * @param {Buffer} buffer - Raw buffer to write + */ + writeRaw(buffer) { + // no-op - not needed for this implementation + }src/appViewerLoad.ts (1)
16-27: Consider alternative to exposing loadBackend globallyExposing the loadBackend function directly on the window object creates a global that could be accidentally overwritten or misused.
Consider using a more controlled approach to expose API functionality:
-window.loadBackend = loadBackend +// Create a namespace for API functions +window.prismcraftAPI = window.prismcraftAPI || {} +window.prismcraftAPI.loadBackend = loadBackendrenderer/viewer/lib/webgpuShaders/RadialBlur/frag.wgsl (1)
6-7: Unused constants detected.
sampleDistandsampleStrengthare never referenced in the shader. Consider removing or leveraging them if they are part of future plans.-const sampleDist : f32 = 1.0; -const sampleStrength : f32 = 2.2;src/react/MainMenu.tsx (1)
116-118: Verify debug tooltip usage.
alwaysTooltip='CHECK MAPS PERF!'appears as a debug note. Ensure it’s intentional or restricted to development builds.src/createLocalServer.ts (1)
8-11: TypeScript error suppression.
//@ts-expect-erroronserver.optionsandserver.looseProtocolModemay mask genuine type issues. Address these errors directly if possible.renderer/viewer/webgpu/worldrendererWebgpu.ts (1)
234-317: Large initialization method.
initWebgpuhandles multiple concerns (canvas setup, worker spawning, event loops). Consider extracting sub-steps into separate helper functions to improve clarity and maintainability.src/optionsStorage.ts (2)
70-70: New WebGPU configuration parameter object.The addition of
webgpuRendererParamsas an emptyRecord<string, any>provides a placeholder for future WebGPU-specific configurations. Consider adding TypeScript typing rather than usinganyto improve type safety and development experience.- webgpuRendererParams: {} as Record<string, any>, + webgpuRendererParams: {} as Record<string, WebGPURendererParam>,Where
WebGPURendererParamwould be a type defined elsewhere in your codebase.
114-118: WebGPU set as default renderer.The configuration now sets WebGPU as the default renderer and consolidates shared renderer options. This is a significant change that:
- Makes WebGPU the default experience for all users
- Restructures how renderer options are organized
Ensure this default setting aligns with the stability of the WebGPU implementation. Consider setting Three.js as default until WebGPU is fully stable, or implement feature detection to automatically select the most appropriate renderer.
src/worldSaveWorker.ts (2)
23-74: Chunk reading implementation with proper error handling.The implementation correctly:
- Processes messages of type 'readChunks'
- Configures the filesystem
- Initializes the world loader
- Processes chunks and posts results back to the main thread
Some improvements to consider:
- Add error handling around async operations
- Add progress reporting for large chunk operations
- Clean up commented code (lines 34, 38-43)
Example implementation for error handling:
for (const chunk of chunks) { - const column = await world.getColumn(chunk[0], chunk[1]) - if (!column) throw new Error(`Column ${chunk[0]} ${chunk[1]} not found`) - postMessage({ - column: columnToTransfarable(column) - }) + try { + const column = await world.getColumn(chunk[0], chunk[1]) + if (!column) throw new Error(`Column ${chunk[0]} ${chunk[1]} not found`) + postMessage({ + column: columnToTransfarable(column) + }) + } catch (error) { + postMessage({ + error: `Failed to load column at ${chunk[0]},${chunk[1]}: ${error.message}` + }) + } }
76-76: Remove commented out code.This commented line should be removed as it's redundant - the same functionality is already implemented on line 14.
-// window.fs = fssrc/optionsGuiScheme.tsx (2)
72-78: Added WebGPU renderer selection in UI.Good implementation of the renderer selection option in the UI, allowing users to switch between Three.js (stable) and WebGPU (new).
Consider adding a tooltip explaining the implications of choosing each renderer, especially since WebGPU is marked as "new" and may have stability or compatibility issues on some devices.
activeRenderer: { text: 'Renderer', + tooltip: 'WebGPU offers improved performance but may not be supported on all devices. Three.js provides better compatibility.', values: [ ['three', 'Three.js (stable)'], ['webgpu', 'WebGPU (new)'], ], },
157-157: Increased render distance limits.Increasing the maximum render distance from 16 to 32 for single-player and from 12 to 16 for multiplayer is a significant change that will affect performance and memory usage. This aligns with the WebGPU renderer's capabilities but should be tested thoroughly for performance impact, especially on lower-end devices.
Consider implementing progressive render distance adjustment based on the client's performance capabilities to ensure a smooth experience across different hardware configurations.
renderer/viewer/webgpu/graphicsBackendWebgpu.ts (2)
9-45: Consider implementing the unfilled backend methods or adding TODOs.
The methodsstartPanorama,disconnect, andsetRenderingare placeholders and remain empty. If they’re intended for future expansion, adding a brief comment or TODO can help track upcoming work.startPanorama () { + // TODO: Implement panorama rendering or remove this if unused }, disconnect () { + // TODO: Handle WebGPU disconnection or cleanup if needed }, setRendering (rendering) { + // TODO: Toggle or update rendering options if necessary },
9-45: Optional: Add error handling for the asynchronous startWorld method.
startWorldawaitsworldRenderer.readyPromise. Consider a try/catch block if unexpected initialization failures can occur, particularly since rendering backends often deal with hardware constraints.src/controls.ts (1)
602-608: Remove or clarify commented-out code.
Several lines are commented out here, referencingviewer.world.chunksReset()andlocalServer. Unless these are placeholders for debugging or future enhancements, consider removing or adding a clear comment describing why they're commented out.renderer/playground/chunksStorage.ts (4)
23-37: O(n) chunk search could affect performance at scale.
findBelongingChunklinearly traversesthis.chunks. For a large number of chunks, a more efficient data structure (e.g., a segment tree or binary search approach) may help.
59-72:printBlockis straightforward, but watch for possible performance overhead.
Looping over all blocks in a chunk section is fine for small or moderate size. Consider an index-based lookup if you anticipate large arrays.
145-175:removeChunkeffectively marks chunks as free.
The partial or commented-out merging code indicates potential future optimization for combining adjacent free chunks. Consider finishing it or removing it for clarity.
233-235:clearRangesets blocks to undefined effectively.
This method cleanly invalidates a range of blocks. If large ranges are cleared frequently, consider splicing or chunk-level resets to optimize performance.src/index.ts (4)
95-95: Side-effect import
Confirm thatappViewerLoadis indeed necessary. Removing undesired side effects can keep the module lean.
219-221: Remove or clarify commented-out code
These lines reference a TODO but remain fully commented out, which may cause confusion.
334-334: Avoid using 'any'
Consider using a more specific type definition forCustomClient.
348-367: Large commented block
These lines are fully commented out. Consider removing or converting to a clearly tracked TODO if not needed.src/integratedServer/worker.ts (4)
8-8: Improve type for 'options'
Currently typed as{ options }without a concrete shape. Consider defining a more accurate interface for better type safety.
36-37: Remove 'debugger' statement
Leaving adebuggerin production code can cause unintended breakpoints.🧰 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)
63-63: Specify a type for 'serverOptions'
Adding a more explicit type signature helps detect errors early.
145-149: Implement auto-save logic
An interval is set, butsaveServeris marked as TODO.Would you like help implementing the auto-save feature?
src/loadSave.ts (1)
20-20: Avoid polluting global scope
AttachingfsStatetoglobalThismay lead to naming collisions or hidden dependencies. Consider alternatives if feasible.renderer/playground/Cube.vert.wgsl (4)
58-58: Use a constant for color normalizationDividing by 255 directly is not ideal. Use a constant to make the code more maintainable.
- colorBlend /= 255; + const COLOR_NORMALIZE = 1.0 / 255.0; + colorBlend *= COLOR_NORMALIZE;
64-90: Consider refactoring the face selection switch statementThe current switch statement has significant repetition of similar operations, making it harder to maintain.
Consider refactoring to reduce repetition:
- switch (normalIndex) { - case 0: - { - Uv = vec2((1.0f-uv.x), (1.0 - uv.y)); - textureIndex = models[modelIndex].textureIndex123 & 1023; - } - case 1: - { - textureIndex = (models[modelIndex].textureIndex123 >> 10) & 1023; - } - case 2: - { - textureIndex = (models[modelIndex].textureIndex123 >> 20) & 1023; - } - case 3: - { - textureIndex = models[modelIndex].textureIndex456 & 1023; - } - case 4: - { - textureIndex = (models[modelIndex].textureIndex456 >> 10) & 1023; - } - case 5, default: - { - textureIndex = (models[modelIndex].textureIndex456 >> 20) & 1023; - } - } + // Flip UVs for front face + if (normalIndex == 0) { + Uv = vec2((1.0f-uv.x), (1.0 - uv.y)); + } + + // Extract texture index based on face index + let shift = normalIndex % 3 * 10u; + let textureContainer = normalIndex < 3 ? models[modelIndex].textureIndex123 : models[modelIndex].textureIndex456; + textureIndex = (textureContainer >> shift) & 1023u;
94-94: Overly complex UV calculationThe UV calculation is complex and lacks comments to explain its purpose, making it harder to debug and maintain.
Break down the calculation into smaller steps with meaningful variable names:
- Uv = vec2(Uv / tilesPerTexture + vec2f(trunc(f32(textureIndex) % tilesPerTexture.y), trunc(f32(textureIndex) / tilesPerTexture.x)) / tilesPerTexture); + // Calculate the position of the texture tile in the atlas + let tileX = trunc(f32(textureIndex) % tilesPerTexture.y); + let tileY = trunc(f32(textureIndex) / tilesPerTexture.x); + let tileOffset = vec2f(tileX, tileY) / tilesPerTexture; + + // Scale and offset UVs to map to the correct tile + Uv = vec2(Uv / tilesPerTexture + tileOffset);
28-35: Consider grouping bindings by their binding groupsThe bindings are not organized by their groups, making it harder to understand the resource layout.
Reorganize the bindings by their groups for better readability:
-@group(1) @binding(0) var<storage, read> cubes: array<Cube>; -@group(0) @binding(0) var<uniform> ViewProjectionMatrix: mat4x4<f32>; -@group(0) @binding(3) var<storage, read> models: array<CubeModel>; -@group(1) @binding(1) var<storage, read> visibleCubes: array<CubePointer>; -@group(1) @binding(2) var<storage, read> chunks : array<Chunk>; -@group(0) @binding(4) var<uniform> rotatations: array<mat4x4<f32>, 6>; -@group(0) @binding(2) var myTexture: texture_2d<f32>; -@group(0) @binding(5) var<uniform> tileSize: vec2<f32>; +// Camera and global rendering parameters (Group 0) +@group(0) @binding(0) var<uniform> ViewProjectionMatrix: mat4x4<f32>; +@group(0) @binding(2) var myTexture: texture_2d<f32>; +@group(0) @binding(3) var<storage, read> models: array<CubeModel>; +@group(0) @binding(4) var<uniform> rotatations: array<mat4x4<f32>, 6>; +@group(0) @binding(5) var<uniform> tileSize: vec2<f32>; + +// Cube and chunk data (Group 1) +@group(1) @binding(0) var<storage, read> cubes: array<Cube>; +@group(1) @binding(1) var<storage, read> visibleCubes: array<CubePointer>; +@group(1) @binding(2) var<storage, read> chunks : array<Chunk>;renderer/playground/webgpuRendererShared.ts (4)
1-2: Browser detection can be improvedThe current Safari detection relies on a regex pattern against the user agent, which is brittle and may break with browser updates. Additionally, the worker parameter retrieval lacks error handling.
Consider using feature detection instead of browser detection and add error handling:
-const workerParam = new URLSearchParams(typeof window === 'undefined' ? '?' : window.location.search).get('webgpuWorker') -const isSafari = /^((?!chrome|android).)*safari/i.test(navigator.userAgent) +const workerParam = (() => { + try { + return new URLSearchParams(typeof window === 'undefined' ? '?' : window.location.search).get('webgpuWorker') + } catch (e) { + console.warn('Failed to get webgpuWorker parameter:', e) + return null + } +})() + +// Use feature detection instead of user agent sniffing +const hasWorkersFeature = typeof window !== 'undefined' && 'Worker' in window +const isSafari = (() => { + // First try feature detection for Safari-specific issues + try { + const canvas = document.createElement('canvas') + // Safari-specific limitation check + return !('colorBufferFloat' in (canvas.getContext('webgpu') || {})) + } catch (e) { + // Fallback to user agent if feature detection fails + return typeof navigator !== 'undefined' && /^((?!chrome|android).)*safari/i.test(navigator.userAgent) + } +})()
4-13: Add documentation for renderer parametersThe renderer parameters lack documentation explaining their purpose and impact on performance/visual quality.
Add JSDoc comments to explain each parameter:
export const defaultWebgpuRendererParams = { + /** Enables secondary camera view (e.g., for debugging or picture-in-picture) */ secondCamera: false, + /** Enables Multi-Sample Anti-Aliasing for smoother edges */ MSAA: false, + /** Applies offset to the camera position [x, y, z] */ cameraOffset: [0, 0, 0] as [number, number, number], + /** Uses a Web Worker for rendering calculations (improves main thread performance) */ webgpuWorker: workerParam ? workerParam === 'true' : !isSafari, + /** Enables volumetric light scattering effect */ godRays: true, + /** Enables occlusion culling to improve performance */ occlusionActive: true, + /** Enables early Z rejection optimization */ earlyZRejection: false, + /** Allows dynamic updates to chunk view distance based on performance */ allowChunksViewUpdate: false }
27-28: Define constants with meaningful namesThe constants
WEBGPU_FULL_TEXTURES_LIMITandWEBGPU_HEIGHT_LIMITare defined without explanation of their purpose or why they're set to 1024.Add comments and consider using power-of-two values explicitly:
-export const WEBGPU_FULL_TEXTURES_LIMIT = 1024 -export const WEBGPU_HEIGHT_LIMIT = 1024 +/** + * Maximum number of textures that can be loaded in the atlas (2^10) + * Limited by WebGPU implementation constraints and memory considerations + */ +export const WEBGPU_FULL_TEXTURES_LIMIT = 1024 // 2^10 + +/** + * Maximum height for textures in pixels (2^10) + * Ensures compatibility with various GPUs and memory constraints + */ +export const WEBGPU_HEIGHT_LIMIT = 1024 // 2^10
30-32: Improve type definitionsThe type definitions are minimal and could benefit from more detailed information about their properties.
Enhance the type definitions with more detailed information:
-export type RendererInitParams = GPURequestAdapterOptions & {} +/** + * Parameters used to initialize the WebGPU renderer + * Extends the standard GPURequestAdapterOptions with additional configuration + */ +export type RendererInitParams = GPURequestAdapterOptions & { + // Add any additional initialization parameters here + canvas?: HTMLCanvasElement; + devicePixelRatio?: number; +} -export type RendererParams = typeof defaultWebgpuRendererParams +/** + * Runtime parameters for the WebGPU renderer + * Controls various rendering features and optimizations + */ +export type RendererParams = typeof defaultWebgpuRendererParamsrenderer/viewer/three/worldrendererThree.ts (1)
122-125: Consider adding a fallback for fog configurationThe fog effect is conditionally set based on the
waterBreathingproperty, but there might be edge cases where this property is undefined or invalid.Add a default fallback for the fog configuration:
override watchReactivePlayerState () { this.onReactiveValueUpdated('inWater', (value) => { - this.scene.fog = value ? new THREE.Fog(0x00_00_ff, 0.1, this.displayOptions.playerState.reactive.waterBreathing ? 100 : 20) : null + if (value) { + const waterBreathing = this.displayOptions.playerState.reactive.waterBreathing ?? false; + const visibilityDistance = waterBreathing ? 100 : 20; + this.scene.fog = new THREE.Fog(0x00_00_ff, 0.1, visibilityDistance); + } else { + this.scene.fog = null; + } })renderer/playground/baseScene.ts (5)
28-28: Optional: Clarify or remove unused boolean property.
If this property is never set to true, consider removing it or clarifying its intent.
204-205: Commented-out rotation call.
The oldviewer.camera.rotation.set()remains commented. If it’s no longer needed, consider removing it to reduce clutter.
209-218: Camera state to string is functional.
Storing rotation in decimal is fine, but be mindful of rounding if you need high precision.
381-382: Large vertical jump bound to KeyT.
Consider a smaller increment or matching WASD logic if needed for consistency. If only for debug, this is fine.
424-427: Alt-based sprint factor.
Consider making speed scaling customizable in the UI or config. Currently this magic number (4) might need future tuning.- const scalar = pressedKeys.has('AltLeft') ? 4 : 1 + const movementFactor = this.params.movementSpeed ?? 4 + const scalar = pressedKeys.has('AltLeft') ? movementFactor : 1renderer/playground/CubeDef.ts (1)
71-73: Fix indentation and end-of-file newline.
Please address the ESLint errors:+// 0.5, 0.5, 0.5, 1, 1, // top-right +// -0.5, -0.5, 0.5, 0, 0, // bottom-let + // Add a newline at the end of file🧰 Tools
🪛 ESLint
[error] 71-71: Expected indentation of 2 spaces but found 1.
(@stylistic/indent)
[error] 72-72: Expected indentation of 2 spaces but found 1.
(@stylistic/indent)
[error] 73-73: Newline required at end of file but not found.
(@stylistic/eol-last)
renderer/playground/scenes/floorRandom.ts (2)
5-5: Setting WebGPU renderer params by default.
Ensure that this scene truly needs WebGPU-specific params. Otherwise, consider toggling it in user configs.
31-47: Consider removing old commented-out logic.
If no longer needed, removing commented blocks can help prevent confusion.src/integratedServer/main.ts (2)
34-53:restorePatchedDataDeepfunction mutates arrays/objects to include_isBuffer.
This approach can be risky if other code expects vanillaUint8Arrays. Consider a safer approach or disclaimers about possible side effects.
100-114:setupEventsestablishes event listeners for loading status and notifications.
Implementation is straightforward. Confirm no risk of double-listeners if called multiple times.renderer/viewer/lib/worldrendererCommon.ts (4)
19-19: Unused import noticed.
removeStatis imported but does not appear to be referenced in the changed code. Consider removing it if not needed.-import { removeAllStats, removeStat, updateStatText } from './ui/newStats' +import { removeAllStats, updateStatText } from './ui/newStats'
175-175: Ensure subscription lifecycle.
Invokingthis.watchReactivePlayerState()is valid. However, consider whether you'll need to unsubscribe from reactivity when destroying the renderer to avoid potential memory leaks.
250-253: Return unsubscribe handle.
onReactiveValueUpdatedimmediately calls the callback and then subscribes withsubscribeKey. Returning the unsubscribe handle could help manage subscriptions more gracefully.onReactiveValueUpdated<T extends keyof typeof this.displayOptions.playerState.reactive>( key: T, callback: (value: typeof this.displayOptions.playerState.reactive[T]) => void ) { callback(this.displayOptions.playerState.reactive[key]) - subscribeKey(this.displayOptions.playerState.reactive, key, callback) + return subscribeKey(this.displayOptions.playerState.reactive, key, callback) }
526-526: Clarify stat label for geometry count.
Currently labeled as(.../${this.geometryReceiveCountPerSec}ss/...). For readability, consider a clearer suffix such asgeom/sor a short tooltip.renderer/playground/webgpuBlockModels.ts (1)
47-103: Selective handling of single-element block models.
addBlockModelfilters out multi-element models (exceptgrass_block), skipping partial or composite geometry. This approach is valid for straightforward, full-cube models, but may miss blocks with more complex shapes. If you plan to support multi-element blocks, consider extending this logic.src/appViewer.ts (2)
58-58: Setter for custom renderer properties.
setRendererSpecificSettingswith(key: string, value: any)works but losing type safety onvalue. Consider restricting it to maintain stronger type checks.
153-153: Consider removing commented code.
If the callback toworldView!.initis no longer needed, remove this or add a clear TODO comment to avoid confusion.renderer/viewer/lib/mesher/models.ts (4)
5-5: Consider removing or renaming the alias import.Importing
WorldBlocktwice (as bothBlockandWorldBlock) may be confusing. If you need both names, consider clarifying their purposes; otherwise, remove the extra alias.
127-202: Refactor the largerenderLiquidfunction for better maintainability.This function performs multiple nested operations for neighbor checks, height computations, and tile attribute assignments. Splitting these concerns into smaller helper functions (e.g., one for height calculations, one for tile assignments) can improve readability and reduce cognitive load.
155-155: Avoid fixed type references in color logic.The water tint logic is hard-coded to
[1, 1, 1]fallback. Consider either passing an explicit color or using a config-driven approach if new liquid types need distinct tints in the future.
409-434: Extract tile-building logic into a dedicated helper.The code repeatedly checks or initializes
tiles[blockKey], updates visible faces, and pushes face data. If you do this in multiple places or foresee expansions, extracting it into its own function can enhance maintainability and reusability.renderer/playground/CubeSort.comp.wgsl (1)
45-99: Improve documentation of occlusion logic and bit manipulation.The shift operations and chunk index manipulations are not immediately obvious. Adding comments (e.g., clarifying
(occlusionData - 1) << 3) can help maintainers quickly understand how side bits and indices are encoded.src/browserfs.ts (3)
144-163: Remove or restore commented-out ZipFS code.The entire ZipFS configuration is commented out. If it’s no longer needed, remove the dead code. Otherwise, clarify if you plan to re-enable it later.
255-275: Decide on resource pack detection approach.Much of the resource pack detection logic is commented out. If you plan to handle resource packs differently, remove the outdated code. Otherwise, document how or when you’ll restore these lines.
279-325: Clarify UI-based directory picker strategy.All code for opening a world directory is commented out, disabling direct file system access. If you intend to re-enable it, confirm whether you require user read-write permissions. Otherwise, consider removing these commented blocks to simplify maintenance.
renderer/playground/Cube.comp.wgsl (1)
14-16: Be mindful of large atomic array usage.Defining a 4096×4096 array of atomics (over 16 million elements) can significantly impact GPU memory usage and performance. Consider chunking or reducing resolution if not strictly required.
src/integratedServer/browserfsShared.ts (1)
18-18: Avoid using the delete operator.Deleting object properties can degrade performance. Instead, assign
undefinedor refactor the data structure to handle conditional mount points more efficiently.-if (typeof localStorage === 'undefined') { - delete defaultMountablePoints['/world'] -} +if (typeof localStorage === 'undefined') { + 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)
renderer/playground/webgpuRenderer.ts (4)
104-110: Handle initialization errors more robustly.When
this.init()fails, the code logs an error and posts a message. Consider retry strategies or fallback modes if WebGPU initialization is partially possible.
950-955: Pause loop behavior clarity.When
this.renderingis false, the loop early exits unless forced. If the loop is paused for a long time and then resumed, confirm you don’t need any re-initialization before rendering again.
1110-1120: Check performance impact of volumetric pass.The separate volumetric pass can be expensive. Evaluate whether you need a partial or full-screen pass every frame, especially under high object counts or low-end devices.
1184-1187: Confirm camera tween usage.Using a Tween for camera updates might induce unpredictable results if other camera movements are happening in parallel. Confirm it aligns with your desired animation style or consider direct matrix setting for performance-critical paths.
📜 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 (99)
.eslintrc.json(1 hunks).npmrc(1 hunks).vscode/launch.json(1 hunks).vscode/tasks.json(1 hunks)buildWorkers.mjs(1 hunks)config.json(1 hunks)cypress/e2e/performance.spec.ts(1 hunks)package.json(7 hunks)renderer/buildMesherWorker.mjs(2 hunks)renderer/package.json(1 hunks)renderer/playground.html(1 hunks)renderer/playground/Cube.comp.wgsl(1 hunks)renderer/playground/Cube.frag.wgsl(1 hunks)renderer/playground/Cube.vert.wgsl(1 hunks)renderer/playground/CubeDef.ts(1 hunks)renderer/playground/CubeSort.comp.wgsl(1 hunks)renderer/playground/TextureAnimation.ts(1 hunks)renderer/playground/TouchControls2.tsx(1 hunks)renderer/playground/baseScene.ts(10 hunks)renderer/playground/chunksStorage.test.ts(1 hunks)renderer/playground/chunksStorage.ts(1 hunks)renderer/playground/messageChannel.ts(1 hunks)renderer/playground/scenes/cubesHouse.ts(1 hunks)renderer/playground/scenes/floorRandom.ts(1 hunks)renderer/playground/scenes/floorStoneWorld.ts(1 hunks)renderer/playground/scenes/index.ts(1 hunks)renderer/playground/scenes/layers.ts(1 hunks)renderer/playground/shared.ts(2 hunks)renderer/playground/webgpuBlockModels.ts(1 hunks)renderer/playground/webgpuRenderer.ts(1 hunks)renderer/playground/webgpuRendererShared.ts(1 hunks)renderer/playground/webgpuRendererWorker.ts(1 hunks)renderer/playground/workerProxy.ts(1 hunks)renderer/rsbuildSharedConfig.ts(4 hunks)renderer/viewer/common/webglData.ts(1 hunks)renderer/viewer/lib/basePlayerState.ts(3 hunks)renderer/viewer/lib/mesher/getPreflatBlock.ts(1 hunks)renderer/viewer/lib/mesher/mesher.ts(1 hunks)renderer/viewer/lib/mesher/models.ts(8 hunks)renderer/viewer/lib/mesher/modelsGeometryCommon.ts(6 hunks)renderer/viewer/lib/mesher/test/mesherTester.ts(1 hunks)renderer/viewer/lib/mesher/world.ts(7 hunks)renderer/viewer/lib/ui/newStats.ts(3 hunks)renderer/viewer/lib/webgpuShaders/RadialBlur/frag.wgsl(1 hunks)renderer/viewer/lib/webgpuShaders/RadialBlur/vert.wgsl(1 hunks)renderer/viewer/lib/workerProxy.ts(3 hunks)renderer/viewer/lib/worldDataEmitter.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(9 hunks)renderer/viewer/three/graphicsBackend.ts(1 hunks)renderer/viewer/three/holdingBlock.ts(1 hunks)renderer/viewer/three/worldrendererThree.ts(1 hunks)renderer/viewer/webgpu/graphicsBackendWebgpu.ts(1 hunks)renderer/viewer/webgpu/worldrendererWebgpu.ts(1 hunks)rsbuild.config.ts(2 hunks)src/appViewer.ts(6 hunks)src/appViewerLoad.ts(1 hunks)src/browserfs.ts(5 hunks)src/builtinCommands.ts(3 hunks)src/controls.ts(4 hunks)src/createLocalServer.ts(1 hunks)src/customClient.js(0 hunks)src/customServer.ts(0 hunks)src/devtools.ts(2 hunks)src/flyingSquidEvents.ts(0 hunks)src/globals.d.ts(1 hunks)src/googledrive.ts(0 hunks)src/index.ts(9 hunks)src/integratedServer/browserfsServer.ts(1 hunks)src/integratedServer/browserfsShared.ts(1 hunks)src/integratedServer/customClient.js(1 hunks)src/integratedServer/customServer.ts(1 hunks)src/integratedServer/main.ts(1 hunks)src/integratedServer/worker.ts(1 hunks)src/integratedServer/workerMcData.mjs(1 hunks)src/loadSave.ts(3 hunks)src/localServerMultiplayer.ts(4 hunks)src/optionsGuiScheme.tsx(3 hunks)src/optionsStorage.ts(4 hunks)src/packetsReplay/replayPackets.ts(1 hunks)src/react/AppStatusProvider.tsx(1 hunks)src/react/ButtonWithTooltip.tsx(2 hunks)src/react/CreateWorldProvider.tsx(1 hunks)src/react/DebugOverlay.module.css(1 hunks)src/react/MainMenu.tsx(3 hunks)src/react/MainMenuRenderApp.tsx(3 hunks)src/react/PauseScreen.tsx(5 hunks)src/react/SingleplayerProvider.tsx(1 hunks)src/resourcePack.ts(1 hunks)src/resourcesManager.ts(3 hunks)src/shims/empty.ts(1 hunks)src/shims/fs.js(1 hunks)src/shims/perf_hooks_replacement.js(1 hunks)src/styles.css(1 hunks)src/utils.ts(2 hunks)src/watchOptions.ts(2 hunks)src/water.ts(3 hunks)src/worldSaveWorker.ts(1 hunks)tsconfig.json(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (4)
- src/googledrive.ts
- src/customServer.ts
- src/flyingSquidEvents.ts
- src/customClient.js
🧰 Additional context used
🧬 Code Definitions (21)
src/devtools.ts (1)
renderer/viewer/three/worldrendererThree.ts (1)
WorldRendererThree(31-610)
src/integratedServer/customServer.ts (1)
src/integratedServer/customClient.js (2)
createLocalServerClientImpl(20-76)createLocalServerClientImpl(20-76)
src/localServerMultiplayer.ts (1)
src/integratedServer/main.ts (1)
getLocalServerOptions(30-32)
src/optionsGuiScheme.tsx (1)
src/optionsStorage.ts (1)
options(208-213)
src/integratedServer/browserfsServer.ts (2)
src/loadSave.ts (1)
readLevelDat(29-43)src/integratedServer/browserfsShared.ts (3)
localFsState(44-46)mountRemoteFsBackend(184-204)configureBrowserFs(20-29)
src/react/ButtonWithTooltip.tsx (1)
src/react/MainMenu.tsx (1)
Props(32-186)
src/watchOptions.ts (1)
src/appViewer.ts (1)
appViewer(249-249)
src/appViewerLoad.ts (3)
src/appViewer.ts (2)
loadBackend(117-156)appViewer(249-249)src/optionsStorage.ts (1)
options(208-213)src/watchOptions.ts (1)
watchOptionsAfterViewerInit(33-116)
src/controls.ts (1)
src/loadSave.ts (1)
fsState(19-19)
src/react/PauseScreen.tsx (2)
src/loadSave.ts (1)
fsState(19-19)src/integratedServer/browserfsShared.ts (1)
uniqueFileNameFromWorldName(206-223)
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)
src/react/MainMenuRenderApp.tsx (1)
src/react/SignEditorProvider.tsx (1)
location(19-86)
renderer/playground/webgpuBlockModels.ts (1)
renderer/viewer/webgpu/worldrendererWebgpu.ts (1)
WorldRendererWebgpu(18-318)
src/resourcesManager.ts (1)
renderer/viewer/lib/guiRenderer.ts (1)
getLoadedImage(250-254)
renderer/viewer/lib/worldrendererCommon.ts (1)
renderer/viewer/lib/ui/newStats.ts (2)
updateStatText(82-85)removeAllStats(87-92)
renderer/viewer/lib/mesher/world.ts (2)
renderer/playground/webgpuBlockModels.ts (1)
AllBlocksStateIdToModelIdMap(159-159)renderer/viewer/lib/mesher/shared.ts (3)
CustomBlockModels(16-18)BlockStateModelInfo(49-54)MesherGeometryOutput(22-45)
renderer/playground/webgpuRendererWorker.ts (2)
renderer/playground/webgpuRenderer.ts (1)
WebgpuRenderer(16-1208)src/integratedServer/worker.ts (1)
workerProxyType(115-142)
src/loadSave.ts (1)
src/integratedServer/browserfsShared.ts (1)
initialFsState(31-43)
src/browserfs.ts (2)
src/loadSave.ts (2)
fsState(19-19)loadSave(45-179)src/integratedServer/browserfsShared.ts (1)
mountRemoteFsBackend(184-204)
src/builtinCommands.ts (2)
src/loadSave.ts (1)
fsState(19-19)src/globalState.ts (1)
miscUiState(109-129)
renderer/playground/webgpuRenderer.ts (2)
renderer/playground/webgpuRendererWorker.ts (5)
canvas(97-113)camera(211-237)postMessage(21-27)updateSize(52-55)chunksStorage(10-10)renderer/playground/webgpuBlockModels.ts (1)
BlocksModelData(161-167)
🪛 Biome (1.9.4)
.eslintrc.json
[error] 13-13: The key unicorn/no-typeof-undefined was already declared.
This where a duplicated key was declared again.
If a key is defined multiple times, only the last definition takes effect. Previous definitions are ignored.
(lint/suspicious/noDuplicateObjectKeys)
.vscode/launch.json
[error] 44-44: Expected a property but instead found '}'.
Expected a property here.
(parse)
.vscode/tasks.json
[error] 35-35: Expected a property but instead found '}'.
Expected a property here.
(parse)
src/worldSaveWorker.ts
[error] 20-21: A global variable should not be reassigned.
Assigning to a global variable can override essential functionality.
(lint/suspicious/noGlobalAssign)
src/controls.ts
[error] 950-950: This is an unexpected use of the debugger statement.
Unsafe fix: Remove debugger statement
(lint/suspicious/noDebugger)
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)
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)
[error] 186-203: Promise executor functions should not be async.
(lint/suspicious/noAsyncPromiseExecutor)
🪛 ESLint
src/devtools.ts
[error] 24-24: Expected to return a value in method 'get'.
(getter-return)
renderer/playground/messageChannel.ts
[error] 14-14: Missing space before function parentheses.
(@stylistic/space-before-function-paren)
[error] 26-26: Missing space before function parentheses.
(@stylistic/space-before-function-paren)
renderer/playground/CubeDef.ts
[error] 71-71: Expected indentation of 2 spaces but found 1.
(@stylistic/indent)
[error] 72-72: Expected indentation of 2 spaces but found 1.
(@stylistic/indent)
[error] 73-73: Newline required at end of file but not found.
(@stylistic/eol-last)
🔇 Additional comments (201)
renderer/viewer/lib/workerProxy.ts (3)
2-2: Potential confusion usingglobalThisinstead of a more explicit target.Using
channel ?? globalThisis acceptable in modern browser or worker environments that implementglobalThis. However, ensure this code doesn’t need to run in environments whereglobalThismay be undefined or differ fromselforwindow.Would you like to verify that all intended platforms and browsers properly implement
globalThisand event listeners? If so, I can generate a script to check for compatibility.
23-23: Enhancement to accept bothWorkerandMessagePort.Allowing
useWorkerProxyto accept either aWorkerorMessagePortbroadens the range of message channels that can be used, making the function more flexible.
44-44: Broadening the range of transferables to includeImageData.Including
ImageDataenables passing image pixel data more directly. Ensure all target runtimes support transferringImageData.Let me know if you want a script to locate existing usage of
ImageDataand confirm cross-browser support..npmrc (1)
3-3: Shell Emulator Setting Updated
The change to setshell-emulator=falseis clear and intentional. Please verify that this change aligns with your expected shell behavior in both local and CI environments, as it can affect how npm scripts execute.config.json (1)
5-5: Maps Provider URL Update
The maps provider URL now includes the query parameter?label=webgpu, which is likely used to distinguish requests or trigger WebGPU-specific handling. Please ensure that any backend or third-party service consuming this URL is updated to interpret this parameter correctly.renderer/playground.html (2)
12-15: Disable Touch Action for Mobile Optimization
The addition oftouch-action: none;within thehtml, bodyselector (line 14) prevents default touch behaviors, which can help provide more controlled interactions in your WebGPU renderer. This change looks appropriate for the intended mobile-friendly interface.
20-23: Universal Selector for Text Selection Disabling
The new universal selector (lines 20-23) applyinguser-select: none;and-webkit-user-select: none;effectively prevents unwanted text selection across the document. Verify that this global setting does not interfere with any interactive text elements you might need selectable.vitest.config.ts (1)
11-12: Excellent test configuration update.Adding a clarifying comment for the sign-renderer test path and including the new chunksStorage test file are both good practices. This ensures proper testing coverage for the new WebGPU renderer implementation.
tsconfig.json (2)
19-19: Reintroduced experimentalDecorators option.Enabling experimental decorators support is appropriate if the WebGPU renderer implementation uses decorator patterns or libraries that depend on this feature.
Have you confirmed that this feature is needed for the new WebGPU renderer? Since it was previously removed, it would be good to verify that it's still required.
24-29: Added path mapping for flying-squid.The baseUrl and paths configuration will facilitate imports from the flying-squid package by redirecting to the source files. This is a good practice for improving developer experience.
.vscode/tasks.json (1)
37-42:✅ Verification successful
Updated combined task with WebGL worker dependency.
The updated task now correctly includes all necessary dependencies (server, esbuild, and the new webgl-worker) to run in parallel. This streamlines the development workflow for the WebGPU renderer.
Verify that the
buildWorkers.mjsscript exists at the project root as referenced by the new worker task. If it's located elsewhere, update the path accordingly.
🏁 Script executed:
#!/bin/bash # Check if buildWorkers.mjs exists in the expected location ls -la buildWorkers.mjs if [ $? -ne 0 ]; then echo "buildWorkers.mjs not found at project root" echo "Searching for buildWorkers.mjs in the project..." find . -name "buildWorkers.mjs" -type f | grep -v "node_modules" fiLength of output: 109
WebGL Worker Dependency Verified and Approved
The combined task in
.vscode/tasks.jsonnow correctly includes all the necessary dependencies (server, esbuild, and webgl-worker) to run tasks in parallel for the WebGPU renderer. The verification confirmed thatbuildWorkers.mjsis present at the project root, so no path updates are required.
- Verified task dependencies in
.vscode/tasks.json.- Confirmed
buildWorkers.mjsexists at the project root.renderer/buildMesherWorker.mjs (2)
25-25: Optimized minification for development workflow.Changing minification to be conditional based on watch mode is a great improvement. This will make development builds faster and easier to debug, while still ensuring production builds are optimized.
42-42: Minor code style adjustment.Removed space between method name and parameters for consistency.
src/devtools.ts (1)
153-153: Good use of optional chaining for null safetyThe change to use a union type with
undefinedand the optional chaining operator improves code robustness by handling cases wherewindow.worldmight not be defined or not be an instance ofWorldRendererThree.rsbuild.config.ts (2)
114-114: Setting cleanDistPath to false preserves output directory contentThis configuration change prevents the build system from cleaning the output directory before building, which is useful when you need to preserve certain files between builds.
198-203: Proper handling of WebGPU renderer worker dependenciesThe conditional logic ensures that the WebGPU renderer worker script is available in the output directory, either by copying an existing file or by building it if needed. This is essential for the new WebGPU rendering pipeline.
renderer/rsbuildSharedConfig.ts (2)
55-58: Validate environment variables for release information.The lines defining
process.env.RELEASE_TAGandprocess.env.RELEASE_CHANGELOGrely on data fromrelease.json. Ensure these always contain safe strings (e.g., JSON-encoded) before exposing them as environment variables. Also confirm that no private or sensitive data is being leaked.
76-77: Confirm plugin order and compatibility.
pluginNodePolyfillis placed before optionally addingpluginBasicSsl. Double-check that any other plugins you rely on do not require a specific order to function correctly.renderer/viewer/lib/ui/newStats.ts (2)
10-10: Nullish coalescing improves readability.Using
y ?? lastYis a concise way to handle undefined positions. This effectively setstopstyle ifyis missing. Good improvement for clarity.
22-22: Revisit condition for detecting custom position.The check
if (y === undefined && x === rightOffset)is straightforward, but watch out for edge cases where y might be 0 intentionally. Ensure that 0 is never needed for a custom position.buildWorkers.mjs (2)
29-39: Confirm debugging strategy in watch mode.The code sets
sourcemapto'inline'in watch mode and'external'otherwise. Verify that this meets your debugging workflow, especially for large builds.
57-85: Handle dynamic requires carefully.The plugin replaces one dynamic require with a string-literal-based fallback. If prismarine-chunk changes its internal structure, this code might break. Keep an eye on library updates to ensure continuing compatibility.
renderer/viewer/lib/worldDataEmitter.ts (1)
259-259: Verify forgotten chunk cleanup whenallowPositionUpdateis false.
Disabling position updates might prevent chunks from unloading, potentially causing memory growth if the camera moves without re-enabling updates. Verify whether this is intentional or if periodic cleanup is still needed.src/resourcesManager.ts (3)
118-118: Asynchronous block atlas creation approved.
Callingawait this.recreateBlockAtlas(resources)ensures the atlas is ready before continuing. This is a good approach for maintaining resource consistency.
120-120: Abort check prevents unnecessary processing.
This early return respects the cancellation flow, avoiding expensive operations when already aborted.
122-122: Items assets parser initialization looks correct.
InstantiatingAtlasParserhere is straightforward and consistent with the new flow.renderer/playground/shared.ts (1)
18-25: Validate the new fields and optional properties inBlockType.
- Changing
facesto optional (faces?: BlockFaceType[]) requires ensuring that all consumers handle theundefinedcase correctly.- The addition of
visibleFaces,modelId, andtransparentas mandatory fields may break existing logic if any code instantiatesBlockTypewithout these properties. Confirm that all constructors or assignments are updated.- The newly added
chunk?: numbershould have at least minimal documentation or usage examples to confirm how/when it’s expected to be set.renderer/playground/webgpuRendererWorker.ts (7)
1-9: Ensure correct type usage and references.
@webgpu/typesreference suggests you are relying on the experimental WebGPU type definitions. Verify that your TypeScript configuration allows these definitions and that you’re targeting an environment supporting WebGPU as expected.- The imports from
THREEandtween.jsare valid, but confirm thattween.jsis actually utilized or remove the import if not needed.
21-27: DeclarepostMessagewith stronger types.
- Overriding
postMessageglobally may cause confusion between the worker’s message interface and the standardpostMessage.- Provide a clear type annotation for
(data, ...args)if possible.
64-75: Validate callback resolution in onceRendererAvailable().Once
webgpuRendererbecomes ready, each queued callback is invoked, but ensure that no partial- or re-initialization scenarios exist wherewebgpuRenderermight be replaced again.
95-113: Initialization flow check incanvas()method.
- The logic to handle the iOS Safari bug by recreating a canvas is correct, but ensure this scenario does not break event listeners or element references set up elsewhere.
- Validate that the old canvas is always removed from the DOM (line 105).
131-145: Confirm correctness in getFaces().Currently, these faces seem to be stubs with random texture indices. Verify that later usage of faces does not rely on stable textures or consistent geometry.
146-162: Check boundary conditions ingenerateRandom.
- You throw an error if the requested
countis not a perfect square, but confirm that this is the intended constraint.- The resulting blocks are assigned a random
modelIdunlessmodelis provided; ensure that indexing and memory usage are within acceptable bounds ifmodelIdcan climb to 3000.
201-207: Ensure removeBlocksSection cleans up properly.You call
webgpuRenderer?.chunksFadeAnimationController.removeIndex(key, ...). Confirm that once the animation completes, references to these block sets are fully removed from memory and the GPU buffers.package.json (3)
8-10: Scripts updated for concurrency and performance testing.
- Lines 8–10 add
watch-other-workersto bothstartandstart2. Ensure your system can handle these concurrent tasks without confusion, especially if the same watchers run in parallel.- The new script
test:cypress:perfon line 16 is specialized for performance testing. Confirm that you have Microsoft Edge or the specified browser installed during CI/CD.Also applies to: 16-16
26-27: Check the build pipeline for worker scripts.The scripts
watch-other-workersandbuild-other-workersrefer tobuildWorkers.mjs. If that file is critical, ensure version control includes it. Similarly,run-all2andwatch-playgroundmight be best documented if used externally or in CI.Also applies to: 32-32, 34-35
220-222: Assess the reason for never buildingflying-squid.Adding
"flying-squid"toneverBuiltDependenciesensures the build pipeline won’t bundle or transform it, but confirm that your runtime environment can handle its usage.src/shims/perf_hooks_replacement.js (1)
1-1: More cross-environment compatibility with globalThisChanging from
window.performancetoglobalThis.performanceis a great improvement as it makes the code work across different JavaScript environments, not just browsers.
globalThisprovides a standard way to access the global object regardless of the context (browser, Web Worker, Node.js), making this code more versatile and environment-agnostic.src/react/DebugOverlay.module.css (2)
14-14: Debug overlay repositioned verticallyIncreasing the top position from 1px to 25px moves the debug overlay further down from the top of the screen, which likely improves its visibility with the new WebGPU renderer.
19-19: Consistent positioning adjustment for right side debug overlaySimilar to the left side, the right side debug overlay has been moved down from 5px to 25px, ensuring consistent vertical alignment of both debug panels.
renderer/viewer/lib/mesher/test/mesherTester.ts (1)
22-22: Good defensive programming with optional chainingAdding the optional chaining operator (
?.) before accessingfaces.lengthproperly prevents potential runtime errors iffacesis undefined.This change improves code robustness by gracefully handling the case where
sectionGeometry.tiles[key].facesmight be undefined.src/styles.css (1)
190-197: New WebGPU debug UI styling looks goodThe CSS styling for the WebGPU debug interface positions it appropriately and makes the background transparent, which should help it integrate well with the rest of the UI.
These styles are aligned with the PR objective of implementing a new WebGPU rendering engine.
renderer/viewer/lib/mesher/mesher.ts (1)
144-147: Good implementation of WebGPU data handlingThe new case for handling 'webgpuData' messages follows the existing pattern consistently and integrates well with the mesher worker.
This change properly supports the new WebGPU renderer by forwarding data to the World object through the
setDataForWebgpuRenderermethod.src/react/CreateWorldProvider.tsx (1)
4-4: LGTM: Import path updated correctlyThe update to import from the new centralized location aligns with the project's reorganization of file system operations. This change enhances maintainability by centralizing related functionality.
src/react/AppStatusProvider.tsx (1)
74-81: Good refactoring: Animation logic properly encapsulatedExtracting the animation logic into a dedicated function improves code readability and maintainability.
src/localServerMultiplayer.ts (4)
7-7: Good modular refactoring approachThe import of
getLocalServerOptionsfrom the central module is a good architectural decision that helps decouple this file from direct server instance dependencies.
33-33: Improved version access using centralized functionUsing
getLocalServerOptions().versioninstead of directly accessinglocalServer!.options.versionenhances maintainability and follows the principle of single responsibility.
53-53: Consistent null check approachThis change properly uses the new function to check for server options availability, maintaining consistency with the refactoring pattern.
69-69: Improved client initializationUsing the centralized function for version access when creating the client is consistent with the refactoring pattern.
src/react/ButtonWithTooltip.tsx (4)
12-12: Good enhancement to tooltip interfaceAdding the
alwaysTooltipproperty improves component flexibility by allowing tooltips to be permanently displayed when needed.
18-18: Proper prop destructuring updateDestructuring
alwaysTooltipalongside other props maintains clean code and properly implements the interface change.
20-20: Enhanced tooltip visibility logicThe state initialization properly prioritizes
alwaysTooltipbefore falling back to localStorage-based logic.
71-71: Consistent tooltip content renderingThe rendering logic correctly prioritizes
alwaysTooltipoverinitialTooltip.content, maintaining consistency with the state logic.renderer/viewer/lib/basePlayerState.ts (1)
5-5: Adding Valtio's ref for immutable optimizationImporting
reffrom valtio is a good approach for optimizing reactivity of nested objects or arrays.src/react/SingleplayerProvider.tsx (1)
6-6: Good refactoring - moving file operations to a dedicated module.The change improves code organization by relocating filesystem-related functions from the general
browserfsmodule to a more specificintegratedServer/browserfsSharedmodule. This refactoring appears to be part of a larger effort to consolidate file system operations in the integrated server context.Also applies to: 12-12
src/resourcePack.ts (1)
7-7: Consistent refactoring of filesystem functions.This change aligns with the same pattern seen in other files, moving
mkdirRecursiveandremoveFileRecursiveAsyncto the dedicatedintegratedServer/browserfsSharedmodule while keepingcopyFilesAsyncWithProgressin the original location. This maintains a consistent approach across the codebase for organizing filesystem operations.Also applies to: 15-15
src/watchOptions.ts (2)
13-13: Good defensive initialization of global object.Using the nullish coalescing operator to initialize
globalThis.viewerensures the object exists before it's accessed elsewhere, preventing potential runtime errors.
92-93: Aligned renderer configuration with shared options.The change properly updates the rendering configuration to use the centralized
rendererSharedOptionsinstead of renderer-specific options, which aligns with the PR objective of implementing a new WebGPU rendering engine. This ensures consistent configuration across different renderers.cypress/e2e/performance.spec.ts (1)
3-25: Thorough performance test implementation.This test effectively measures rendering time for the singleplayer mode by waiting for the initial loader to disappear and then tracking performance with marks and measurements. The approach using custom events for timing is robust.
renderer/viewer/lib/webgpuShaders/RadialBlur/vert.wgsl (2)
2-5: LGTM! Properly structured vertex shader output.The VertexOutput struct correctly defines the output structure with proper builtin and location attributes.
8-18: Appropriate vertex shader implementation for fullscreen quad rendering.This vertex shader correctly transforms input vertices into a fullscreen quad by applying the sign function, which is an efficient way to create a quad covering the entire screen for post-processing effects like radial blur.
renderer/playground/scenes/floorStoneWorld.ts (1)
4-9: LGTM: Scene class with WebGPU renderer configuration.The class properties are well-defined with appropriate values for the scene. Setting
webgpuRendererParams = trueenables the new WebGPU renderer.src/water.ts (3)
13-13: Good use of optional chaining for safer access.Using optional chaining (
?.) helps prevent runtime errors when accessing properties of potentially undefined objects.
16-16: LGTM: Added water breathing status tracking.Adding
waterBreathingstate to track if the player has the water breathing effect is a good enhancement for gameplay mechanics.
36-36: Proper integration of reactive state management.Using
reffrom the valtio library ensures the background color is properly managed as a reactive state, consistent with other state properties..vscode/launch.json (2)
38-39: Filter removal in "Launch Chrome" configurationThe filter for
*mc-data*files has been removed from theoutFilesconfiguration. Ensure this is intentional, as it will now include these files in debugging sessions, potentially affecting debugging performance.
45-63: New configuration for WebGPU playgroundThis configuration is correctly set up to debug the WebGPU playground on a different port (9090) and directory structure. This aligns with the PR objective of implementing a new WebGPU renderer.
🧰 Tools
🪛 Biome (1.9.4)
[error] 63-63: Expected a property but instead found '}'.
Expected a property here.
(parse)
renderer/playground/scenes/layers.ts (2)
4-8: WebGPU renderer scene configurationThe scene class is properly configured with appropriate default values for the WebGPU renderer. Setting
webgpuRendererParamstotrueenables the WebGPU rendering pipeline as mentioned in the PR objectives.
20-21: Good validation for square sizeThe validation check for maximum square size is a good safeguard against performance issues when rendering large areas.
renderer/playground/TouchControls2.tsx (1)
9-9: Address TODO commentThere's a TODO comment without any details about what setting is needed.
Consider adding more details to the TODO or implementing the setting.
src/react/MainMenuRenderApp.tsx (2)
7-7: Good import optimizationThe import statement has been optimized to only include the specific functions needed, which is a good practice.
113-116: Improved URL handling with proper APIThe code now uses the URL API to properly handle query parameters instead of string concatenation, which is more robust and maintainable.
renderer/viewer/lib/mesher/modelsGeometryCommon.ts (1)
76-88: LGTM - Addition of side properties for WebGPU renderingThe addition of
sideandwebgpuSideproperties to theupface is a good enhancement for WebGPU rendering.renderer/playground/Cube.frag.wgsl (1)
1-11: LGTM! - Good binding and function implementationThe bindings for the sampler and texture, along with the fragment output structure and linearize_depth_ndc function, are well-defined and follow WebGPU best practices.
src/integratedServer/customClient.js (4)
1-5: Imports setup leverages EventEmitter for custom Minecraft protocol clientThe imports establish the foundation for implementing a custom client for Minecraft protocol communication, using EventEmitter for event-based architecture.
20-27: Clean implementation of factory function for client creationThe factory pattern used here allows for flexible instantiation of the CustomChannelClient with configurable data handling and event filtering.
43-51: State management looks goodThe state setter implementation properly maintains the old state reference, updates the protocol state, configures the serializer, and emits events to notify listeners of state changes.
58-66: Packet write implementation handles debugging wellThe write method intelligently filters debugging output using the provided excludeCommunicationDebugEvents array, which is a good practice for reducing noise in production environments.
src/integratedServer/browserfsServer.ts (3)
1-8: Comprehensive imports for filesystem operationsThe imports provide all necessary functionality for file operations, NBT parsing, and browser filesystem integration. Good to see the reuse of shared utilities from browserfsShared.
9-23: Effective implementation of level.dat readerThe function successfully handles error cases and parses NBT data correctly. Good error handling for the case when the file doesn't exist.
42-50: Filesystem backend mounting logic looks goodThe conditional logic for mounting different filesystem backends based on configuration is clean and follows good async/promise patterns.
src/appViewerLoad.ts (2)
9-15: Backend loading looks goodThe loadBackend function cleanly handles loading the appropriate renderer based on the active renderer option.
35-35: Starting options watching as expectedThe call to watchOptionsAfterViewerInit() ensures that option changes are properly tracked after viewer initialization.
src/utils.ts (3)
68-76: New logAction function is well-implementedThe function properly checks if external logging is enabled before attempting to send data, and it accepts all the necessary parameters for comprehensive event tracking.
154-156: Good defensive programming with optional chainingThe addition of optional chaining (
?.) to the bot.setSettings call prevents potential crashes if the bot is undefined.
162-164: Good defensive programming with optional chainingThe addition of optional chaining (
?.) to the worldView.updateViewDistance call prevents potential crashes if worldView is undefined.renderer/viewer/lib/webgpuShaders/RadialBlur/frag.wgsl (1)
59-70: Overshadowed variable and floating comparison.
- The
tempvariable is redeclared inside the loop and reused below, which can cause confusion in scoping.- Relying on
if (temp == 1.0)may be brittle due to floating-point imprecision. Consider a small epsilon comparison if you only intend to check whether a pixel is fully “far.”src/react/MainMenu.tsx (2)
104-105: Minor text change.
Updating the tooltip to "Create worlds to play offline" clarifies the feature.
131-140: New multiplayer button looks good.
The tooltip "Connect to Java servers!" and click action appear consistent with the existing design.src/createLocalServer.ts (1)
6-6: Undefined identifier concern.
mcDatais referenced but never imported or declared in this snippet. Verify thatmcDatais globally available or properly imported.renderer/viewer/webgpu/worldrendererWebgpu.ts (2)
26-29: Non-standard promise construction.
Promise.withResolvers()is not standard. Verify the availability or replace with a typical “new Promise” pattern to ensure cross-platform compatibility.
369-369: TODO reminder: Debug UI teardown.
Marked with// todo destroy. Implement a cleanup path to avoid stale references to GUI or stats resources.src/optionsStorage.ts (2)
30-30: Increased FOV from 75 to 90.This change increases the default Field of View from 75 to 90 degrees, which provides a wider perspective. This aligns well with the WebGPU renderer implementation, but ensure users are made aware of this change as it significantly alters the visual experience.
99-99: External logging enabled by default.Setting
externalLoggingServicetotrueby default might have privacy implications. Consider documenting the purpose of this setting and what data is being logged externally.src/worldSaveWorker.ts (2)
1-15: New web worker for chunk reading with proper initialization.The worker setup looks good with proper imports and BrowserFS configuration for IndexedDB storage.
16-19: Well-defined interface for worker communication.The
ReadChunksRequestinterface provides a clear contract for messaging between the main thread and this worker.renderer/playground/chunksStorage.test.ts (7)
1-6: Good test setup with proper error handling.The test imports are properly structured, and the global error handler ensures that errors are properly exposed during testing rather than silently caught.
7-44: Well-structured test data and helper functions.The test setup creates appropriate test data and helper functions to verify the behavior of the
ChunksStorageclass.The
getRangeStringandtestRangehelper functions are well-designed for validating the behavior of the chunk storage.
45-85: Comprehensive test for adding chunks and validation.The test correctly verifies the state of
chunksMap,chunks,findBelongingChunk, and range strings after adding chunks.
87-96: Proper validation of chunk removal logic.The test correctly verifies that removing a chunk marks it as free and updates the range strings appropriately.
98-128: Thorough testing of chunk reuse and updates.The test verifies that the
ChunksStoragecorrectly reuses a free chunk slot when adding a new chunk, and that thechunksMapandchunksarrays are updated appropriately.
130-159: Good testing of chunk update without map changes.This test ensures that updating an existing chunk doesn't change the chunk map structure, only the block values.
161-239: Complete verification of adding a new chunk when no free spaces exist.The test properly verifies that adding a new chunk creates a new entry in the
chunksarray and correctly updates thechunksMapand range strings.src/optionsGuiScheme.tsx (1)
116-122: Refactored renderer options structure.The code now correctly references
rendererSharedOptionsinstead of the previousrendererOptions.threepath, which aligns with the changes in theoptionsStorage.tsfile.renderer/viewer/webgpu/graphicsBackendWebgpu.ts (3)
1-3: Good import organization.
No issues spotted with these imports; they are concise and relevant.
5-7: Appropriate extension of GraphicsInitOptions.
DefiningWebgpuInitOptionsby extendingGraphicsInitOptionswith an optionalallowChunksViewUpdateproperty is clean and follows good TypeScript practices.
44-45: Neat assignment of static property to the function.
Exporting the function and settingcreateWebgpuBackend.id = 'webgpu'afterwards is a valid approach. Just ensure it’s documented for clarity since it might not be immediately obvious to new maintainers.src/controls.ts (3)
420-422: ConfirmfsState.inMemorySavePathexistence or handle null values.
IffsState.inMemorySavePathis undefined whilefsState.inMemorySaveistrue, calling.split(...)will throw an error. Add a safeguard for robust error handling.if (fsState.saveLoaded && fsState.inMemorySave) { + if (!fsState.inMemorySavePath) { + console.warn("inMemorySavePath is unexpectedly undefined") + return + } const worldFolder = fsState.inMemorySavePath const save = worldFolder.split('/').at(-1) newQs = `loadSave=${save}` }
702-702: Safe usage of async or void with function calls.
Usingvoid keybind.action()helps avoid unhandled Promise rejections ifactionis async. This approach is correct and keeps lint checks happy.
941-947: Useful debug toggle for sessionStorage.
Ensuring thatsessionStorage.delayLoadUntilFocusis toggled can help in debugging performance or load issues. This is generally fine if intentionally kept in dev code.renderer/playground/TextureAnimation.ts (2)
1-4: Straightforward control switches definition.
DefiningAnimationControlSwitcheswithtickandinterpolationTickcleanly separates concerns needed by your animation logic.
5-13: Good structure for animation data.
Dataensures a robust definition of frames, interpolation, and frametime. This helps keep the animation logic self-descriptive.renderer/playground/chunksStorage.ts (8)
5-8: Reasonable initial array fields.
Storing blocks inallBlocksalongsidechunksandchunksMapsets a clear foundation for chunked block management.
39-57:printSectionDatareturns helpful chunk info.
This method offers a clear structure for retrieving chunk data. Ensure that the caller gracefully handlesundefinedresults if the chunk isn’t found.
74-93: Scheduled block updates are well-managed via the queue.
getDataForBufferseffectively pops fromupdateQueuefor incremental updates. This design is effective for throttling large buffer changes.
100-104:clearDataeffectively resets the storage.
Clear usage, removing all references to chunks, blocks, and pending updates. No concerns here.
106-111: Good safeguard on chunk size.
replaceBlocksDataenforces a maximum chunk of 16×16×16. Just ensurenewData.lengthcovers vertical dimension as intended, if required by the game logic.
113-143: Reused or newly allocated chunks logic is sound.
getAvailableChunkreuses free chunks or appends a new one. This approach helps limit memory usage — be mindful of potential fragmentation if chunk merges are incomplete.
177-227: Comprehensive logic for adding new chunks.
CallsremoveChunkbefore adding new data, ensuring no conflicts. Consider verifying the coordinate math rigorously for alignment with chunk boundaries.
229-231: Queue-based approach for chunk updates.
requestRangeUpdateis a straightforward pattern for deferring or batching updates. Well done.src/index.ts (7)
55-55: Imports look good
No issues found with the new import.
73-73: Imports look good
No issues found with the new import.
92-92: No issues with the new import
It is used properly for progress reporting.
94-94: No issues with the new imports
They are used properly for local server lifecycle management.
201-201: Consider awaiting or handling rejections
IfdestroyLocalServerMain(false)is async, ignoring its promise can lead to unhandled exceptions.Please verify whether
destroyLocalServerMainis asynchronous. If it is, consider awaiting it or handling potential errors.
347-347: Assignment looks fine
Your approach to assigningCustomClientfromstartLocalServerMainis properly enclosed in a try/catch.
707-707: Awaiting the world start
Usingawaithere helps ensure the world is initialized before proceeding.src/loadSave.ts (4)
13-13: Imports appear correct
UsingexistsViaStats,initialFsState, andmkdirRecursiveis consistent with the updated code.
19-19: Check environment for structuredClone
Ensure that the runtime environment supportsstructuredClone. If not, an alternative (e.g.lodash.cloneDeep) might be needed.
107-107: Assigning forceCachedDataPaths
Storing player data inforceCachedDataPathsaligns with your in-memory approach.
146-146: forceRedirectPaths addition
Redirecting paths is consistent with your caching approach.src/react/PauseScreen.tsx (3)
24-24: Update to the import pathThe import path for filesystem utilities has been updated from
../browserfsto../integratedServer/browserfsShared, which indicates a potential architecture change.This change aligns with the project's migration of file system utilities to a more organized structure.
56-57: Simplified world folder path retrievalThe code now directly accesses
fsState.inMemorySavePathinstead of using a more complex approach with TypeScript error suppression.This is a good improvement that:
- Eliminates the need for TypeScript error suppression
- Makes the code more maintainable by using the state directly
- Follows the state management pattern used elsewhere in the application
298-298: Updated button text logicThe button text for disconnect now uses a more direct check of
fsState.inMemorySaveand related flags to determine when to show "Save & Quit".This change improves clarity by directly checking the relevant state properties rather than relying on more complex conditions.
renderer/playground/baseScene.ts (13)
23-23: Import looks good.
No issues found importingdefaultWebgpuRendererParamsandrendererParamsGui.
35-35: Initialization structure looks fine.
Storing initial parameter values inallParamsValuesInitis a clear approach.
96-112: Conditional parameter setup is appropriate.
Merging default renderer params intothis.paramsis well-structured. This approach is easy to maintain.
114-123: Query string parsing is consistent.
Overwriting params with query string values is clean. No major issues spotted.
160-171: WebGPU renderer params integration looks solid.
Dynamically applying renderer parameters and enabling orbit control is a neat approach.
190-193: Nice feature for reproducible views.
Locking the camera state into a URL parameter helps share or revisit the exact viewpoint.
196-196: OrbitControls reset is fine.
The optional chaining prevents errors if controls are disabled. This is safe and good.
232-235: Playground flags are appropriate.
Marking the scene and config as playground-specific is straightforward.
245-253: Renderer initialization looks good.
Setting blockstates models, debug mode, lighting, etc., fits the new pipeline. No issues spotted.
254-254: GUI initialization placement is fine.
InvokinginitGui()after the viewer properties are set ensures correct param handling.
279-279: Assigning controls is straightforward.
This ensures that orbit control references are maintained properly.
283-283: Camera parameter fallback is consistent.
Falling back tolocalStorage.cameraif not in query parameters is sensible for a persistent setup.
294-294: LocalStorage usage is fine.
Persisting camera states between sessions is a good UX enhancement.renderer/playground/scenes/floorRandom.ts (7)
1-1: Vec3 import is straightforward.
No issues with importing the Vec3 module.
8-8: Setting the target position to (0,0,0).
No issues spotted; this is a valid default origin.
12-13: New chunksDistance parameter.
Using a chunk-based distance is simpler and clearer for large worlds.
15-17: Reload on change is helpful.
Forcing a page reload to apply chunk distance changes is valid for big structural changes.
19-19: Callingsuper.initGui().
Ensures higher-level parameter logic stays in sync.
23-30:❓ Verification inconclusive
Random chunk generation logic.
Consider potential performance impact for large distances. Looks otherwise fine.
🏁 Script executed:
#!/bin/bash # Quick check for chunk generation calls that might hamper performance at scale rg 'webgpuChannel.generateRandom'Length of output: 382
Attention: Random Chunk Generation – Verify Performance Scaling
The random chunk generation logic in
renderer/playground/scenes/floorRandom.tsworks as intended and is consistent with similar patterns in the codebase (e.g., the commented-out call infloorStoneWorld.tsand the variant incubesHouse.ts). However, looping over a range determined bychunksDistancecan result in many calls togenerateRandomfor large distances, which may impact performance. Please verify that the performance remains acceptable as the grid scales.
- File:
renderer/playground/scenes/floorRandom.ts(lines 23–30)- Note: Similar random generation patterns exist in
cubesHouse.ts(with an extra parameter), so ensure consistency in performance expectations across different scenes.
6-6:❓ Verification inconclusive
Verify zero viewDistance.
A0distance might break or hide chunks. Confirm it’s intentional and tested.
🏁 Script executed:
#!/bin/bash # Searching for references to `viewDistance` usage. ast-grep --pattern $'$_ = $.viewDistance'Length of output: 42
I'll run a new search using
rgto locate any references toviewDistancethroughout the repository. This will help us confirm if theviewDistance = 0assignment is isolated or if there’s further related usage that might impact behavior. Please execute the following script and share its output for further verification:Once we have the results, we can determine whether the zero value is intentional and if additional testing is needed.
🏁 Script executed:
#!/bin/bash # Search for any occurrences of "viewDistance" in the repository. rg -n "viewDistance" .Length of output: 4376
Action: Confirm intentional zero viewDistance usage in this playground scene.
It appears that in the file
renderer/playground/scenes/floorRandom.ts, line 6 explicitly setsviewDistance = 0. A repository-wide grep shows that while some playground scenes (like those inbaseScene.tsand others) also use 0, other scenes use non-zero values (e.g., 16 or 5). Given that a zero view distance may prevent chunks from being rendered as expected, please verify that this value is intentionally set for this scene (for example, to test a no-chunk scenario or to disable chunk visibility). If this is deliberate, consider adding an inline comment to clarify the rationale; otherwise, adjust the value to prevent potential rendering issues.src/builtinCommands.ts (5)
5-6: Imports look aligned with the new state management approach.
No issues found.
11-11: Reorganized import tobrowserfsShared.
This reflects the broader refactor for file operations. Good consistency.
73-73: EnsurefsState.inMemorySavePathis properly initialized.
Previously, the path was retrieved fromlocalServer!.options. Confirm thatfsState.inMemorySavePathis never empty if world folder is required, or consider adding fallback/error handling.
145-145: Refined filter logic ingetBuiltinCommandsList.
Switching fromlocalServertomiscUiState.singleplayeris consistent with the updated UI state management.
150-150: Matching command availability check with singleplayer state.
Verifies that commands are not accessible outside singleplayer mode unlessalwaysAvailableis true. Looks intentional.renderer/viewer/lib/mesher/world.ts (9)
7-8: Imports updated to reference WebGPU model types.
No issues found; these new imports support the updated renderer pipeline.
10-10: AddedgetPreflatBlockimport.
Useful for fallback logic in older MC data versions.
31-31: NewpatchedModelsproperty forWorldBlock.
Facilitates storing alternate or corrected block models. Ensure usage is well covered by tests.
44-44: NewwebgpuModelsMappingproperty inWorld.
Helps map block states to model IDs for WebGPU rendering. Implementation aligns with the new pipeline.
48-51: Defined defaultliquidBlocksfor water and lava.
Recommend verifying thatmcData.blocks.find(...)always returns a valid block. If not found, consider falling back gracefully.
126-126: ExpandedgetBlocksignature to usePartial<MesherGeometryOutput>.
Provides more flexibility for passing mesher attributes. The approach seems correct.
145-145: InitializingpatchedModelsobject.
Ensures block instances support subsequent modifications for custom models.
157-162: Fallback logic for unknown blocks in pre-flat worlds.
Allows usage ofgetPreflatBlockas a patch. Verify rare edge cases for blocks not inmoreBlockDataGeneratedJson.
244-245:setDataForWebgpuRendererassignswebgpuModelsMapping.
Aligns block state with corresponding WebGPU models. Straightforward usage.src/integratedServer/main.ts (5)
1-5: Import statements introduce server/worker dependencies.
All imports align with the new worker-based local server structure.
6-19: Global references tofsStateand initial custom settings.
Ensure thatfsStatecan safely interact with the new worker. The default settings (autoSave,stopLoad) seem reasonable.
21-28: CustomaddEventListenerfor worker messages.
Implementation checkse.data.type. No immediate issues. Confirm no conflicts with other message listeners.
55-59:updateLocalServerSettingsmerges settings intolastCustomSettings.
If the worker isn’t initialized,serverChannelis undefined. Check that no exceptions occur in such a scenario.
60-98:startLocalServerMainspawns a worker and sets up the local server channel.
fsState.inMemorySavePathis reset byserverOptions.worldFolder. Confirm that is expected.- Consider assigning
lastOptions = serverOptionsto sync withgetLocalServerOptions().renderer/viewer/lib/worldrendererCommon.ts (5)
10-10: Good use of subscription utilities.
The import fromvaltio/utilslooks appropriate for reactive state updates within the file.
109-109: Structured worker-based counting.
DefininggeometryReceiveCountas an object keyed by worker index is clear and should work well in single-threaded JavaScript.
141-141: Clear numeric tracking for metrics.
geometryReceiveCountPerSecprovides a helpful way to measure geometry throughput per second. Good addition.
255-259: Reactive background color update looks good.
MonitoringbackgroundColorchanges viaonReactiveValueUpdatedis straightforward and ensures consistent updates to scene rendering.
908-908: Effective stats cleanup.
CallingremoveAllStats()indestroy()aligns with resource cleanup logic for stats.renderer/playground/webgpuBlockModels.ts (5)
1-8: Imports and shared constants.
The imports fromflying-squid/dist/utils,mc-assets/dist/worldBlockProviderand theWEBGPU_FULL_TEXTURES_LIMITconstant set the stage for efficient texture and block model handling. No issues found.
9-46: Initial setup for block data modeling.
The functionprepareCreateWebgpuBlocksModelsDatainitializes various data structures and fallback mappings likeblocksMap. ThevalidateTileIndexhelper ensures indices fall within allowable ranges. Consider offering a bit more context in error messages iftileIndexis out of range.
104-140: Fallback state and texture overrides.
Line 104 adds a default unknown block model with state-1; ensure it's documented or clearly used as a fallback. The texture override logic for liquids (water, lava) is proper.
142-150: Filtering out non-full blocks.
Skipping blocks whose shapes don't span the full voxel range is a feasible compromise for basic modeling. Make sure to revisit if partial-block geometry becomes relevant in future expansions.
151-157: Final data assembly.
CombiningblocksDataModel,allBlocksStateIdToModelIdMap, the set ofinterestedTextureTiles, and debug info is a solid approach for building WebGPU block data. Good job on returning them together.src/appViewer.ts (7)
52-52: Flexible interface design.
Using a generic type parameterSfor renderer-specific settings is a neat approach, letting different renderers specify their own config shape.
55-55: Renderer-specific settings.
DefiningrendererSpecificSettings: Scentralizes your renderer’s additional configuration in one place. Looks good.
70-70: Explicit loader identification.
Attaching anidproperty toGraphicsBackendLoaderclarifies which loader is in use for storing settings, ensuring a consistent naming pattern.
80-80: Optional async world startup.
AllowingstartWorldto returnPromise<void>orvoidgracefully accommodates synchronous or asynchronous initialization steps.
122-129: Dynamically collecting renderer settings.
BuildingrendererSpecificSettingsfrom globaloptionsis helpful. Confirm thatthis.backendLoader?.idis always set to avoid potential empty prefixes when reading keys.
137-140: Simple approach to persisting custom settings.
Writing back tooptions[rendererSettingsKey + '.' + key]effectively saves updated renderer-specific settings.
158-185: Async flow for world startup.
RefactoringstartWorldto async and awaiting the backend’sstartWorldcall ensures the world is reliably initialized before continuing. Returning!!promiseis a neat way to indicate which path was taken.renderer/viewer/lib/mesher/models.ts (2)
244-244: Good use of more specific block type.Switching from
BlocktoWorldBlockclarifies the usage of the block’s extended properties. This improves type safety and likely prevents bugs when accessing new fields.
555-559: Consistent usage ofrenderLiquidsignature.Using
'water'and'lava'as arguments aligns well with the revised function signature, enhancing readability and reducing ambiguity compared to a boolean parameter.renderer/playground/CubeSort.comp.wgsl (2)
1-6: Ensure memory ordering requirements are met with atomic operations.Defining
instanceCountasatomic<u32>is correct for concurrency, but confirm that the default WGSL atomic operations provide sufficient ordering semantics for your usage (e.g., if you need device-side synchronization).
36-44: Verify resource bindings align with your pipeline layout.Each
@groupand@bindingmust match the rendering pipeline’s descriptor set. If the pipeline references them differently, you may encounter runtime binding mismatches.src/browserfs.ts (2)
99-138: Validate remote FS mounting parameters.You set
fsState.usingIndexFileUrlandfsState.remoteBackendBaseUrland then callmountRemoteFsBackend. Ensure that:
- The URL is valid and reachable
- The JSON index structure is present and correct
202-203: Confirm whether “possiblyCleanHandle” is truly obsolete.This function call is commented out, potentially leaving behind references to a now-useless cleanup routine. Either reintroduce the call if needed or remove the function entirely.
renderer/playground/Cube.comp.wgsl (2)
36-38: Double-check linear depth function.The formula for
linearize_depth_ndccan vary depending on projection matrix conventions. Ensure the existing equation matches your near/far plane setup for accurate depth calculations.
41-87: Validate global invocation logic for all chunks.The compute shader processes chunks based on the global invocation ID. Verify that
arrayLength(&chunks)matches your intended chunk count to avoid out-of-bounds reads, and ensure large chunk arrays don’t exceed the workgroup dispatch limits.src/integratedServer/browserfsShared.ts (1)
294-303: Assess potential parallel I/O overhead.The current implementation deletes files in parallel via
Promise.all. While generally efficient, confirm that your environment can handle parallel I/O without performance or concurrency issues.renderer/playground/webgpuRenderer.ts (2)
139-147: Confirm twoGig limit feasibility.You're requesting large buffer limits (2,147,483,644 bytes). Ensure the target GPUs can fulfill these requirements or gracefully handle fallback cases to avoid allocation failures.
821-899: Validate chunk/cube indexing correctness.The logic packs block positions and attributes into static bit offsets. Ensure the bit shifts (e.g., 10 or 4 bits) align with the maximum coordinate ranges for x, y, z, and block ID. Out-of-range values could silently corrupt data.
| "label": "webgl-worker", | ||
| "type": "shell", | ||
| "command": "node buildWorkers.mjs -w", | ||
| "problemMatcher": "$esbuild-watch", | ||
| "presentation": { | ||
| "reveal": "silent" | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Added WebGL worker build task.
The new WebGL worker task is a good addition for the WebGPU renderer implementation. However, there's a potential syntax error in the JSON structure.
Fix the trailing comma issue at line 35:
"presentation": {
"reveal": "silent"
- },
+ }This will resolve the parse error and ensure the tasks.json file is valid.
📝 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.
| "label": "webgl-worker", | |
| "type": "shell", | |
| "command": "node buildWorkers.mjs -w", | |
| "problemMatcher": "$esbuild-watch", | |
| "presentation": { | |
| "reveal": "silent" | |
| }, | |
| }, | |
| "label": "webgl-worker", | |
| "type": "shell", | |
| "command": "node buildWorkers.mjs -w", | |
| "problemMatcher": "$esbuild-watch", | |
| "presentation": { | |
| "reveal": "silent" | |
| } | |
| }, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 35-35: Expected a property but instead found '}'.
Expected a property here.
(parse)
| if (action === 'Dump loaded chunks') { | ||
| // viewer.world.exportLoadedTiles() | ||
| } |
There was a problem hiding this comment.
Incomplete implementation of "Dump loaded chunks"
The handler for the "Dump loaded chunks" action contains a commented-out function call, indicating incomplete functionality.
Complete the implementation by uncommenting and ensuring the function is properly defined:
if (action === 'Dump loaded chunks') {
- // viewer.world.exportLoadedTiles()
+ if (typeof viewer !== 'undefined' && viewer.world && typeof viewer.world.exportLoadedTiles === 'function') {
+ try {
+ viewer.world.exportLoadedTiles();
+ showNotification('Chunks exported successfully', 'Check your downloads folder');
+ } catch (err) {
+ console.error('Failed to export chunks:', err);
+ showNotification('Failed to export chunks', err.message);
+ }
+ } else {
+ showNotification('Export not available', 'This feature is not supported in the current context');
+ }
}📝 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.
| if (action === 'Dump loaded chunks') { | |
| // viewer.world.exportLoadedTiles() | |
| } | |
| if (action === 'Dump loaded chunks') { | |
| if (typeof viewer !== 'undefined' && viewer.world && typeof viewer.world.exportLoadedTiles === 'function') { | |
| try { | |
| viewer.world.exportLoadedTiles(); | |
| showNotification('Chunks exported successfully', 'Check your downloads folder'); | |
| } catch (err) { | |
| console.error('Failed to export chunks:', err); | |
| showNotification('Failed to export chunks', err.message); | |
| } | |
| } else { | |
| showNotification('Export not available', 'This feature is not supported in the current context'); | |
| } | |
| } |
|
|
||
| setInterval(() => { | ||
| this.geometryReceiveCountPerSec = Object.values(this.geometryReceiveCount).reduce((acc, curr) => acc + curr, 0) | ||
| this.geometryReceiveCount = {} | ||
| this.updateChunksStats() | ||
| }, 1000) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential memory leak from interval.
A setInterval is created but no reference is stored to clear it later. If multiple renderer instances are created, this may lead to memory leaks. Consider storing the interval ID as a class property and calling clearInterval in destroy().
private geometryStatsInterval?: NodeJS.Timeout
constructor(...) {
...
+ this.geometryStatsInterval = setInterval(() => {
this.geometryReceiveCountPerSec = Object.values(this.geometryReceiveCount).reduce((acc, curr) => acc + curr, 0)
this.geometryReceiveCount = {}
this.updateChunksStats()
}, 1000)
}
destroy() {
...
+ if (this.geometryStatsInterval) clearInterval(this.geometryStatsInterval)
}📝 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.
| setInterval(() => { | |
| this.geometryReceiveCountPerSec = Object.values(this.geometryReceiveCount).reduce((acc, curr) => acc + curr, 0) | |
| this.geometryReceiveCount = {} | |
| this.updateChunksStats() | |
| }, 1000) | |
| class WorldRendererCommon { | |
| // Add an instance variable to hold the interval ID. | |
| private geometryStatsInterval?: NodeJS.Timeout | |
| constructor(...) { | |
| // ... other constructor code ... | |
| // Store the interval ID to later clear it when needed. | |
| this.geometryStatsInterval = setInterval(() => { | |
| this.geometryReceiveCountPerSec = Object.values(this.geometryReceiveCount) | |
| .reduce((acc, curr) => acc + curr, 0) | |
| this.geometryReceiveCount = {} | |
| this.updateChunksStats() | |
| }, 1000) | |
| } | |
| destroy() { | |
| // ... other destruction/cleanup code ... | |
| // Clear the interval to prevent a potential memory leak. | |
| if (this.geometryStatsInterval) clearInterval(this.geometryStatsInterval) | |
| } | |
| } |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
Deployed to Vercel Preview: https://prismarine-ea59l3alk-zaro.vercel.app |
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.
src/core/timers.ts
Outdated
| @@ -124,12 +124,12 @@ export const preventThrottlingWithSound = () => { | |||
| // Start playing | |||
| 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 |
WORKING PLAYGROUND DEMO - PERF CHUNKS
LATEST MAP DEMO (Future City)
tasks:
@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.