feat: app <-> renderer REWORK. Add new layers for stability #315
Conversation
Review or Edit in CodeSandboxOpen the branch in Web Editor • VS Code • Insiders |
WalkthroughThis pull request removes several legacy viewer modules and refactors core rendering functionality. The changes centralize rendering by replacing the old Viewer class with a new WorldRendererThree architecture. New modules for sound, document rendering, VR, and backend management have been introduced. Additionally, resource and state management has been consolidated under an AppViewer class with updates to API signatures, event handling, and import paths. Minor UI and debugging enhancements and adjustments to configuration settings are also included. Changes
Sequence Diagram(s)sequenceDiagram
participant WR as WorldRendererThree
participant TS as ThreeJsSound
participant Loader as THREE.AudioLoader
participant Scene as THREE.Scene
WR->>TS: playSound(position, path, volume, pitch)
TS->>Loader: Load audio asynchronously
Loader-->>TS: Return audio buffer
TS->>Scene: Attach audio to listener and play
TS->>TS: Monitor playback and clean up resources
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
PR Code Suggestions ✨Explore these optional code suggestions:
|
|||||||||||||||||||||||||
|
TLDR
@Phoenix616 do you think its a good change? given it changes a lot of code I will try to merge it asap |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (2)
src/index.ts (1)
366-370: 🛠️ Refactor suggestionCover errors in resource loading.
Duringawait appViewer.resourcesManager.loadSourceData(version), failures can occur for missing or corrupted files. Include a try/catch to handle these gracefully without halting the entire connection process.src/resourcePack.ts (1)
314-323: 🛠️ Refactor suggestionHandle undefined
currentResourcescautiously.
Line 315 usesappViewer.resourcesManager.currentResources!, forcing non-null. IfresourcesManagerorcurrentResourcesis not set at runtime, this can cause runtime errors. Add guards (or a fallback) to ensure stability.
🧹 Nitpick comments (32)
renderer/viewer/three/panorama.ts (2)
30-30: Validate abort controller usage in constructor.Defining
abortControllerat construction is helpful for cleanup, but ensure it’s canceled or reset if this instance can be reused or re-initialized. If re-initialization occurs often, consider exposing an API to recreate the abort controller for each new rendering session.
130-130: ESM vs. CommonJS mismatch.Using
require('prismarine-block')can cause bundling issues if the rest of the code uses ES modules. Consider migrating to ESM imports consistently for a more uniform build.renderer/viewer/lib/threeJsSound.ts (1)
17-46: Consider adding error handling for audio loading failures.The
playSoundmethod handles timeout cases but doesn't have explicit error handling for audio loading failures. If the audio fails to load for reasons other than timeout, there might be silent failures.void audioLoader.loadAsync(path).then((buffer) => { if (Date.now() - start > 500) return // play sound.setBuffer(buffer) sound.setRefDistance(20) sound.setVolume(volume) sound.setPlaybackRate(pitch) // set the pitch this.worldRenderer.scene.add(sound) // set sound position sound.position.set(position.x, position.y, position.z) sound.onEnded = () => { this.worldRenderer.scene.remove(sound) sound.disconnect() this.activeSounds.delete(sound) audioLoader.manager.itemEnd(path) } sound.play() +}).catch((error) => { + console.error(`Failed to load sound: ${path}`, error) + this.activeSounds.delete(sound) + audioLoader.manager.itemError(path) })src/rendererUtils.ts (1)
94-94: Usage ofappViewer.backend?.updateCameralooks good.
This new approach updates the camera using thebackendinterface, consistent with the refactored rendering separation. Verify thatbot.entityis always defined at this point to avoid runtime errors.renderer/viewer/three/documentRenderer.ts (3)
9-52: Consider validating creation errors more granularly.
In the constructor, the try-catch block for the WebGL context creation is good practice. Consider logging whether running in WebGL1 vs. WebGL2 for detailed diagnostics, especially if the user’s hardware only supports WebGL1.
53-59: Pixel ratio precision check.
The logic here forces a pixel ratio of 1 for WebGL1, which can lead to blurry rendering on high-DPI displays. Consider allowing a user-configurable override or fallback ratio that’s less thandevicePixelRatiobut still not forced to exactly 1.
72-81: Validate FPS drop calculation usage.
this.droppedFpsPercentageis assigned but not visibly consumed. If your design intends to track or display it, ensure there's a follow-up or a binding to handle the value. Otherwise, consider removing it to clean up the code.src/devtools.ts (1)
152-152: ConfirmholdingBlocklifecycle.
When settingholdingBlockon a global object, ensure that references tothis.holdingBlockremain valid throughout the rendering lifecycle. Ifwindow.worldchanges or re-initializes, a stale reference might lead to inconsistent behavior.src/resourcesManager.ts (3)
42-44: Consider extensibility in event definitions.
TheResourceManagerEventstype defining onlyassetsTexturesUpdatedis fine for now, but you may want to plan for future events such as a “resourcesLoadingFailed” or “resourcesLoadingStarted.”
95-183: Check concurrency and partial loading states when updating assets.
The core logic performs several async steps—loading source data, merging custom textures, and building atlases. If the game triggers multiple calls toupdateAssetsDatain quick succession, consider guarding against race conditions (e.g., theabortControlleris used, but always confirm earlier calls are properly disposed/cancelled).+ // Example check before starting a new update: + if (!this.abortController.signal.aborted) { + this.abortController.abort(); + this.abortController = new AbortController(); + }
185-194: Allow optional on-demand debug images.
Downloading debug atlases is a great debugging feature. Consider controlling access with a debug or admin-only setting in production builds to avoid unintentional large file downloads.renderer/viewer/lib/entities.ts (2)
248-248: InjectWorldRendererThreefor stronger coupling to rendering logic.
Replacing a generic viewer parameter with aWorldRendererThreereference simplifies entity rendering. However, remain cautious if you ever need an abstraction layer—this tight coupling to a specific renderer may limit future backend changes.
352-352: Conditional ear rendering.
renderEarsreferencesthis.worldRenderer.worldRendererConfig; ensure it remains valid across changes. You might also consider a fallback if the config is missing, preventing null or undefined accesses.renderer/viewer/three/graphicsBackend.ts (3)
16-17: Avoid global references unless absolutely necessary.
Storing the renderer inglobalThis.rendererandwindow.panoramaRenderercan lead to potential side effects and difficulties in debugging. Consider limiting these to a single debugging file or pass them as function arguments where possible.
35-39: ExpandprepareResourcesor document its minimal implementation.
Currently,prepareResourcesonly sets a localversionand does not handle progress reporting or resource loading. If additional logic is planned, add a TODO comment or partial implementation to guide future maintainers.
54-66: Ensure robust cleanup indisconnect().
This method disposes renderers and sets them tonull; confirm that there are no event listeners or references left attached to the dom or other modules, which could lead to memory leaks.src/index.ts (2)
124-127: Consider lazy initialization inloadBackend().
If backend creation is expensive, you could wrap it in a lazy loader or check if it's already loaded. This helps avoid redundant calls when transitions happen (e.g., repeated user actions).
220-232: Validate session storage delay logic.
The session-based “delayLoadUntilFocus” approach can block the game load if the user never focuses the tab. Confirm this is intended or consider a maximum timeout/autostart fallback.src/resourcePack.ts (2)
366-378: Ensure consistent error handling in blockstates/models reading.
You resetresources.customBlockStatesandresources.customModelstoundefinedif reading fails. Confirm you also handle partial reads or partial failures—some files might be valid while others are missing.
567-569: Clarify usage ofresourcepackReload.
resourcepackReloadcallsupdateTextures, but if the environment or resources have changed drastically (e.g., version mismatch), the function might need additional logic. Document or handle version changes more explicitly.src/watchOptions.ts (4)
19-25: Add explicit parameter types forupdateTouch.
While the logic looks correct, consider adding a type definition for parameteroto maintain clarity and catch potential type errors in TypeScript.
44-51: Use enumeration or constants instead of magic numbers.
Using numeric values (0,1,2) to represent stats-visibility modes is fine but consider a small enum or named constants to make the code more descriptive and robust.
89-89: Maintain consistency in watchers.
You're mixingwatchValueandsubscribeKeyfor differentoptionsproperties. While this works, consider standardizing on one approach for harmony and simpler maintenance.Also applies to: 91-91
107-108: Commented-out code forneighborChunkUpdates.
Leaving commented code for reference is typically fine in an experimental feature. If you don’t plan to resurrect it soon, consider removing to keep the file clean.src/react/DebugOverlay.tsx (1)
150-150: Left debug overlay display.
This neatly replaces the old custom entries approach. Just keep in mind how frequentlygetDebugOverlay()might be called, to avoid performance overhead in heavy frames.src/appViewer.ts (1)
46-62:GraphicsBackendinterface.
Method signatures are clear, and the return types are well-defined. Consider ensuringgetDebugOverlayreturns a typed object to reduce casting overhead.renderer/viewer/lib/worldrendererCommon.ts (5)
6-25: Consider removing unused imports and constants.
The newly added imports forAtlasParser,ResourcesManager,DisplayWorldOptions,WorldDataEmitter,WorldRendererThree, andSoundSystemappear relevant. However,const appViewer = undefined(line 25) seems unused. Removing it may help keep the codebase clear of placeholders if it’s not necessary for upcoming changes.
64-64: Validate negative-Y handling.
The defaultworldSizeParamsrestricts the minimum Y to 0, even though modern Minecraft versions can have negative world depths. If you plan to support newer world formats, consider making this dynamic or clarifying that negative depths are out of scope.
347-350: Consider capturing resource loading errors.
await this.resourcesManager.loadMcData(version)andupdateAssetsData(...)can throw. Currently, there’s no error fallback. Ensure that upstream calls or the caller handle these exceptions to prevent silent failures during version or asset initialization.
372-401: Ensure workers are properly initialized before asset updates.
InupdateAssetsData(), throwing an error ifthis.workers.length === 0might be abrupt in asynchronous scenarios. Depending on load sequences, you may wish to queue or defer asset updates until workers are confirmed.
498-498: Remove or implement empty method.
updateEntity(e: any)is currently a no-op. If it’s just a placeholder, remove it or add a TODO comment to clarify future usage.renderer/viewer/lib/worldrendererThree.ts (1)
535-547: Camera roll feature is a nice addition.
setCameraRollmodifies the camera quaternion for immersive rotation. Keep an eye out for potential conflicts with other camera transformations—testing advanced camera interactions (like VR) helps ensure no unintended overrides.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (47)
renderer/viewer/index.js(0 hunks)renderer/viewer/lib/entities.ts(10 hunks)renderer/viewer/lib/guiRenderer.ts(1 hunks)renderer/viewer/lib/threeJsSound.ts(1 hunks)renderer/viewer/lib/viewer.ts(0 hunks)renderer/viewer/lib/viewerWrapper.ts(0 hunks)renderer/viewer/lib/worldDataEmitter.ts(2 hunks)renderer/viewer/lib/worldrendererCommon.ts(11 hunks)renderer/viewer/lib/worldrendererThree.ts(12 hunks)renderer/viewer/three/documentRenderer.ts(1 hunks)renderer/viewer/three/graphicsBackend.ts(1 hunks)renderer/viewer/three/panorama.ts(1 hunks)src/appViewer.ts(1 hunks)src/botUtils.ts(1 hunks)src/cameraRotationControls.ts(1 hunks)src/chatUtils.ts(1 hunks)src/connect.ts(1 hunks)src/controls.ts(1 hunks)src/core/progressReporter.ts(2 hunks)src/dayCycle.ts(1 hunks)src/devtools.ts(3 hunks)src/entities.ts(2 hunks)src/globals.d.ts(1 hunks)src/index.ts(12 hunks)src/inventoryWindows.ts(3 hunks)src/loadSave.ts(1 hunks)src/mineflayer/cameraShake.ts(2 hunks)src/mineflayer/playerState.ts(1 hunks)src/mineflayer/plugins/mouse.ts(1 hunks)src/mineflayer/plugins/ping.ts(1 hunks)src/optimizeJson.ts(1 hunks)src/panorama.ts(0 hunks)src/react/BookProvider.tsx(1 hunks)src/react/DebugOverlay.tsx(5 hunks)src/react/HotbarRenderApp.tsx(3 hunks)src/react/IndicatorEffectsProvider.tsx(1 hunks)src/react/MinimapProvider.tsx(1 hunks)src/reactUi.tsx(1 hunks)src/rendererUtils.ts(2 hunks)src/resourcePack.ts(4 hunks)src/resourcesManager.ts(2 hunks)src/shims/minecraftData.ts(1 hunks)src/sounds/botSoundSystem.ts(1 hunks)src/sounds/soundsMap.ts(1 hunks)src/topRightStats.ts(0 hunks)src/vr.ts(1 hunks)src/watchOptions.ts(3 hunks)
💤 Files with no reviewable changes (5)
- renderer/viewer/index.js
- renderer/viewer/lib/viewer.ts
- src/topRightStats.ts
- renderer/viewer/lib/viewerWrapper.ts
- src/panorama.ts
🧰 Additional context used
🧬 Code Definitions (13)
src/globals.d.ts (1)
src/appViewer.ts (1) (1)
appViewer(161-161)
src/cameraRotationControls.ts (1)
src/appViewer.ts (1) (1)
appViewer(161-161)
src/rendererUtils.ts (1)
src/appViewer.ts (1) (1)
appViewer(161-161)
renderer/viewer/three/graphicsBackend.ts (4)
src/appViewer.ts (7) (7)
GraphicsBackendLoader(46-46)GraphicsInitOptions(33-38)startPanorama(113-121)prepareResources(123-127)startWorld(129-144)DisplayWorldOptions(40-44)GraphicsBackend(48-61)renderer/viewer/three/panorama.ts (1) (1)
PanoramaRenderer(23-204)renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(23-548)src/core/progressReporter.ts (1) (1)
ProgressReporter(5-18)
src/mineflayer/cameraShake.ts (1)
src/appViewer.ts (1) (1)
appViewer(161-161)
renderer/viewer/lib/entities.ts (1)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(23-548)
src/resourcePack.ts (4)
src/appViewer.ts (1) (1)
appViewer(161-161)renderer/viewer/lib/entity/armorModels.ts (1) (1)
armorTextures(19-35)src/mineflayer/plugins/mouse.ts (1) (1)
bot(142-149)renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(23-548)
src/inventoryWindows.ts (1)
src/appViewer.ts (1) (1)
appViewer(161-161)
renderer/viewer/lib/worldrendererCommon.ts (3)
src/appViewer.ts (2) (2)
appViewer(161-161)DisplayWorldOptions(40-44)src/resourcesManager.ts (1) (1)
ResourcesManager(75-201)renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(21-249)
src/watchOptions.ts (2)
src/globalState.ts (1) (1)
miscUiState(109-129)src/optionsStorage.ts (2) (2)
watchValue(213-235)options(186-191)
src/react/DebugOverlay.tsx (1)
src/appViewer.ts (1) (1)
appViewer(161-161)
renderer/viewer/lib/worldrendererThree.ts (3)
renderer/viewer/lib/entities.ts (1) (1)
Entities(209-1028)src/appViewer.ts (2) (2)
GraphicsInitOptions(33-38)DisplayWorldOptions(40-44)renderer/viewer/lib/hand.ts (1) (1)
getMyHand(5-35)
src/vr.ts (1)
src/appViewer.ts (1) (1)
appViewer(161-161)
🪛 GitHub Check: build-and-deploy
renderer/viewer/three/panorama.ts
[failure] 9-9:
Cannot find module '../lib/viewer' or its corresponding type declarations.
src/react/HotbarRenderApp.tsx
[failure] 113-113:
Property 'itemsAtlasParser' does not exist on type 'ResourcesManager'.
src/inventoryWindows.ts
[failure] 49-49:
Property 'itemsAtlasParser' does not exist on type 'ResourcesManager'.
🪛 ESLint
renderer/viewer/lib/worldrendererThree.ts
[error] 314-314: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (88)
src/mineflayer/plugins/mouse.ts (1)
146-146: Double-check removal of display manager.By commenting out
createDisplayManager, the display manager responsible for block breaking visuals is disabled. If these visuals are still desired, consider providing an alternative or confirming that this functionality is no longer needed.renderer/viewer/lib/guiRenderer.ts (1)
5-5: Consolidated import looks good.The unified import of both
getLoadedImageandversionToNumberfrommc-assets/dist/utilsreduces confusion and improves maintainability. No issues spotted.renderer/viewer/three/panorama.ts (1)
199-204: Properly dispose resources.The
dispose()method clears the scene, aborts ongoing processes, and disposes the world renderer. This is a good pattern for preventing memory leaks. Confirm that any existing event listeners on global objects (likedocument.body) are also removed to avoid side effects.renderer/viewer/lib/threeJsSound.ts (3)
5-8: Well-designed interface for sound system.The
SoundSysteminterface provides a clean abstraction with just two essential methods:playSoundfor audio playback anddestroyfor resource cleanup. This design allows for different sound system implementations while maintaining a consistent API.
10-15: Good implementation of the SoundSystem interface.The
ThreeJsSoundclass properly implements theSoundSysteminterface and maintains a clear state with theaudioListenerandactiveSoundsproperties. The constructor accepts aworldRendererwhich follows dependency injection principles.
48-60: Comprehensive resource cleanup in destroy method.The
destroymethod properly stops all active sounds, disconnects them from the audio context, and cleans up the audio listener. This prevents memory leaks and ensures complete cleanup of audio resources.src/globals.d.ts (1)
15-15: Global appViewer declaration added.The declaration adds a new global constant
appViewerwhich aligns with the PR objective of creating a new stable layer to separate the application code from the graphics code. The declaration is properly typed with an import from './appViewer'.src/loadSave.ts (1)
6-6: Updated import path for versionToNumber utility.The import path has been changed from 'renderer/viewer/prepare/utils' to 'renderer/viewer/common/utils' as part of the broader code reorganization. The function reference remains the same, so functionality shouldn't be affected.
src/sounds/botSoundSystem.ts (2)
2-2: Updated import path for versionToNumber utility.The import path has been changed from 'renderer/viewer/prepare/utils' to 'renderer/viewer/common/utils' as part of the broader code reorganization. The function reference remains the same, so functionality shouldn't be affected.
49-54:❓ Verification inconclusive
Verify compatibility with new AppViewer structure.
This code references the global
viewerobject to play sounds, but the PR is introducing a newappViewerstructure. Make sure this integration point is properly updated or addressed in related changes.
🏁 Script executed:
#!/bin/bash # Search for the definition of the playSound method in the viewer/appViewer classes # and check how it's being integrated with the sound system. echo "Searching for playSound method definitions:" rg -A 2 "playSound.*=.*function" --glob "*.ts" rg -A 2 "playSound.*\(.*\).*{" --glob "*.ts" echo "Checking for viewer references in sound-related code:" rg "viewer\.playSound" --glob "*.ts" echo "Checking for appViewer sound-related references:" rg "appViewer\..*[sS]ound" --glob "*.ts"Length of output: 657
Sound Integration Check: Confirm Use of New AppViewer Structure
It appears that the current implementation in
src/sounds/botSoundSystem.tsstill invokesviewer.playSound(lines 49–54). Our recent code search did not reveal any references to anappViewerhandling sound, which raises the question of whether the newappViewerstructure is intentionally omitted here or if the integration should be updated. Please verify that:
- The use of the global
vieweris deliberate (e.g., for backwards compatibility) or if it should be replaced withappViewerto align with the new architecture.- Any related changes for the new sound integration in
appViewerare correctly applied elsewhere in the PR.src/botUtils.ts (1)
1-1: Import path updated to reflect new module structure.The import path has been updated from
'renderer/viewer/prepare/utils'to'renderer/viewer/common/utils', which aligns with the architectural changes introduced in this PR to separate application code from graphics code.src/cameraRotationControls.ts (1)
40-40:✅ Verification successful
Camera update timestamp property moved to AppViewer.
The camera update timestamp is now stored in
appViewer.lastCamUpdateinstead ofviewer.world.lastCamUpdate, which aligns with the new architecture that separates application code from graphics code.However, I notice that line 42 still references
viewer.world.freeFlyMode. Let's check if this is intentional or if other viewer references should also be updated:
🏁 Script executed:
#!/bin/bash # Find all remaining references to viewer.world in the codebase rg "viewer\.world" --type tsLength of output: 10077
Update: Camera timestamp and free fly mode usage verified
- The camera update timestamp property has been successfully migrated to
appViewer.lastCamUpdateinsrc/cameraRotationControls.ts(line 40) as intended.- The search confirms that while the timestamp has moved, references to
viewer.world(includingfreeFlyModeand related properties) remain widespread across the codebase (e.g., insrc/controls.tsand others). This indicates that leavingviewer.world.freeFlyModein place (line 42) is intentional and consistent with the current architecture separating application and graphics responsibilities.src/reactUi.tsx (2)
56-62: Added Firefox-specific styling.Browser detection for Firefox has been added with a custom CSS property to adjust styling specifically for Firefox browsers. This is a good practice for handling browser-specific UI adjustments.
64-68: Added iPhone-specific bottom HUD spacing.Device detection for iPhone has been added to adjust the bottom HUD spacing, likely to account for the notch/home indicator on iPhones. There's a TODO comment about extending this to iPads.
Could you test this on different iPhone models to ensure the 21px value works universally? Also, consider addressing the TODO to determine if this adjustment should apply to iPads as well.
src/react/BookProvider.tsx (1)
1-1: Import path updated correctly.The import of
versionToNumberhas been updated to use the new common utils location, which is consistent with the architectural changes in this PR.src/chatUtils.ts (1)
5-5: Import path updated correctly.The import of
versionToNumberhas been updated to use the new common utils location, which aligns with the architectural refactoring in this PR.src/optimizeJson.ts (1)
1-1: Import path updated correctly.The import of
versionToNumberhas been updated to use the new common utils location, which is consistent with the architectural changes being made across the codebase.src/mineflayer/plugins/ping.ts (1)
1-1: Import path updated correctly.The import of
versionToNumberhas been updated to use the new common utils location, which aligns with the architectural refactoring in this PR.src/mineflayer/playerState.ts (1)
43-43:❓ Verification inconclusive
Constructor visibility changed from private to public.
The constructor for PlayerStateManager is now public, which changes the instantiation control pattern. This aligns with the architectural refactoring goals of the PR to make components more modular and independently instantiable.
However, keep in mind this change could potentially lead to multiple instances of PlayerStateManager being created outside the singleton pattern, which might cause unexpected behavior if the class is not designed to handle multiple instances properly.
🏁 Script executed:
#!/bin/bash # Check if PlayerStateManager is instantiated directly in other files after this change rg "new PlayerStateManager\(\)" --type tsLength of output: 120
Action Required: Verify Controlled Instantiation of PlayerStateManager
After running the search, the only occurrence of a direct instantiation (
new PlayerStateManager()) was found insrc/mineflayer/playerState.ts(where it is assigned tothis.instance). This indicates that, at the moment, the singleton instantiation is maintained internally. However, now that the constructor is public, any future external calls tonew PlayerStateManager()could unintentionally create additional instances. Please ensure that:
- The public constructor is documented clearly to indicate that instantiation should only occur within controlled contexts.
- External parts of the codebase avoid instantiating PlayerStateManager directly.
- The singleton behavior is either safeguarded within the class design or additional checks are put in place to prevent multiple uncontrolled instances.
src/sounds/soundsMap.ts (1)
3-3: Import path updated appropriatelyThe import path for utility functions has been updated from 'renderer/viewer/prepare/utils' to 'renderer/viewer/common/utils' as part of the broader refactoring to separate application code from rendering code.
src/shims/minecraftData.ts (1)
1-1: Import path updated correctlyThe import path for versionToNumber has been updated from 'renderer/viewer/prepare/utils' to 'renderer/viewer/common/utils', consistent with the architectural changes being made across the codebase.
src/react/MinimapProvider.tsx (1)
5-5: Import path updated consistentlyThe import path has been correctly updated to use the new module location for utility functions, maintaining consistency with other files in this PR.
src/vr.ts (1)
194-195: Camera control updated to use the new AppViewer architectureThe code has been updated to use the new AppViewer backend for camera control instead of the deprecated viewer implementation. This change now passes the bot's actual yaw and pitch values instead of using a calculated yawOffset, which should provide more accurate camera positioning in VR mode.
Please verify that VR camera orientation works correctly with this change, as you've commented out the original implementation that used yawOffset. The current implementation might behave differently since it's using the bot's actual orientation values.
src/controls.ts (2)
929-931: Updated keyboard shortcut refactor is well implemented.The updated condition for the 'KeyL' keyboard shortcut adds proper modifier key checks, making the clear console command more deliberate and less likely to be triggered accidentally.
932-938:✅ Verification successful
Good addition of a developer feature to control resource loading.
This new keyboard shortcut adds a useful debugging capability that aligns with the PR's objective of enhancing performance. The toggle implementation for the session storage item is clean and follows the existing code patterns.
🏁 Script executed:
#!/bin/bash # Check where the session storage flag is used in the codebase rg "delayLoadUntilFocus" -A 5 -B 5Length of output: 1148
Approved: Clean Implementation for Resource Loading Toggle
The new key binding consistently toggles the debugging flag for delayed resource loading. Our search confirmed that the
sessionStorage.delayLoadUntilFocusflag is used both insrc/controls.tsand insrc/index.ts, aligning with the intended behavior. The implementation is clear, follows existing patterns, and integrates well with the connect logic.
- Verified usage in both
src/controls.tsandsrc/index.ts- Consistent toggle behavior for the session storage flag
src/connect.ts (1)
46-47:❓ Verification inconclusive
Function signature simplified appropriately.
Removing the
importBlockstatesModelsparameter aligns with the PR's goal of separating app code from renderer code. The block states models loading is likely moved to the new ResourcesManager mentioned in the PR objectives.However, let's verify there are no usages of this function with the removed parameter:
🏁 Script executed:
#!/bin/bash # Check for any calls to loadMinecraftData with a second parameter rg "loadMinecraftData\(.*,.+\)" --type tsLength of output: 43
Action: Verify that no call sites pass a second parameter to loadMinecraftData
The function signature now accepts only a single parameter (
version), which aligns with the PR’s objective of decoupling app code from renderer responsibilities (the block states models loading has been moved to the new ResourcesManager). An automated search usingrg "loadMinecraftData\(.*,.+\)" --type tsdid not return any results. However, since the output was minimal and might not be exhaustive, please perform a manual verification to ensure that no call to
loadMinecraftDatais inadvertently passing a second parameter.
- Confirm that the removal of the
importBlockstatesModelsparameter hasn’t led to any hidden calls with two arguments.- Double-check any usage sites of
loadMinecraftDatain the codebase, ensuring they pass the singleversionparameter.renderer/viewer/lib/worldDataEmitter.ts (2)
8-8: Good addition of reactive state management.Adding the Valtio proxy import supports the new reactive state pattern being implemented throughout the codebase.
30-33:❓ Verification inconclusive
Excellent implementation of reactive properties for cursor state.
This addition provides a centralized reactive state for tracking cursor block information, which aligns with the PR's goal of restructuring the rendering pipeline. The use of Valtio's proxy will allow components to subscribe to these state changes efficiently.
Since you've added this reactive state but removed the mouse event listener (as mentioned in the AI summary), make sure there's code elsewhere that updates these reactive properties:
🏁 Script executed:
#!/bin/bash # Find where the cursor reactive properties are being updated rg "reactive\.cursorBlock" --type ts rg "reactive\.cursorBlockBreakingStage" --type tsLength of output: 90
Action Required: Verify Updates to Reactive Properties
Excellent implementation of reactive properties for cursor state. However, our search (using
rg "reactive\.cursorBlock"andrg "reactive\.cursorBlockBreakingStage") did not reveal any instances where these properties are updated. Given that the mouse event listener was removed, please verify manually that the logic to update these properties exists elsewhere. Specifically:
- Confirm whether updates to
reactive.cursorBlockandreactive.cursorBlockBreakingStageoccur in a different part of the codebase.- If the update logic has been moved or is expected to be triggered indirectly, indicate the precise location or mechanism responsible for these updates.
Once you’ve verified that the reactive state is properly maintained, we can be confident that the restructuring aligns with the overall pipeline improvements.
src/core/progressReporter.ts (2)
184-187: Good enhancement to logging flexibility.Adding the optional
groupparameter tocreateConsoleLogProgressReporterallows for better organization of logs by categorizing messages with their source or context. This change improves debugging capabilities, especially when multiple components are reporting progress simultaneously.
221-230: Well-implemented null object pattern.The
createNullProgressReporterfunction is a clean implementation of the null object pattern. This is useful for cases where a progress reporter interface is required, but no actual reporting is needed, allowing for consistent API usage without conditional checks throughout the codebase.src/mineflayer/cameraShake.ts (2)
55-56: Simplified camera roll implementation.The camera roll application has been refactored to use the new
appViewer.backend?.setRoll()method instead of direct quaternion manipulations. This aligns with the PR's goal of abstracting rendering operations behind a stable API layer.
70-72: Event listener moved outside of bot creation.Moving the 'hurtAnimation' event listener outside of the 'mineflayerBotCreated' event improves the event handling structure. This ensures that camera shake can be triggered consistently regardless of when the bot is created.
src/entities.ts (2)
2-2: Updated import path for utility function.The import path for
versionToNumberhas been updated from 'renderer/viewer/prepare/utils' to 'renderer/viewer/common/utils'. This change aligns with the overall restructuring of the codebase.
38-40: Intentional early return breaks entity functionality.Adding a
returnstatement at the beginning of the 'gameLoaded' event handler prevents the execution of entity tracking, auto-jump, animation, and skin loading functionality. This deliberately disables this functionality as part of separating application (mineflayer) code from the renderer.This change appears intentional based on the PR description's goal to separate application code from rendering code, but it significantly changes game behavior.
Could you confirm that disabling all entity-related functionality in this handler is intentional? This includes:
- Player tracking
- Auto-jump functionality
- Player animations
- Skin loading
If this is temporary during the refactoring process, consider adding a TODO comment to indicate future restoration.
src/react/HotbarRenderApp.tsx (1)
127-127: Event handler updated to use new resource management system.The event listener has been changed from
viewer.world.renderUpdateEmitter.on('textureDownloaded', upHotbarItems)toappViewer.resourcesManager.on('assetsTexturesUpdated', upHotbarItems). This aligns with the PR's goal of centralizing resource management through the newappViewer.resourcesManager.Also applies to: 200-200
src/rendererUtils.ts (1)
77-78: Confirm commented-out camera updates.
These lines previously updated the camera's FOV, but they are now commented out. Ensure that this is intentional and that camera FOV updates are handled elsewhere as needed for the new rendering pipeline.Would you like a script to search for all references to
viewer.cameraassignments to verify they are safely replaced?renderer/viewer/three/documentRenderer.ts (1)
142-151: Properly disposing of resources
Disposing the renderer, canceling animation frames, and clearing intervals ensure minimal memory leaks. This is a well-structured cleanup method.src/inventoryWindows.ts (2)
58-58: Event reference looks consistent with the new resources flow.
UsingappViewer.resourcesManager.on('assetsTexturesUpdated', ...)appears correct if this event is properly emitted. Ensure thatassetsTexturesUpdatedaligns with the new resource manager events.
9-9:✅ Verification successful
Check correctness of utility path update.
This import path changed fromrenderer/viewer/prepare/utilstorenderer/viewer/common/utils. Verify that all references toversionToNumberin your codebase have been updated consistently.
🏁 Script executed:
#!/bin/bash # Verify usage of versionToNumber references across files rg 'versionToNumber'Length of output: 3232
Utility Path Update Verified
- The import update in
src/inventoryWindows.tsis correct and now points to'renderer/viewer/common/utils'as intended.- A repository-wide search shows that nearly all internal modules import
versionToNumberfrom'renderer/viewer/common/utils'.- Note that a few files (e.g.
src/optionsGuiScheme.tsx,renderer/viewer/lib/guiRenderer.ts, andsrc/mineflayer/java-tester/commands.ts) importversionToNumberfrom external packages (mc-assets/dist/utilsorflying-squid/dist/utils). Please confirm these exceptions are intentional.src/devtools.ts (2)
24-24: Ensurewindow.worldis available before rendering debug chunks.
Ifwindow.worldhasn't been initialized yet or ifbot.entity.positionis undefined, the optional chaining might result in unexpected or silent failures. Consider asserting or validating these objects' states before callinggetLoadedChunksRelative.
166-210: Evaluate potential overhead & memory usage in performance stats code.
This block introduces a performance-debugging mechanism that records marks, intervals, and per-second metrics. While this is useful, consider adding clean-up logic or bounding the recorded labels to avoid potential memory bloat in long sessions.src/resourcesManager.ts (5)
1-20: Imports seem well-structured.
These imports appear to be properly scoped for resource management, referencing both internal and external libraries. There's no immediate concern, and the usage of typed-emitter and MC-assets modules is beneficial for type safety.
46-66: Good use of a dedicated class to hold loaded resources.
TheLoadedResourcesclass cleanly groups atlas parsers, custom textures, and versioning. For maintainability, keep an eye on possible custom properties that might be better split into multiple classes if this grows larger.
68-73: Well-defined request interface.
This interface effectively communicates required vs. optional properties for updating assets. Good job using a minimal set of fields.
75-94: Initialize shared state carefully.
TheResourcesManagerclass fields (likecurrentResources,abortController) are properly declared. Ensure that usage ofabortControlleris consistent across all asynchronous functions and that partial updates don’t leavecurrentResourcesin an incomplete state.
196-201: Reset resources to avoid memory leaks.
Aborting and clearingthis.currentResourcesis correct. Double-check that all references (e.g., event listeners, UI references) are equally cleaned up to prevent memory retention.renderer/viewer/lib/entities.ts (8)
27-27: New import indicates rendering refactor.
ImportingWorldRendererThreehere rather than a legacy viewer structure aligns with the overall refactoring of the rendering pipeline.
257-257: Scene removal aligns with new architecture.
Removing entities fromthis.worldRenderer.sceneis consistent with the new refactor. Ensure that any references to scene objects (e.g., bounding boxes) are also released to avoid orphaned objects in memory.
279-281: Safely adding or removing from the scene.
Double-check concurrency with mid-render modifications. Usually, Three.js can handle scene modifications between frames, but if these calls occur in render callbacks, consider deferring them or verifying thread safety in multi-threaded environments.
288-288: Dependency onworldRenderer.viewerPosition.
Relying onthis.worldRenderer.viewerPositionis straightforward. Ensure the property’s updates are in step with entity logic to prevent culling or distance-based rendering anomalies.
313-313: Prevent accidental visibility toggles.
this.worldRenderer.finishedChunks[chunkKey]is a neat check for finished chunk geometry. If chunks load or unload rapidly, watch for flickers or missed updates (e.g., partial chunk states).
507-507: Passingthis.worldRenderer.materialfor block meshes.
This works well for uniform material usage. If future expansions demand per-block or per-model customization, consider separating materials.
606-606: Remove object from new scene reference.
Ensure garbage collection for geometry, textures, and custom data. As you remove the entity from the scene, confirm that any references in local caches or maps are cleared.
678-678: Forwardingthis.worldRenderertogetEntityMesh.
This is consistent with the architectural shift away from the old viewer. Confirm thatgetEntityMeshremains renderer-agnostic if you plan to support multiple backends.renderer/viewer/three/graphicsBackend.ts (5)
10-11: Clarify the implications of disabling color management.
DisablingTHREE.ColorManagement.enabledcan create inconsistencies with color spaces across textures, materials, and post-processing pipelines. Confirm that this setting suits your project's rendering requirements, especially if you expect sRGB color spaces or HDR.
21-24: Confirm proxy usage is thread-safe and efficient.
The creation of a proxy object (worldState) is straightforward, but ensure that the usage patterns (e.g., frequent writes/reads) do not cause performance overhead or unexpected reactivity.
26-33: Handle potential concurrency instartPanorama().
If multiple calls tostartPanorama()occur rapidly (e.g., from different user actions), the early return on line 27 might skip a needed reinitialization. Confirm that ignoring calls whenworldRendererexists is the intended behavior.
40-52: Check disposal order instartWorld().
Disposing the panorama renderer before creating a new world renderer is reasonable, but if the user quickly reenters a panorama state, resources might thrash. Confirm that this is acceptable and that all allocated resources are properly released.
72-94: Validate interface completeness.
Thebackendobject exposes key methods likestartPanorama,disconnect, etc. Confirm that all expected rendering-lifecycle methods are covered, considering future expansions (e.g., hooking into scene updates or resource reloading).src/index.ts (3)
128-133: Verify single-file build mode readiness.
The environment block handlesSINGLE_FILE_BUILD_MODE, but confirm that external references (such as filesystem handling in the rest of the code) won’t break or degrade user experience in single-file mode.
708-716: Revisit the commented-out world rendering readiness logic.
This block checks for chunks finishing and triggers a final render. If you rely onviewer.worldto confirm chunk readiness, ensure an equivalent mechanism exists in the new approach or consider reviving this code if still needed.
737-760: Initialize the viewer carefully within spawn event.
RunningappViewer.startWorld(...), hookingwatchOptionsAfterWorldViewInit(), and callingappViewer.worldView.init(...)can lead to race conditions if the bot is not fully ready. Confirm that the Bot’s world data is stable by the time these are called.src/resourcePack.ts (1)
527-566: Watch for sequence issues inupdateTextures.
The function loads custom textures, updates replacable textures, and callsupdateAssetsData. IfupdateAssetsDatadepends on the new textures, ensure all textures are fully loaded first (e.g., checking image readiness). Otherwise, you might cause rendering artifacts.src/watchOptions.ts (6)
54-79: Validate window focus event listeners.
The new focus/blur listeners dynamically update the FPS limit. Overall, the approach looks solid. Make sure to confirm that no stale references arise when re-initializing or hot-reloading.Also applies to: 81-82
84-87: Clean configuration management.
StoringclipWorldBelowYandextraBlockRenderersinappViewer.inWorldRenderingConfigappears consistent and straightforward. No concerns.
95-96: Double-checkbotavailability.
Ensurebotis always defined here; otherwise, referencingbot.supportFeature('blockStateId')may throw ifbotis uninitialized or null.
98-100: ConfirmmineflayerBotCreatedtiming.
It’s good that you handle configuration as soon as the bot is created. Just confirm that relevant watchers likesubscribeKey(options, 'newVersionsLighting', ...)have run or won’t conflict with this event.
102-104: Straightforward refactor to starfield rendering.
Using a single watcher forstarfieldRenderingis concise. Continue with this approach for other features if possible.
118-120: Excellent approach for world-view settings.
Applying user preferences for chunk distance, ears rendering, hand visibility, and bobbing toappViewer.inWorldRenderingConfigkeeps the code modular.src/react/DebugOverlay.tsx (4)
41-41: Graceful fallback for no render backend.
Using “No render backend” as a fallback is fine; ensure code paths handle a nullishappViewer.backendconsistently throughout.
108-108: Check for cursor state readiness.
bot.mouse.getCursorState()might fail ifbot.mouseis absent or uninitialized. Safeguard with optional checks if needed.
137-137: Conditional entity string display.
Good approach verifyingentitiesStringbefore rendering. This ensures no runtime errors ifgetDebugOverlay()returns undefined.
194-194: Right debug overlay entries.
Accommodating an additional data region for the debug overlay is flexible. Confirm that callinggetDebugOverlay().rightis always valid.src/appViewer.ts (5)
1-16: Imports & basic setup look consistent.
Imports from therendererlibrary,valtio, and local modules are well organized. Keep an eye on any cyclical dependencies.
17-45: Interfaces define a clear contract.
Creating separate interfaces for state (WorldReactiveState), backend config (GraphicsBackendConfig), and initialization (GraphicsInitOptions,DisplayWorldOptions) enhances maintainability.
63-159:AppViewerclass architecture is cohesive and modular.
- Using
resourcesManageris a good approach for reusability.loadBackend,resetBackend,startPanorama, andstartWorldare well separated.- The queued display approach stands out as a flexible way of deferring actions.
164-179:initialMenuStartflow.
Recreating a backend if inworldmode then starting the panorama is logical. Ensure the future usage (commented lines 170-177) is either removed or properly integrated later.
180-193: Automatic modal stack checks.
Setting rendering based on modals is a neat approach. Confirm that heavy modals or concurrent modals don’t cause multiple redundant calls.renderer/viewer/lib/worldrendererCommon.ts (6)
35-46: Configuration fields look good.
ExpandingdefaultWorldRendererConfigwith new fields—likeviewBobbing,smoothLighting,enableLighting, etc.—improves flexibility. Ensure that any new options (e.g.,clipWorldBelowY,starfield) are respected throughout the rendering logic to avoid confusion or partially implemented features.
147-148: New class fields integrate cleanly.
IntroducingitemsTexture,soundSystem, andworldRendererConfiginto the class is coherent with the refactor. They align with the new resource-driven design.Also applies to: 152-152
154-181: Check error handling in constructor and worker initialization.
The constructor andinit()method rely onsetVersionto load resources asynchronously. If these calls fail, the promise chain may silently swallow errors. Consider a try/catch or.catch()block to handle failures gracefully and maintain robust initialization.
405-406: Verify clipping logic and chunk iteration.
UsingworldMinYRenderensures that chunks below a certain threshold are skipped. If future updates include negative Y-level, verify that the floor logic aligns correctly to avoid partial chunk rendering or skipping.Also applies to: 433-433, 462-465
480-496: Review queued block updates for concurrency conflicts.
The async queue approach insetBlockStateIdmay temporarily block updates for the affected chunk. Consider adding test coverage to ensure no race conditions occur when multiple updates happen rapidly.
735-756: Resource cleanup is well-structured.
Terminating workers, disposing of textures, and clearing event listeners helps avoid memory leaks. This explicit teardown supports the new resource-driven design effectively.renderer/viewer/lib/worldrendererThree.ts (4)
9-21: Imports appear relevant for new features.
The introduction ofDisplayWorldOptionsandGraphicsInitOptionsaligns with the rewritten resource handling. Similarly,getMyHandandEntitiesusage match the updated 3D rendering approach.
33-38: Additional fields improve scene setup.
DeclaringcameraRoll, a dedicatedscene, and ambient/directional lights is a clean approach. Centralizing entity management via theentitiesfield also clarifies responsibilities for rendering.
49-68: Validate critical errors in constructor flow.
The constructor throws ifresourcesManageris missing, but other potential issues (e.g., invalid config) are not caught. Consider a more robust strategy for fallback or user feedback if initialization partially fails.
70-82: Entity updates and scene resets appear sound.
updateEntity(e)calls intothis.entities.update(...)for posture updates, andresetScene()re-initializes core lighting and camera properties. These additions fit well into the new event-driven design.
Well while I can't say anything regarding the exact implementation of the change I think the idea behind adjusting it like that is sound. More abstraction of the different part to be independent from each other is definitely a good direction! Besides that it's just a lot of code so I can't say much about it but I didn't notice any obvious flaws while scrolling through it so there's that. |
Yeah, its not ready yet (though I'm planning to merge it soon already), but just wanted to let you know that there will be no direct access to the renderer from the app's code. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
renderer/viewer/lib/worldrendererCommon.ts (1)
500-595:⚠️ Potential issueAdded comprehensive event connection system
The new
connectmethod properly sets up all necessary event listeners for world interactions, entity updates, chunk loading, lighting changes, and time tracking. However, there are duplicate 'renderDistance' event handlers at lines 552 and 557 that might cause confusion or lead to repeated logic.There appears to be duplicate event handlers for 'renderDistance' at lines 552-555 and 557-561. This could cause unexpected behavior since both handlers update the same properties. Either remove one of the handlers or consolidate them into a single handler.
- worldEmitter.on('renderDistance', (d) => { - this.viewDistance = d - this.chunksLength = d === 0 ? 1 : generateSpiralMatrix(d).length - }) - - worldEmitter.on('renderDistance', (d) => { - this.viewDistance = d - this.chunksLength = d === 0 ? 1 : generateSpiralMatrix(d).length - this.allChunksFinished = Object.keys(this.finishedChunks).length === this.chunksLength - }) + worldEmitter.on('renderDistance', (d) => { + this.viewDistance = d + this.chunksLength = d === 0 ? 1 : generateSpiralMatrix(d).length + this.allChunksFinished = Object.keys(this.finishedChunks).length === this.chunksLength + })renderer/viewer/three/panorama.ts (1)
131-138:⚠️ Potential issueUndefined loadedData reference.
The
loadedDatavariable is referenced but not defined or imported in this file. This will cause a runtime error when this code executes.This needs to be either properly imported or passed as a parameter to the function. Fix by adding an import or defining the variable:
- const fullBlocks = loadedData.blocksArray.filter(block => { + // Example fix: Import or access loadedData from the appropriate source + const fullBlocks = this.options.resourcesManager.currentResources?.blockstatesModels?.blockstates?.latest?.blocks || []; + const filteredBlocks = fullBlocks.filter(block => {
🧹 Nitpick comments (14)
renderer/viewer/lib/entity/EntityMesh.ts (1)
240-240: Resource access structure improvementThe code now retrieves the texture information through the resource manager's current resources, which aligns with the architectural changes of the PR to better organize resource handling. This change supports the goal of separating the application from the renderer and improving resource management.
However, be cautious with the non-null assertion operator (
!) oncurrentResources. Consider adding a fallback or explicit null check to handle cases where resources might not be fully initialized.- const textureInfo = worldRenderer.resourcesManager.currentResources!.blocksAtlasParser.getTextureInfo(blockName) + const currentResources = worldRenderer.resourcesManager.currentResources + if (!currentResources) { + throw new Error('Resources not initialized for block textures') + } + const textureInfo = currentResources.blocksAtlasParser.getTextureInfo(blockName)renderer/viewer/three/panorama.ts (2)
124-125: Commented out resource loading code.There's a commented out call to load MC data, but the next line uses the resource manager. Consider removing the commented line if it's no longer needed.
- // await this.options.resourcesManager!.loadMcData(version) await this.options.resourcesManager.updateAssetsData({ version })
176-194: Event listener not properly cleaned up.While the event listener is created with the abort controller's signal, the event is attached to document.body directly. This could lead to event listener accumulation if the panorama is created and disposed multiple times.
Consider storing a reference to the listener function to ensure proper cleanup:
+ let pointerMoveListener: ((e: PointerEvent) => void) | null = null; document.body.addEventListener('pointermove', (e) => { if (e.pointerType !== 'mouse') return const pos = new THREE.Vector2(e.clientX, e.clientY) // ...rest of the function }, { signal: this.abortController.signal })And then in the dispose method:
dispose () { this.scene.clear() this.worldRenderer?.destroy() this.abortController.abort() + // Explicit removal if needed for older browsers that don't support AbortSignal + if (pointerMoveListener) { + document.body.removeEventListener('pointermove', pointerMoveListener) + pointerMoveListener = null + } }src/resourcesManager.ts (3)
108-109: Unresolved TODO comment in production code.There's a TODO comment about resolving the version, which suggests incomplete functionality. Consider either implementing the version resolution or documenting why it's deferred.
- // todo-low resolve version + // TODO (low priority): Implement version resolution for different Minecraft versions
132-144: Console timing logs in production code.The code includes
console.timeandconsole.timeEndcalls that are likely intended for development debugging. These should be removed or wrapped in a debug flag condition for production code.- console.time('createBlocksAtlas') + if (process.env.NODE_ENV !== 'production') console.time('createBlocksAtlas') const { atlas: blocksAtlas, canvas: blocksCanvas } = await blocksAssetsParser.makeNewAtlas( // ... parameters ) - console.timeEnd('createBlocksAtlas') + if (process.env.NODE_ENV !== 'production') console.timeEnd('createBlocksAtlas')
147-160: Duplicate console timing code pattern.Similar to the previous issue, the createItemsAtlas timing code should follow the same pattern for consistency. Consider applying the same fix here or extracting into a helper function.
- console.time('createItemsAtlas') + if (process.env.NODE_ENV !== 'production') console.time('createItemsAtlas') // ... code - console.timeEnd('createItemsAtlas') + if (process.env.NODE_ENV !== 'production') console.timeEnd('createItemsAtlas')renderer/viewer/three/graphicsBackend.ts (3)
15-17: Global renderer assignment could cause conflicts.Assigning to
globalThis.renderermakes the renderer accessible globally, which could lead to conflicts with other code or libraries. Consider using a more specific namespace or avoiding global assignment.- globalThis.renderer = documentRenderer.renderer + // If global access is needed, use a more specific namespace + globalThis.mcWebClient = globalThis.mcWebClient || {} + globalThis.mcWebClient.renderer = documentRenderer.renderer
30-33: Global panorama renderer assignment.Similar to the previous comment, setting
window.panoramaRenderercreates a global variable that could conflict with other code. Consider a more isolated approach.panoramaRenderer = new PanoramaRenderer(documentRenderer, initOptions, !!process.env.SINGLE_FILE_BUILD_MODE) void panoramaRenderer.start() - window.panoramaRenderer = panoramaRenderer + // If needed for debugging, use a namespaced global object + (window.mcWebClient = window.mcWebClient || {}).panoramaRenderer = panoramaRenderer
69-70: Commented out code.There's a commented out line that appears to be leftover debugging or development code. Consider removing it or adding a comment explaining why it's kept.
const renderer = WorldRendererThree.getRendererInfo(documentRenderer.renderer) ?? '...' - // viewer.setFirstPersonCamera(null, bot.entity.yaw, bot.entity.pitch)src/appViewer.ts (3)
125-130: Optional error handling for resource preparation.
If the underlying resource load fails,prepareResourceslogs and rethrows, but you might want to catch specific errors (e.g., network failures) and prompt user retry.
153-160:disconnectBackendleavesqueuedDisplayintact.
If the backend is torn down, any future queued action might be stale. Consider resetting or re-queuing relevant actions to avoid confusion if this method is invoked often.
162-176:utils.waitingForChunks()polling approach.
Using setInterval until chunks are loaded works, though you could observe chunk events for a more event-driven approach. Long intervals can hamper performance if scaled up.src/inventoryWindows.ts (2)
140-141: Usage ofblocksAtlasParser.latestImage.
Relying on a non-null assertion (!) might risk runtime errors ifblocksAtlasParserfails to load. Consider a fallback or user-friendly error if the parser is missing.
142-142:itemsAtlasParser.latestImage: same fallback consideration.
As withblocksAtlasParser, providing a graceful fallback image for items could improve user experience if the parser fails.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
renderer/playground/scenes/main.ts(0 hunks)renderer/viewer/lib/basePlayerState.ts(1 hunks)renderer/viewer/lib/entity/EntityMesh.ts(1 hunks)renderer/viewer/lib/guiRenderer.ts(5 hunks)renderer/viewer/lib/worldrendererCommon.ts(11 hunks)renderer/viewer/lib/worldrendererThree.ts(12 hunks)renderer/viewer/three/graphicsBackend.ts(1 hunks)renderer/viewer/three/panorama.ts(1 hunks)src/appViewer.ts(1 hunks)src/dayCycle.ts(1 hunks)src/globals.d.ts(1 hunks)src/inventoryWindows.ts(5 hunks)src/mineflayer/playerState.ts(3 hunks)src/react/HotbarRenderApp.tsx(3 hunks)src/resourcesManager.ts(2 hunks)src/water.ts(2 hunks)
💤 Files with no reviewable changes (1)
- renderer/playground/scenes/main.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/dayCycle.ts
- src/react/HotbarRenderApp.tsx
- src/globals.d.ts
- renderer/viewer/lib/guiRenderer.ts
🧰 Additional context used
🧬 Code Definitions (7)
src/water.ts (1)
src/appViewer.ts (1) (1)
appViewer(179-179)
renderer/viewer/three/panorama.ts (5)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(24-570)src/appViewer.ts (1) (1)
GraphicsInitOptions(34-39)renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(21-249)renderer/viewer/lib/worldrendererCommon.ts (1) (1)
defaultWorldRendererConfig(33-47)renderer/viewer/lib/basePlayerState.ts (1) (1)
BasePlayerState(41-96)
src/mineflayer/playerState.ts (1)
renderer/viewer/lib/basePlayerState.ts (1) (1)
BasePlayerState(41-96)
renderer/viewer/three/graphicsBackend.ts (3)
src/appViewer.ts (8) (8)
GraphicsBackendLoader(47-47)GraphicsInitOptions(34-39)WorldReactiveState(17-21)startPanorama(115-123)prepareResources(125-129)startWorld(131-146)DisplayWorldOptions(41-45)GraphicsBackend(49-62)renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(24-570)src/core/progressReporter.ts (1) (1)
ProgressReporter(5-18)
renderer/viewer/lib/worldrendererCommon.ts (3)
src/appViewer.ts (2) (2)
appViewer(179-179)DisplayWorldOptions(41-45)src/resourcesManager.ts (1) (1)
ResourcesManager(75-201)renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(21-249)
src/appViewer.ts (6)
src/resourcesManager.ts (1) (1)
ResourcesManager(75-201)src/core/progressReporter.ts (7) (7)
error(131-133)error(156-159)error(178-180)error(193-195)error(215-217)error(227-228)ProgressReporter(5-18)renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(21-249)renderer/viewer/lib/worldrendererCommon.ts (2) (2)
WorldRendererConfig(49-49)defaultWorldRendererConfig(33-47)renderer/viewer/lib/basePlayerState.ts (1) (1)
IPlayerState(16-39)src/mineflayer/playerState.ts (1) (1)
playerState(197-197)
renderer/viewer/lib/worldrendererThree.ts (4)
renderer/viewer/lib/entities.ts (1) (1)
Entities(209-1028)src/appViewer.ts (2) (2)
GraphicsInitOptions(34-39)DisplayWorldOptions(41-45)renderer/viewer/lib/holdingBlock.ts (1) (1)
HoldingBlock(93-510)renderer/viewer/lib/hand.ts (1) (1)
getMyHand(5-35)
🪛 ESLint
renderer/viewer/lib/worldrendererThree.ts
[error] 336-336: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (48)
renderer/viewer/lib/basePlayerState.ts (2)
32-38: Added reactive state properties to enhance environment handlingThe additions to the
reactiveobject in theIPlayerStateinterface properly extend the player state with environmental properties such as water state, background color, and lighting values. This change supports the architecture refactoring by centralizing state management through reactive properties.
42-48: Good initialization of reactive state with proper typingThe explicit typing of
playerSkinasstring | undefinedand the other initialization values follow good TypeScript practices. The tuple type definition forbackgroundColorensures type safety when handling color values.renderer/viewer/lib/worldrendererCommon.ts (8)
13-15: Improved imports with clear dependenciesThe imports have been updated to include ResourcesManager and DisplayWorldOptions, properly reflecting the new architecture where resources are managed centrally. The removal of direct texture imports aligns with moving resource management to a dedicated manager.
33-46: Enhanced renderer configuration with additional parametersThe configuration has been expanded with several new options that improve flexibility:
- Renamed
numWorkersto the more descriptivemesherWorkers- Added support for extra block renderers, clip world thresholds, and lighting options
- Included a parameter for controlling chunk batch loading time
These additions provide better control over rendering behavior and performance.
147-148: Added new texture and sound system propertiesAdding dedicated properties for item textures and sound system aligns with the improved architecture, where resources are managed more systematically.
154-165: Refactored constructor to use resource manager and display optionsThe constructor now takes a ResourcesManager, DisplayWorldOptions, and version string, properly separating resource management from the renderer itself. The initialization flow is improved by calling
this.connect()to set up event listeners, which enhances the separation of concerns.
167-176: New initialization method with resource update handlingThe
initmethod properly checks if the renderer is already active, sets up the version, and adds listeners for asset texture updates. This pattern ensures that resources are properly loaded and updated when needed.
347-351: Improved resource loading with central resource managerThe
setVersionmethod now correctly delegates Minecraft data loading to the resource manager and ensures resources are updated appropriately, which better separates concerns and improves architecture clarity.
372-400: Refactored asset update method to use resource managerThe
updateAssetsDatamethod now properly retrieves textures from the centralized resource manager instead of managing them directly. This improves code organization and maintainability.
735-757: Enhanced destroy method with proper resource cleanupThe improved
destroymethod now correctly handles the termination of workers, sound system, and material resources. This prevents memory leaks and ensures clean disconnection of the renderer.renderer/viewer/lib/worldrendererThree.ts (8)
35-40: Added essential scene components as class propertiesThe addition of scene, lights, and entities as class properties provides better structure and encapsulation. This change aligns with the architectural goal of more modular rendering components.
50-69: Refactored constructor with improved initialization flowThe constructor now properly validates required parameters, sets up the renderer with the resource manager, and initializes the scene and camera. The initialization flow is more structured and modular.
71-81: Added entity update methodThe new
updateEntitymethod properly handles updating entity rotation values, separating this responsibility from the event handling code.
83-93: Added scene initialization methodThe
resetScenemethod properly sets up the Three.js scene with lights, camera, and other essential components. This refactoring improves readability and maintainability.
95-114: Added reactive state subscription handlingThe new
watchReactivePlayerStatemethod properly subscribes to changes in the player state and reflects those changes in the rendering components (background color, fog, lighting). This pattern decouples the application state from the renderer.
309-315: Added first-person camera handlingThe new
setFirstPersonCameramethod appropriately handles setting the camera position and orientation for first-person view, taking into account the player's eye height from the player state.
331-343: Enhanced render method with size handlingThe updated
rendermethod now properly handles window size changes and updates the camera aspect ratio accordingly. The addition of entity rendering ensures that all scene elements are rendered properly.🧰 Tools
🪛 ESLint
[error] 336-336: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
557-569: Added camera roll handlingThe new
setCameraRollmethod provides a clean way to apply roll rotation to the camera, using quaternions for proper composition of rotations. This enhances the camera control capabilities of the renderer.src/water.ts (3)
8-8: Water state now updates player state reactive propertiesThe code now sets the
inWaterproperty in the player's reactive state instead of directly manipulating the scene. This aligns with the architectural change to separate application state from rendering concerns.
15-15: Consistent use of reactive state for water detectionThis change ensures that water detection consistently updates the player's reactive state rather than directly modifying the renderer.
35-35: Background color updates now use reactive stateThe
updateBackgroundfunction now properly updates the player's reactive state with the background color instead of directly modifying the renderer. This is consistent with the architectural change to separate application state from rendering.src/mineflayer/playerState.ts (2)
3-3: Import alignment with new architecture.Good addition of the BasePlayerState import. This aligns with the refactoring goal of separating application code from rendering code as mentioned in the PR objectives.
32-32: Improved state reuse through BasePlayerState.Using
BasePlayerState().reactiveinstead of directly creating a proxy improves code reuse and consistency across components. This change is aligned with the PR's goal of isolating rendering concerns from application code.renderer/viewer/three/panorama.ts (6)
1-13: Dependencies organization and imports look good.The imports are well-organized and include all necessary dependencies for a Three.js-based panorama renderer. This follows a good pattern of separating external libraries from internal modules.
14-21: Well-defined panorama file configuration.The panorama files array clearly defines the ordering and mapping for a cubemap texture, which is essential for proper skybox rendering. The comments indicating which direction each texture represents are helpful for maintainability.
23-49: Clean PanoramaRenderer class structure.The class is well-structured with private fields for internal state management and a clean constructor that sets up the 3D scene with proper lighting. This implementation aligns with the PR's goal of having a clear separation between rendering components.
51-66: Good conditional rendering logic in start method.The start method correctly handles the different rendering paths based on the doWorldBlocksPanorama flag and properly updates the document renderer. This implements the flexible rendering approach mentioned in the PR objectives.
68-120: Comprehensive classic panorama implementation.The implementation of the classic panorama with proper texture handling, rotation animation, and entity decoration is well done. The use of texture matrix manipulation instead of repeat/offset for flipping is a good approach.
199-204: Proper cleanup in dispose method.The dispose method appropriately cleans up resources by clearing the scene, destroying the world renderer, and aborting ongoing operations. This is crucial for preventing memory leaks, especially in a renderer that may be created and destroyed multiple times during the application lifecycle.
src/resourcesManager.ts (5)
42-44: Well-defined event types for the ResourceManager.Using TypedEmitter with clearly defined event types enhances type safety and makes the codebase more maintainable. The 'assetsTexturesUpdated' event provides a clear contract for subscribers.
46-67: Comprehensive LoadedResources class design.The LoadedResources class is well-structured with clear separation between the different types of resources (atlas parsers, custom data, renderers). This organized approach will make resource management more maintainable.
68-74: Clear UpdateAssetsRequest interface.The interface provides a well-defined contract for resource updates with appropriate optional parameters. This will help ensure consistency across different types of update operations.
75-85: ResourcesManager class with proper inheritance.The ResourcesManager extends EventEmitter with typed events, which is a good pattern for enabling reactive behavior. The initialization of source data properties and abort controller shows careful consideration of resource lifecycle management.
196-201: Proper resource cleanup in destroy method.The destroy method properly cleans up resources by aborting the controller, clearing the current resources, and creating a new abort controller. This is essential for preventing memory leaks, especially in a component that manages potentially large assets.
renderer/viewer/three/graphicsBackend.ts (2)
10-11: Good Three.js color management configuration.Setting
THREE.ColorManagement.enabled = falseis important for maintaining consistent color handling across different Three.js versions. The comment with a link to the relevant documentation is helpful for future maintenance.
73-95: Well-defined GraphicsBackend implementation.The public interface of the backend is well-structured with clear responsibilities for each method. The use of getters for properties like
soundSystemthat depend on the world renderer is a good pattern to avoid null/undefined access.src/appViewer.ts (9)
1-16: Imports look well-structured and comprehensive.
They provide necessary references for rendering, events, and state management, all in one place without obvious redundancies.
64-81: GlobalAppViewerdesign might lead to hidden state pitfalls.
Exporting a single global instance can cause concurrency issues if this code is ever run in multi-instance scenarios or if multiple viewers are needed. This was brought up in a prior comment.
109-114: Resetting the backend.
TheresetBackend()method succinctly reloads the loader. This is a clear approach if you need to reinitialize for new configurations or after a crash.
115-123: Queued actions design is effective.
Storing and replaying the panorama start call when a backend is not yet ready simplifies usage. This pattern ensures user actions aren’t lost.
148-151: Graceful resource destruction.
CallingdisconnectBackend()before destroying resources helps prevent dangling references. This approach is generally safe.
179-181: Global instance can lead to hidden state issues if concurrency arises.
This exact feedback was given in a previous review.
182-187:initialMenuStartdesign.
This method triggers a backend reset if already in a world, then starts the panorama. No concerns here, but consider logging or user feedback if a reset occurs.
198-210:modalStackUpdateChecksrenders conditionally.
This is a clear use of reactive state to manage rendering. Good logic to resume the panorama when no modals are active.
131-146:❓ Verification inconclusive
startWorldusage of default player state.
The logic is straightforward, but if multiple players or concurrency are ever planned, consider injecting a player state per instance ofAppViewerto avoid collisions.Would you like to confirm how many references to
startWorldpass a customplayerStateSendinstead of defaulting to the global one?
🏁 Script executed:
#!/bin/bash rg -A 5 "startWorld\("Length of output: 872
Action: Reassess Player State Injection Strategy in
startWorld
The grep search confirms that the only active call (fromsrc/index.ts) does not supply a customplayerStateSend, so the method always defaults to the globalplayerState. There is a commented-out example insrc/appViewer.tsthat shows using a custom state (i.e.new BasePlayerState()), hinting at an alternative approach.
- src/index.ts: Active call uses default state (three arguments are passed).
- src/appViewer.ts: Contains a commented-out call with a custom player state.
If you plan on supporting multiple players or concurrency, consider injecting a unique player state per
AppViewerinstance rather than relying on a global default to avoid potential state collisions.src/inventoryWindows.ts (3)
9-9: Updated import path looks appropriate.
Switching torenderer/viewer/common/utilssuggests better organization of utility functions.
49-50: Conditional check foritemsAtlasParser.
If it’s unavailable, the function returns early, preventing errors. Subscribing to'assetsTexturesUpdated'ensures dynamic updates—this is a clean approach.Also applies to: 58-58
225-228: Robust fallback for missing or errored textures.
CallinggetItemTextureand falling back to'item/missing_texture'or'block/errored'ensures the GUI won’t break. This is a best practice for resource-laden apps.Also applies to: 231-231
| @@ -40,7 +38,7 @@ export class PlayerStateManager implements IPlayerState { | |||
| return this.instance | |||
| } | |||
|
|
|||
| private constructor () { | |||
| constructor () { | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Constructor visibility changed from private to public.
The constructor visibility has been changed from private to public, which breaks the singleton pattern enforced by the class. While the singleton instance is still accessible via getInstance(), this change allows creating multiple instances directly which may lead to inconsistent state.
Consider if this change is intentional as part of the refactoring. If it is, ensure that:
- Documentation is updated to reflect this architectural change
- Any code that depends on there being a single instance is reviewed
🏁 Script executed:
#!/bin/bash
# Search for direct instantiations of PlayerStateManager that bypass getInstance()
rg "new PlayerStateManager\(\)" --type tsLength of output: 120
Singleton Pattern Enforcement: Public Constructor Allows Direct Instantiation
The change from a private to a public constructor in PlayerStateManager is confirmed. A direct instantiation (new PlayerStateManager()) is found in src/mineflayer/playerState.ts, which bypasses the expected singleton behavior provided by getInstance(). This risks multiple instances being created and could lead to inconsistent state.
- Risk: Public access to the constructor allows creation of new instances outside of the controlled singleton pattern.
- Action Items:
- If this refactor is intentional, please update the documentation and carefully review any code that assumes only one instance exists.
- Otherwise, revert the constructor’s visibility to private to maintain the singleton guarantee.
| const prepareResources = async (ver: string, progressReporter: ProgressReporter): Promise<void> => { | ||
| version = ver | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Incomplete prepareResources implementation.
The prepareResources method only stores the version but doesn't actually prepare any resources. This seems inconsistent with its purpose and the method signature that returns a Promise.
const prepareResources = async (ver: string, progressReporter: ProgressReporter): Promise<void> => {
version = ver
+ // Example implementation (adapt based on actual requirements):
+ try {
+ await progressReporter.executeWithMessage('Loading resources', async () => {
+ await initOptions.resourcesManager.loadMcData(ver)
+ await initOptions.resourcesManager.updateAssetsData({ version: ver })
+ })
+ } catch (error) {
+ initOptions.displayCriticalError(error instanceof Error ? error : new Error('Failed to prepare resources'))
+ }
}📝 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 prepareResources = async (ver: string, progressReporter: ProgressReporter): Promise<void> => { | |
| version = ver | |
| } | |
| const prepareResources = async (ver: string, progressReporter: ProgressReporter): Promise<void> => { | |
| version = ver | |
| // Example implementation (adapt based on actual requirements): | |
| try { | |
| await progressReporter.executeWithMessage('Loading resources', async () => { | |
| await initOptions.resourcesManager.loadMcData(ver) | |
| await initOptions.resourcesManager.updateAssetsData({ version: ver }) | |
| }) | |
| } catch (error) { | |
| initOptions.displayCriticalError(error instanceof Error ? error : new Error('Failed to prepare resources')) | |
| } | |
| } |
src/appViewer.ts
Outdated
| loadBackend (loader: GraphicsBackendLoader) { | ||
| if (this.backend) { | ||
| this.disconnectBackend() | ||
| } | ||
|
|
||
| this.backendLoader = loader | ||
| const loaderOptions: GraphicsInitOptions = { | ||
| resourcesManager: this.resourcesManager, | ||
| config: this.config, | ||
| displayCriticalError (error) { | ||
| console.error(error) | ||
| setLoadingScreenStatus(error.message, true) | ||
| }, | ||
| } | ||
| this.backend = loader(loaderOptions) | ||
|
|
||
| if (this.resourcesManager.currentResources) { | ||
| void this.prepareResources(this.resourcesManager.currentResources.version, createNotificationProgressReporter()) | ||
| } | ||
|
|
||
| // Execute queued action if exists | ||
| if (this.queuedDisplay) { | ||
| const { method, args } = this.queuedDisplay | ||
| this.backend[method](...args) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider error-handling around backend loading.
Within loadBackend, if backend initialization fails, the code just logs the error. For large-scale applications, you might handle it more robustly (e.g., fallback strategy or user-facing error message) to avoid silent failures.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
renderer/viewer/lib/worldrendererCommon.ts (1)
514-609:⚠️ Potential issueRefactor duplicated event listeners for render distance.
Within the
connect(worldView)method, there are duplicate event listeners for 'renderDistance' (lines 566-569 and 571-575), which could lead to duplicate handling of the same event.Apply this diff to consolidate the event listeners and avoid potential side effects:
- worldEmitter.on('renderDistance', (d) => { - this.viewDistance = d - this.chunksLength = d === 0 ? 1 : generateSpiralMatrix(d).length - }) - - worldEmitter.on('renderDistance', (d) => { - this.viewDistance = d - this.chunksLength = d === 0 ? 1 : generateSpiralMatrix(d).length - this.allChunksFinished = Object.keys(this.finishedChunks).length === this.chunksLength - }) + worldEmitter.on('renderDistance', (d) => { + this.viewDistance = d + this.chunksLength = d === 0 ? 1 : generateSpiralMatrix(d).length + this.allChunksFinished = Object.keys(this.finishedChunks).length === this.chunksLength + })
🧹 Nitpick comments (11)
src/mineflayer/items.ts (1)
93-110: Good abstraction for item model resolutionThis function cleanly separates the logic for determining an item's model name, making it reusable across the application.
However, there's an undeclared reference to
appViewerat line 103 and 105. This global variable usage could make testing more difficult.Consider adding the missing import for
appViewerand potentially refactoring to accept a resources manager as a parameter (similar to thegetItemUvfunction) to reduce global state dependency:+ import { appViewer } from '../../appViewer' - export const getItemModelName = (item: GeneralInputItem, specificProps: ItemSpecificContextProperties) => { + export const getItemModelName = ( + item: GeneralInputItem, + specificProps: ItemSpecificContextProperties, + resourcesManager = appViewer.resourcesManager + ) => { // ... - const modelFromDef = getItemDefinition(appViewer.resourcesManager.itemsDefinitionsStore, { + const modelFromDef = getItemDefinition(resourcesManager.itemsDefinitionsStore, { name: itemModelName, - version: appViewer.resourcesManager.currentResources!.version, + version: resourcesManager.currentResources!.version, properties: itemSelector })?.model // ... }src/resourcesManager.ts (2)
74-164: Complex updateAssetsData method needs refactoring.This method is too long and handles multiple concerns, making it difficult to maintain and test.
Consider breaking this method into smaller, more focused methods:
async updateAssetsData (request: UpdateAssetsRequest) { const abortController = new AbortController() await this.loadSourceData(request.version) if (abortController.signal.aborted) return const resources = this.currentResources ?? new LoadedResources() resources.version = request.version resources.texturesVersion = request.texturesVersion ?? resources.version + this.initializeBlockstatesModels(resources) + await this.createAtlases(resources, request, abortController) + + if (abortController.signal.aborted) return + this.finalizeResourcesSetup(resources) + + this.emit('assetsTexturesUpdated') + this.currentResources = resources } + private initializeBlockstatesModels(resources: LoadedResources) { + resources.blockstatesModels = { + blockstates: {}, + models: {} + } + // todo-low resolve version + resources.blockstatesModels.blockstates.latest = { + ...this.sourceBlockStatesModels.blockstates.latest, + ...resources.customBlockStates + } + + resources.blockstatesModels.models.latest = { + ...this.sourceBlockStatesModels.models.latest, + ...resources.customModels + } + } + private async createAtlases(resources: LoadedResources, request: UpdateAssetsRequest, abortController: AbortController) { + // Create blocks atlas + const blockTexturesChanges = this.getSeasonalTextureChanges() + const customBlockTextures = Object.keys(resources.customTextures.blocks?.textures ?? {}) + const customItemTextures = Object.keys(resources.customTextures.items?.textures ?? {}) + + console.time('createBlocksAtlas') + const blocksAssetsParser = new AtlasParser(this.sourceBlocksAtlases, blocksAtlasLatest, blocksAtlasLegacy) + const { atlas: blocksAtlas, canvas: blocksCanvas } = await blocksAssetsParser.makeNewAtlas( + resources.texturesVersion, + (textureName) => { + if (request.includeOnlyBlocks && !request.includeOnlyBlocks.includes(textureName)) return false + const texture = resources.customTextures.blocks?.textures[textureName] + return blockTexturesChanges[textureName] ?? texture + }, + undefined, + undefined, + customBlockTextures + ) + console.timeEnd('createBlocksAtlas') + + if (abortController.signal.aborted) return + + // Create items atlas + console.time('createItemsAtlas') + const itemsAssetsParser = new AtlasParser(this.sourceItemsAtlases, itemsAtlasLatest, itemsAtlasLegacy) + const { atlas: itemsAtlas, canvas: itemsCanvas } = await itemsAssetsParser.makeNewAtlas( + resources.texturesVersion, + (textureName) => { + const texture = resources.customTextures.items?.textures[textureName] + if (!texture) return + return texture + }, + resources.customTextures.items?.tileSize, + undefined, + customItemTextures + ) + console.timeEnd('createItemsAtlas') + + // Create atlas parsers + resources.blocksAtlasParser = new AtlasParser({ latest: blocksAtlas }, blocksCanvas.toDataURL()) + resources.itemsAtlasParser = new AtlasParser({ latest: itemsAtlas }, itemsCanvas.toDataURL()) + resources.blocksAtlasImage = await getLoadedImage(blocksCanvas.toDataURL()) + resources.itemsAtlasImage = await getLoadedImage(itemsCanvas.toDataURL()) + } + private getSeasonalTextureChanges() { + const blockTexturesChanges = {} as Record<string, string> + const date = new Date() + if ((date.getMonth() === 11 && date.getDate() >= 24) || (date.getMonth() === 0 && date.getDate() <= 6)) { + Object.assign(blockTexturesChanges, christmasPack) + } + return blockTexturesChanges + } + private finalizeResourcesSetup(resources: LoadedResources) { + if (resources.version && resources.blockstatesModels && resources.itemsAtlasParser && resources.blocksAtlasParser) { + resources.itemsRenderer = new ItemsRenderer( + resources.version, + resources.blockstatesModels, + resources.itemsAtlasParser, + resources.blocksAtlasParser + ) + resources.worldBlockProvider = worldBlockProvider( + resources.blockstatesModels, + resources.blocksAtlasParser.atlas, + 'latest' + ) + } + }
166-175: Add error handling for downloadDebugAtlas method.The current implementation doesn't handle potential errors when creating the debug image or when the download fails.
Add try-catch block for error handling:
async downloadDebugAtlas (isItems = false) { + try { const resources = this.currentResources if (!resources) throw new Error('No resources loaded') const atlasParser = (isItems ? resources.itemsAtlasParser : resources.blocksAtlasParser)! const dataUrl = await atlasParser.createDebugImage(true) const a = document.createElement('a') a.href = dataUrl a.download = `atlas-debug-${isItems ? 'items' : 'blocks'}.png` a.click() + } catch (error) { + console.error(`Failed to download debug atlas for ${isItems ? 'items' : 'blocks'}:`, error) + throw new Error(`Failed to download debug atlas: ${error.message}`) + } }renderer/viewer/lib/worldrendererCommon.ts (1)
521-547: Chunk loading batch system should use a queue structure.The current implementation creates a batch for all chunks and processes them together after a timeout. This could lead to uneven frame times and potential stuttering if many chunks arrive at once.
Consider implementing a more sophisticated queue system that processes chunks in smaller batches to distribute the workload more evenly across frames:
- let currentLoadChunkBatch = null as { - timeout - data - } | null - worldEmitter.on('loadChunk', ({ x, z, chunk, worldConfig, isLightUpdate }) => { - this.worldSizeParams = worldConfig - this.queuedChunks.add(`${x},${z}`) - const args = [x, z, chunk, isLightUpdate] - if (!currentLoadChunkBatch) { - // add a setting to use debounce instead - currentLoadChunkBatch = { - data: [], - timeout: setTimeout(() => { - for (const args of currentLoadChunkBatch!.data) { - this.queuedChunks.delete(`${args[0]},${args[1]}`) - this.addColumn(...args as Parameters<typeof this.addColumn>) - } - for (const fn of this.queuedFunctions) { - fn() - } - this.queuedFunctions = [] - currentLoadChunkBatch = null - }, this.worldRendererConfig.addChunksBatchWaitTime) - } - } - currentLoadChunkBatch.data.push(args) + const chunkQueue: [number, number, any, boolean][] = [] + let chunkProcessingTimer: NodeJS.Timeout | null = null + + const processNextChunkBatch = () => { + const BATCH_SIZE = 5 // Adjust based on performance testing + const batch = chunkQueue.splice(0, BATCH_SIZE) + + for (const args of batch) { + this.queuedChunks.delete(`${args[0]},${args[1]}`) + this.addColumn(...args as Parameters<typeof this.addColumn>) + } + + if (chunkQueue.length > 0) { + chunkProcessingTimer = setTimeout(processNextChunkBatch, 16) // ~60 FPS + } else { + // Process queued functions when all chunks are loaded + for (const fn of this.queuedFunctions) { + fn() + } + this.queuedFunctions = [] + chunkProcessingTimer = null + } + } + + worldEmitter.on('loadChunk', ({ x, z, chunk, worldConfig, isLightUpdate }) => { + this.worldSizeParams = worldConfig + this.queuedChunks.add(`${x},${z}`) + chunkQueue.push([x, z, chunk, isLightUpdate]) + + if (!chunkProcessingTimer) { + chunkProcessingTimer = setTimeout( + processNextChunkBatch, + this.worldRendererConfig.addChunksBatchWaitTime + ) + }This approach provides more consistent frame times by processing chunks in smaller batches and distributes the workload more evenly.
renderer/viewer/lib/entities.ts (1)
223-235: Remove commented code.This commented code block for
getItemUvis no longer needed and should be removed to improve code clarity.- // getItemUv: undefined | (() => { - // texture: THREE.Texture; - // u: number; - // v: number; - // su?: number; - // sv?: number; - // size?: number; - // modelName?: string; - // } | { - // resolvedModel: BlockModel - // modelName: string - // } | undefined)src/index.ts (1)
647-656: Remove or fix commented code for world readiness.This commented block should either be removed or updated to use the new architecture since it references the old
viewer.worldapproach.If you want to preserve this functionality with the new architecture, consider updating it to:
- const worldWasReady = false - // void viewer.world.renderUpdateEmitter.on('update', () => { - // // todo might not emit as servers simply don't send chunk if it's empty - // if (!viewer.world.allChunksFinished || worldWasReady) return - // worldWasReady = true - // console.log('All chunks done and ready! Time from renderer open to ready', (Date.now() - start) / 1000, 's') - // viewer.render() // ensure the last state is rendered - // document.dispatchEvent(new Event('cypress-world-ready')) - // }) + let worldWasReady = false + const unsub = subscribe(appViewer.backend?.reactiveState.world ?? {}, () => { + if (!appViewer.backend?.reactiveState.world.allChunksLoaded || worldWasReady) return + worldWasReady = true + console.log('All chunks done and ready! Time from renderer open to ready', (Date.now() - start) / 1000, 's') + // ensure the last state is rendered + document.dispatchEvent(new Event('cypress-world-ready')) + unsub() + })src/appViewer.ts (2)
37-42: Validate GraphicsInitOptions for error handling.The
displayCriticalErrorfunction might need more robust error handling. Currently, it logs the error and updates the loading screen but doesn't include any retry logic or fallback mechanisms, which could improve user experience during initialization failures.Consider adding recovery strategies:
displayCriticalError (error: Error) { console.error(error) setLoadingScreenStatus(error.message, true) + // Consider adding one of these strategies: + // 1. Retry with fallback renderer: this.retryWithFallbackRenderer() + // 2. Show actionable message with retry button + // 3. Switch to a minimal compatibility mode }
193-206: Clean up commented-out code in initialMenuStart.The function contains significant commented-out code (lines 199-205) that suggests experimental or unfinished functionality. This should be cleaned up before merging to production.
Either remove the commented code completely or add a TODO comment explaining why it's kept for future reference:
const initialMenuStart = async () => { if (appViewer.currentDisplay === 'world') { appViewer.resetBackend() } appViewer.startPanorama() - // await appViewer.resourcesManager.loadMcData('1.21.4') - // const world = getSyncWorld('1.21.4') - // await appViewer.prepareResources('1.21.4', createNullProgressReporter()) - // world.setBlockStateId(new Vec3(0, 64, 0), 1) - // appViewer.startWorld(world, 3, new Vec3(0, 64, 0), new BasePlayerState()) - // appViewer.backend?.updateCamera(new Vec3(0, 64, 2), 0, 0) - // void appViewer.worldView.init(new Vec3(0, 64, 0)) + /* TODO: This was a test for direct world loading, keep for reference until the panorama-to-world transition is fully implemented and tested */ }src/vr.ts (1)
86-89: Add confirmation dialog for VR disabling.When a user clicks the close button, VR is immediately disabled without confirmation. Consider adding a confirmation dialog for a better user experience, especially since VR mode changes can be disorienting.
closeButton.addEventListener('click', () => { + // Confirm before disabling VR to avoid accidental clicks + if (confirm('Exit VR mode?')) { container.hidden = true worldRenderer.worldRendererConfig.vrSupport = false + } })renderer/viewer/lib/worldrendererThree.ts (2)
342-352: Commented-out free fly mode code.There are several commented-out code blocks related to a
freeFlyModefeature. This suggests that the refactoring is incomplete or that this feature might be reintroduced later.If the free-fly feature is planned for future implementation, add a TODO comment with details. If it's been permanently removed, remove all related commented code.
updateCamera (pos: Vec3 | null, yaw: number, pitch: number): void { - // if (this.freeFlyMode) { - // pos = this.freeFlyState.position - // pitch = this.freeFlyState.pitch - // yaw = this.freeFlyState.yaw - // } + // TODO: Implement free-fly camera mode in the new architecture if (pos) { new tweenJs.Tween(this.camera.position).to({ x: pos.x, y: pos.y, z: pos.z }, 50).start() - // this.freeFlyState.position = pos }
363-364: ESLint rule is disabled but never reported.According to static analysis, the ESLint rule '@typescript-eslint/non-nullable-type-assertion-style' is disabled but never reported in this line.
Remove the unused ESLint disable directive to keep the codebase clean.
- // eslint-disable-next-line @typescript-eslint/non-nullable-type-assertion-style const cam = this.camera instanceof THREE.Group ? this.camera.children.find(child => child instanceof THREE.PerspectiveCamera) as THREE.PerspectiveCamera : this.camera🧰 Tools
🪛 ESLint
[error] 363-363: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
renderer/viewer/baseGraphicsBackend.ts(1 hunks)renderer/viewer/lib/basePlayerState.ts(3 hunks)renderer/viewer/lib/entities.ts(18 hunks)renderer/viewer/lib/holdingBlock.ts(5 hunks)renderer/viewer/lib/mesher/mesher.ts(1 hunks)renderer/viewer/lib/worldrendererCommon.ts(11 hunks)renderer/viewer/lib/worldrendererThree.ts(12 hunks)renderer/viewer/three/appShared.ts(1 hunks)renderer/viewer/three/graphicsBackend.ts(1 hunks)renderer/viewer/three/panorama.ts(1 hunks)renderer/viewer/three/threeJsMethods.ts(1 hunks)src/appViewer.ts(1 hunks)src/cameraRotationControls.ts(2 hunks)src/customChannels.ts(2 hunks)src/index.ts(15 hunks)src/inventoryWindows.ts(6 hunks)src/mineflayer/items.ts(2 hunks)src/mineflayer/maps.ts(2 hunks)src/mineflayer/playerState.ts(4 hunks)src/react/DebugOverlay.tsx(8 hunks)src/rendererUtils.ts(2 hunks)src/resourcesManager.ts(1 hunks)src/sounds/botSoundSystem.ts(2 hunks)src/vr.ts(5 hunks)src/watchOptions.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/sounds/botSoundSystem.ts
- src/cameraRotationControls.ts
- renderer/viewer/three/panorama.ts
- renderer/viewer/three/graphicsBackend.ts
- src/mineflayer/playerState.ts
- src/inventoryWindows.ts
- src/react/DebugOverlay.tsx
🧰 Additional context used
🧬 Code Definitions (11)
src/customChannels.ts (1)
renderer/viewer/three/threeJsMethods.ts (1) (1)
getThreeJsRendererMethods(4-15)
src/rendererUtils.ts (1)
src/appViewer.ts (1) (1)
appViewer(190-190)
renderer/viewer/three/appShared.ts (4)
renderer/viewer/lib/basePlayerState.ts (1) (1)
ItemSpecificContextProperties(9-9)src/resourcesManager.ts (1) (1)
ResourcesManager(54-182)src/inventoryWindows.ts (1) (1)
renderSlot(188-249)src/mineflayer/items.ts (2) (2)
getItemModelName(93-110)GeneralInputItem(22-26)
src/mineflayer/items.ts (3)
renderer/viewer/lib/basePlayerState.ts (1) (1)
ItemSpecificContextProperties(9-9)src/mineflayer/playerState.ts (1) (1)
playerState(201-201)src/appViewer.ts (1) (1)
appViewer(190-190)
renderer/viewer/lib/holdingBlock.ts (3)
renderer/viewer/lib/basePlayerState.ts (1) (1)
IPlayerState(16-40)renderer/viewer/lib/worldrendererCommon.ts (1) (1)
WorldRendererConfig(51-51)renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-601)
renderer/viewer/lib/worldrendererThree.ts (4)
renderer/viewer/lib/entities.ts (1) (1)
Entities(209-1037)src/appViewer.ts (3) (3)
GraphicsInitOptions(37-42)DisplayWorldOptions(44-48)RendererReactiveState(16-24)renderer/viewer/lib/basePlayerState.ts (1) (1)
ItemSpecificContextProperties(9-9)renderer/viewer/lib/hand.ts (1) (1)
getMyHand(5-35)
src/watchOptions.ts (3)
src/globalState.ts (1) (1)
miscUiState(109-129)src/optionsStorage.ts (2) (2)
watchValue(213-235)options(186-191)renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(21-249)
renderer/viewer/lib/entities.ts (1)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-601)
src/appViewer.ts (6)
src/core/progressReporter.ts (8) (8)
error(131-133)error(156-159)error(178-180)error(193-195)error(215-217)error(227-228)ProgressReporter(5-18)createNotificationProgressReporter(165-182)renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(21-249)renderer/viewer/lib/worldrendererCommon.ts (2) (2)
WorldRendererConfig(51-51)defaultWorldRendererConfig(32-49)renderer/viewer/lib/basePlayerState.ts (1) (1)
IPlayerState(16-40)src/mineflayer/playerState.ts (1) (1)
playerState(201-201)src/watchOptions.ts (1) (1)
watchOptionsAfterWorldViewInit(110-118)
src/index.ts (1)
src/appViewer.ts (2) (2)
loadBackend(87-115)appViewer(190-190)
src/vr.ts (2)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-601)src/appViewer.ts (1) (1)
appViewer(190-190)
🪛 ESLint
renderer/viewer/lib/worldrendererThree.ts
[error] 363-363: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (62)
renderer/viewer/lib/mesher/mesher.ts (1)
124-126: Improved robustness with null/undefined checkAdding a check for undefined or null state IDs prevents potential errors when updating blocks. This defensive programming practice aligns well with the overall PR goal of improving stability.
renderer/viewer/baseGraphicsBackend.ts (1)
1-11: Good foundation for reactive state managementThis new utility function creates a consistent default state object for the renderer, which will help maintain a predictable state across the application. The structure includes essential world metrics and renderer settings.
src/mineflayer/maps.ts (2)
3-3: Added import for new renderer methods utilityThe import aligns with the PR's goal of creating a more stable renderer layer by accessing rendering capabilities through a dedicated utility function.
21-21: Improved abstraction with optional chainingReplacing direct viewer entity access with
getThreeJsRendererMethods()?.updateMap(id, png)creates a cleaner separation between app and renderer. The optional chaining handles cases where the renderer might not be available.renderer/viewer/three/threeJsMethods.ts (1)
1-15: Well-designed renderer method abstraction layerThis utility function creates an elegant abstraction for accessing Three.js specific renderer methods. The implementation has several strengths:
- Type safety with proper return type declaration
- Safe handling when the renderer isn't available or is a different type
- Using a Proxy to maintain the asynchronous nature of the methods
- Clean error handling with optional chaining
This approach supports the PR's goal of creating a stable layer between the app and renderer components.
src/customChannels.ts (1)
54-54: Excellent simplification of custom block updatingThe refactoring replaces complex, hard-coded block model management with a single method call, which aligns perfectly with the PR's goal of separating application and rendering code.
This change greatly improves maintainability by abstracting rendering-specific operations behind a proper method boundary.
src/mineflayer/items.ts (1)
4-7: New imports to support item model resolutionThese imports are necessary for the new
getItemModelNamefunction.src/rendererUtils.ts (2)
77-77: Proper abstraction for FOV updatesThis change moves from directly manipulating camera properties to updating a configuration object, following the separation of concerns principle.
However, there's no import for
appViewerin this file. Make sure to add:+ import { appViewer } from './appViewer'
93-93: Improved camera update mechanismThe change from
viewer.setFirstPersonCameratoappViewer.backend?.updateCameracorrectly abstracts the camera positioning logic.The optional chaining (
?.) is good defensive programming in case the backend is not yet initialized.renderer/viewer/lib/basePlayerState.ts (4)
25-25: Adding getPosition method enhances position tracking capabilities.This new method provides a standard way to access the player's position across different implementations, which is essential for consistent position-based rendering and game mechanics.
35-38: Good addition of rendering-related reactive properties.These new reactive properties (inWater, backgroundColor, ambientLight, directionalLight) properly separate visual state from core player state, allowing for more dynamic rendering effects based on player conditions.
45-48: Correct implementation of reactive properties with sensible defaults.The initialization of the new reactive properties with appropriate default values ensures consistency across implementations while providing a clean starting state.
86-88: Base implementation of getPosition returns a default position.This implementation provides a reasonable default for the base class. Derived classes should override this method to provide actual position data.
Ensure all implementations that extend BasePlayerState override this method to return meaningful position data, as relying on the default Vec3(0,0,0) would cause rendering issues.
src/resourcesManager.ts (5)
1-22: Good modular design with typed events.Using TypedEmitter with well-defined event types enhances type safety and makes the event system more maintainable.
23-46: Well-structured resource containment class.The LoadedResources class provides a clean encapsulation of different resource types with appropriate typing. The organization separates atlas parsers from user-specific data, which is good for maintainability.
47-52: Clear interface for asset updates.The UpdateAssetsRequest interface provides a well-defined contract for updating assets, with appropriate options for filtering and versioning.
54-64: ResourcesManager correctly extends EventEmitter with typed events.The class properly extends EventEmitter with typed events and initializes source data with reasonable defaults.
177-181: Clean resource management in destroy method.The destroy method properly cleans up resources and resets state, which is essential for preventing memory leaks.
renderer/viewer/lib/holdingBlock.ts (4)
3-3: Updated imports support the renderer refactoring.The addition of WorldBlockProvider and WorldRendererThree imports align with the PR's objective of improving the separation between app and renderer.
Also applies to: 13-13
118-120: Added properties maintain original functionality.These properties ensure the HoldingBlock class maintains access to playerState and config while following the new architecture.
121-129: Constructor refactored to work with WorldRendererThree.The constructor now properly extracts the playerState and config from the worldRenderer, maintaining the original functionality while simplifying dependencies.
334-341: Updated method call to use worldRenderer.The change from using viewer directly to worldRenderer.entities follows the new architecture pattern, improving modularity.
src/watchOptions.ts (9)
5-9: Updated imports support new architecture.The imports now include WorldDataEmitter and isCypress, which align with the PR's goals of improving modularity and providing better testing support.
20-26: Extracted updateTouch function improves reusability.Moving the touch detection logic to a standalone function makes the code more maintainable and reduces duplication when the same logic is needed in multiple places.
30-32: Updated references from viewer to appViewer.These changes align with the PR's objective of moving away from direct viewer access to a more modular structure with appViewer as the central point of coordination.
Also applies to: 35-36, 39-40, 43-50
54-77: Added window focus tracking for FPS management.This new feature intelligently manages FPS limits based on window focus state, which can significantly improve battery life and performance on devices when the application is in the background.
80-81: Updated FPS limit watcher to use new function.The watcher now calls the updateFpsLimit function, which centralizes the FPS limit logic and ensures consistency.
84-86: Updated worldRenderer config references.The property paths have been updated to use appViewer.inWorldRenderingConfig, aligning with the new architecture.
88-95: Updated lighting configuration handlers.These changes ensure that lighting settings are properly applied through the new appViewer interface.
106-107: Commented out neighborChunkUpdates setting.This line is commented out, which might indicate an incomplete implementation or a feature that was intentionally disabled.
Is this feature intentionally disabled? If not, consider uncommenting and implementing the property:
- // appViewer.inWorldRenderingConfig.neighborChunkUpdates = o.neighborChunkUpdates + appViewer.inWorldRenderingConfig.neighborChunkUpdates = o.neighborChunkUpdates
110-117: Updated watchOptionsAfterWorldViewInit with proper parameter typing.The function now accepts a properly typed WorldDataEmitter parameter and updates the configuration properties via appViewer.
renderer/viewer/lib/worldrendererCommon.ts (7)
35-47: Good additions to configuration options to improve rendering flexibility.These new configuration options provide more control over the rendering pipeline, which aligns well with the architectural refactoring goals outlined in the PR objectives.
141-142: Great addition of resource management.The addition of
itemsTextureandsoundSystemproperties centralizes resource management, which is a key improvement in this refactoring.
148-159: Well-designed constructor with improved dependency injection.The constructor now takes a
resourcesManager,displayOptions, andversioninstead of a config object. This change correctly separates concerns and allows for better modularity with clear responsibility boundaries.
161-171: Good implementation of initialization logic with proper error handling.The init method properly checks if the renderer is already active before initialization and sets up appropriate event listeners for asset updates.
366-370: Resource loading pattern improved.The refactored resource loading pattern is more maintainable, using the centralized
resourcesManagerto load and manage Minecraft data.
391-415: Asset update handling is now more modular.The
updateAssetsDatamethod now correctly pulls data from the centralized resource manager, which improves maintainability and reduces duplicated code.
749-766: Good resource cleanup in destroy method.The
destroymethod now properly cleans up all resources, including terminating workers and destroying the sound system, which helps prevent memory leaks.renderer/viewer/lib/entities.ts (9)
26-27: Good refactoring to use WorldRendererThree.Importing the
WorldRendererThreeclass directly rather than using the now-removedViewerclass is a good architectural change.
212-216: Good default initialization for font family.Setting a default
fontFamilyof 'mojangles' ensures consistent rendering across the application.
219-219: Improved naming from 'rendering' to 'currentlyRendering'.The rename to
currentlyRenderingis more descriptive and accurately represents the current state.
250-250: Constructor updated to use WorldRendererThree.The constructor now correctly accepts
worldRendererof typeWorldRendererThreeinstead of aviewerinstance, aligning with the new architecture.
257-263: Entity cleanup now uses consistent world renderer reference.The
clearmethod now correctly usesthis.worldRenderer.sceneto remove entities from the scene.
277-286: Entity rendering toggle properly updated.The
setRenderingmethod now usesthis.currentlyRenderingandthis.worldRenderer.sceneconsistently.
289-294: Good configuration access through worldRenderer.The
rendermethod now correctly accesses the configuration throughthis.worldRenderer.worldRendererConfig.
1055-1055: Function signature updated for addArmorModel.The
addArmorModelfunction now correctly acceptsworldRendereras its first parameter, matching the new architecture.
1089-1089: Resource access through the resource manager.The armor texture is now correctly accessed through
worldRenderer.resourcesManager.currentResources.src/index.ts (9)
97-100: Good imports for the new architecture.Importing
appViewerandcreateGraphicsBackendaligns with the new architectural approach, making the code more modular.
118-132: Well-structured backend loading function.The
loadBackendfunction properly initializes the graphics backend and handles conditional loading based on the build mode.
162-171: Good implementation of focus handling.This block ensures the application starts loading when the user focuses on the window, which improves user experience.
305-306: Resource loading now uses appViewer.The resource loading now correctly uses
appViewer.resourcesManager.loadSourceDatainstead of direct loading.
328-330: Resource preparation refactored.Resource preparation now uses
appViewer.prepareResourcesinstead of the previous approach with direct viewer access.
691-694: Good implementation of world initialization.The world initialization now correctly uses
appViewer.startWorldandappViewer.worldView.listenToBotto connect the world view to the bot.
699-704: Camera position update correctly implemented.The camera position update now properly uses
appViewer.backend?.updateCameraandappViewer.worldView.updatePosition.
717-738: Good chunk loading status tracking.The chunk loading status is now correctly tracked through
appViewer.backend.reactiveState, which provides a more reactive approach to monitoring loading progress.
742-742:❓ Verification inconclusive
Consider re-enabling waitForChunks.
The
waitForChunksfunction is commented out, but it might be useful for ensuring all chunks are loaded before proceeding, especially in singleplayer mode where waiting for chunk loading improves the user experience.Check if
waitForChunksshould be re-enabled or if there's a new mechanism to ensure chunks are loaded before proceeding. If it's intentionally disabled as part of a performance improvement, consider adding a comment explaining the rationale.
🏁 Script executed:
#!/bin/bash # Look for references to waitForChunks in the codebase to see if it's used elsewhere rg -A 3 -B 3 "waitForChunks" --glob "!node_modules/**"Length of output: 3812
Action: Verify and Document Chunk Waiting Behavior
The
waitForChunksfunction insrc/index.tsis defined but its invocation (// await waitForChunks()) remains commented out. Our investigation shows that:
- Other parts of the codebase (e.g., in
renderer/viewer/three/panorama.tsandrenderer/playground/*) are already usingwaitForChunksToRenderto manage chunk rendering.- The configuration in
src/optionsStorage.ts(withwaitForChunksRender: 'sp-only') indicates that chunk-waiting might be intended only for singleplayer mode.- The early return in the
waitForChunksfunction (if (appQueryParams.sp === '1') return //todo) suggests that its behavior in singleplayer mode is still under deliberation.Next Steps:
- If the current mechanism (
waitForChunksToRender) is sufficient: Please add a comment explaining why thewaitForChunkscall insrc/index.tsis intentionally left disabled.- If waiting for chunks in the index file is still needed: Uncomment the line and add documentation to clarify its role and any performance implications, especially regarding mode-specific behavior.
src/appViewer.ts (2)
87-115: Consider error-handling around backend loading.Within
loadBackend, if backend initialization fails, the code just logs the error. For large-scale applications, you might handle it more robustly (e.g., fallback strategy or user-facing error message) to avoid silent failures.
190-192: GlobalappViewerinstance.Exporting a single global instance is convenient but can lead to hidden state issues if anything else tries to instantiate
AppViewer. Ensure thread-safety if concurrency arises in the future.src/vr.ts (2)
12-19: Improved dependency injection with worldRenderer parameter.Good refactoring to accept a
worldRendererparameter instead of relying on global variables, which makes the code more maintainable and testable.
195-196: Camera update code is commented out.Line 195 is commented out and replaced with a different approach in line 196. This suggests an incomplete migration or a temporary workaround. The current code updates the camera to match bot.entity position, which might not be the original intent.
Please review whether the camera update approach in line 196 is the intended implementation or if line 195 should be uncommented. If this is intentional, consider removing the commented code.
- // appViewer.backend?.updateCamera(null, yawOffset, 0) appViewer.backend?.updateCamera(null, bot.entity.yaw, bot.entity.pitch)renderer/viewer/lib/worldrendererThree.ts (2)
165-188: Demo methods contain unnecessary ts-expect-error directives.The
demoModelanddemoItemmethods use@ts-expect-errorwithout clear indication of what errors are being suppressed. Additionally, they reference an undefinedcursorBlockRelfunction.These methods should either:
- Be properly typed to avoid TS errors, or
- Include comments explaining the suppressed errors
- Define or import the missing
cursorBlockRelfunction
584-596: Well-implemented camera roll functionality.The
setCameraRollmethod is a good addition that implements proper quaternion-based camera rotation, which will be useful for effects like view bobbing and vehicle movements.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
renderer/viewer/three/graphicsBackend.ts (2)
64-66:⚠️ Potential issueAvoid global variable assignments that break encapsulation.
The code assigns to multiple global variables (
window.viewer,window.world,window.viewer.world), which could lead to namespace conflicts and unexpected behavior. This contradicts the PR's goal of improving modularity.- window.viewer ??= {} - window.world = worldRenderer - window.viewer.world = worldRenderer + // If needed for debugging/access, use a single namespaced object + if (process.env.NODE_ENV === 'development') { + const debugGlobals = (window.__mcDebug = window.__mcDebug || {}) + debugGlobals.viewer = debugGlobals.viewer || {} + debugGlobals.world = worldRenderer + debugGlobals.viewer.world = worldRenderer + }
51-53:⚠️ Potential issueThe
prepareResourcesmethod is incomplete.This method only stores the version but doesn't actually prepare any resources. Given the method name and return type (Promise), users would expect it to perform some asynchronous resource preparation.
const prepareResources = async (ver: string, progressReporter: ProgressReporter): Promise<void> => { version = ver + // Implement resource preparation, for example: + try { + await progressReporter.executeWithMessage('Loading resources', async () => { + await initOptions.resourcesManager.loadResources(ver) + }) + } catch (error) { + console.error('Failed to prepare resources:', error) + initOptions.displayCriticalError(error instanceof Error ? error : new Error('Failed to prepare resources')) + } }renderer/viewer/lib/worldrendererThree.ts (1)
165-188:⚠️ Potential issueDemo methods contain unnecessary ts-expect-error directives.
The
demoModelanddemoItemmethods use@ts-expect-errorwithout clear indication of what errors are being suppressed. Additionally, they reference an undefinedcursorBlockRelfunction.These methods should either:
- Be properly typed to avoid TS errors, or
- Include comments explaining the suppressed errors
- Define or import the missing
cursorBlockRelfunction- //@ts-expect-error - const pos = cursorBlockRel(0, 1, 0).position + // Define a proper type or position calculation + const pos = { x: 0, y: 1, z: 0 }
🧹 Nitpick comments (28)
renderer/viewer/lib/mesher/mesher.ts (1)
124-126: Handle invalid state IDs more explicitly?
The newly added check for undefined or null state IDs helps avoid potential runtime errors. However, you might consider logging or otherwise handling these cases more thoroughly to aid debugging when a state ID is not provided.src/mineflayer/maps.ts (1)
21-21: Consider adding fallback handling for map updatesThe optional chaining prevents errors when the renderer methods are unavailable, but map updates would silently fail in that case. Consider adding error handling or logging to notify when map updates can't be processed.
- getThreeJsRendererMethods()?.updateMap(id, png) + const methods = getThreeJsRendererMethods() + if (methods) { + methods.updateMap(id, png) + } else { + console.warn('Unable to update map: Three.js renderer methods not available') + }renderer/viewer/three/threeJsMethods.ts (1)
4-15:❓ Verification inconclusive
Review renderer access mechanism
The implementation provides a clean abstraction for accessing Three.js renderer methods, but has a few areas that could be improved:
The function relies on a non-standard property
__isThreeJsRendererfor type checking. Consider using a more explicit mechanism if possible.The
appViewervariable is used but not imported in the visible code, suggesting it might be a global variable. This could complicate testing and encapsulation.The Proxy makes all method calls asynchronous, which is good for consistency but could be inefficient for methods that don't need to be async.
export function getThreeJsRendererMethods (): ThreeJsBackendMethods | undefined { const renderer = appViewer.backend - if (!renderer?.['__isThreeJsRenderer'] || !renderer.backendMethods) return + // More explicit type checking + if (!renderer || !('__isThreeJsRenderer' in renderer) || !renderer.__isThreeJsRenderer || !renderer.backendMethods) return return new Proxy(renderer.backendMethods, { get (target, prop) { return async (...args) => { const result = await (target[prop as any] as any)(...args) return result } } }) as ThreeJsBackendMethods }
🌐 Web query:
What are best practices for type checking in TypeScript when dealing with duck typing?💡 Result:
TypeScript's approach to duck typing (structural typing) requires specific practices to maintain type safety while leveraging flexibility. Here are the key best practices:
Compile-Time Type Safety
Use interfaces for structural contracts
Define clear interfaces to enforce expected object shapes while allowing structural compatibility[6]. This leverages TypeScript's core principle of structural subtyping:interface User { id: string; email: string; } function welcomeUser(user: User) { // TypeScript verifies any passed object has these properties }Prefer type aliases/intersections over
any
Avoid losing type safety withanywhen possible. Use explicit types:type Admin = User & { privileges: string[] }; // Combine types safely[7]Implement exhaustive union checks
When handling multiple potential types:type Shape = Circle | Square | Triangle; function getArea(shape: Shape): number { switch(shape.kind) { case 'circle': return Math.PI * shape.radius ** 2; case 'square': return shape.size ** 2; case 'triangle': return (shape.base * shape.height)/2; default: const _exhaustiveCheck: never = shape; return _exhaustiveCheck; // Triggers error if new type added[5] } }Runtime Validation
Use type predicates for unknown data
Validate external data at runtime while maintaining type safety[2][6]:interface APIResponse { data: unknown; } function isUser(obj: unknown): obj is User { return !!obj && typeof obj === 'object' && 'id' in obj && 'email' in obj; } function processResponse(response: APIResponse) { if (isUser(response.data)) { console.log(`Welcome ${response.data.email}`); // Now type-safe } }Implement brand checking for critical types
Prevent structural matches with unintended types using branding:type UserID = string & { readonly brand: unique symbol }; function createUser(id: string): UserID { return id as UserID; // Explicit branding prevents accidental misuse[8] }Structural Typing Considerations
Leverage utility types wisely
Transform types while maintaining safety:type PartialUser = Partial<User>; // All properties optional[7] type RequiredUser = Required<User>; // All properties mandatory function updateUser(id: string, updates: PartialUser) { // Safely handle partial updates }Handle excess properties cautiously
Use explicit type annotations to catch unwanted properties:interface Point { x: number; y: number; } const p: Point = { x: 1, y: 2, z: 3 // Error: Object literal may only specify known properties[6] };Best Practice Checklist
Validation layers
Implement input validation at system boundaries using type predicates[2][6]Avoid empty interfaces
Useneveror branded types instead of empty contracts[7]Precise generic constraints
Use generics with structural requirements:function getProperty<T, K extends keyof T>(obj: T, key: K) { return obj[key]; // Type-safe property access }Union type exhaustiveness checks
Usenevertype indefaultcases to catch unhandled types[5]Prefer
unknownoverany
Force explicit type checks before usage[3][7]:function safeParse(data: unknown): User { if (isUser(data)) return data; throw new Error('Invalid user data'); }These practices combine TypeScript's compile-time checks with runtime validation to achieve robust duck typing implementations while preventing common structural typing pitfalls like unintended type compatibility[8]. The key is leveraging TypeScript's type system to create self-documenting code that fails fast and provides clear feedback during development.
Citations:
- 1: https://zerotomastery.io/blog/typescript-type-checking/
- 2: https://javascript.plainenglish.io/what-is-duck-typing-in-typescript-c537d2ff9b61
- 3: https://www.typescriptlang.org/docs/handbook/declaration-files/do-s-and-don-ts.html
- 4: https://www.tpointtech.com/typescript-duck-typing
- 5: https://dev.to/zenstack/typescript-best-type-checking-for-the-best-type-safety-1lcb
- 6: https://www.typescriptlang.org/docs/handbook/interfaces.html
- 7: https://docs.aws.amazon.com/prescriptive-guidance/latest/best-practices-cdk-typescript-iac/typescript-best-practices.html
- 8: https://dev.to/mistomin/typescript-type-compatibility-vs-duck-typing-62d
- 9: https://en.wikipedia.org/wiki/Duck_typing
Renderer Access Abstraction – Verify and Enhance
The updated type check using the
inoperator is a step in the right direction. However, please consider the following points based on TypeScript’s best practices for duck typing and overall design:
Type Checking Improvement:
Instead of scattering inline checks, a dedicated type guard can help clarify intent and improve code reuse. For example, implementing a function such as:function isThreeJsRenderer(renderer: any): renderer is { __isThreeJsRenderer: boolean; backendMethods: any } { return renderer && typeof renderer === 'object' && '__isThreeJsRenderer' in renderer && renderer.__isThreeJsRenderer && !!renderer.backendMethods; }would allow you to simplify the check to:
export function getThreeJsRendererMethods (): ThreeJsBackendMethods | undefined { const renderer = appViewer.backend - if (!renderer?.['__isThreeJsRenderer'] || !renderer.backendMethods) return + if (!isThreeJsRenderer(renderer)) return return new Proxy(renderer.backendMethods, { get (target, prop) { return async (...args) => { const result = await (target[prop as any] as any)(...args) return result } } }) as ThreeJsBackendMethods }Global Dependency Concern:
The use ofappViewerwithout an explicit import indicates it might be a global. This can complicate unit testing and reduce encapsulation. Consider passing it as a parameter or importing it explicitly to improve clarity and maintainability.Asynchronous Proxy Overhead:
Wrapping every method in an async function ensures consistency but may add unnecessary overhead for methods that operate synchronously. Evaluate whether all backend methods genuinely need asynchronous handling or if selective wrapping might be more efficient.Please verify these suggestions in the context of the overall system design and usage patterns.
src/customChannels.ts (1)
54-54: Great simplification of custom block handlingReplacing complex block model management with a single method call significantly simplifies the code while maintaining functionality. This is an excellent example of the benefits of the new abstraction layer introduced in this PR.
Consider adding error handling in case the renderer methods are unavailable:
- getThreeJsRendererMethods()?.updateCustomBlock(chunkKey, blockPosKey, model) + const methods = getThreeJsRendererMethods() + if (methods) { + methods.updateCustomBlock(chunkKey, blockPosKey, model) + } else { + console.warn(`Unable to update custom block at ${blockPosKey}: Three.js renderer methods not available`) + }renderer/viewer/three/appShared.ts (1)
26-29: Consider adding type validation for itemThe spread operator is used to merge item properties, but there's no validation that
itemcontains all required properties forGeneralInputItem. Consider adding runtime validation or using a more specific type for theitemparameter.- const model = getItemModelName({ - ...item, - name, - } as GeneralInputItem, specificProps) + // Ensure item has required properties for GeneralInputItem + const itemWithName: GeneralInputItem = { + ...item, + name, + } + const model = getItemModelName(itemWithName, specificProps)src/resourcesManager.ts (3)
74-164: Consider splitting updateAssetsData into smaller functionsThe
updateAssetsDatamethod is quite long (90+ lines) and handles multiple concerns: loading source data, creating texture atlases, handling seasonal textures, etc. Consider extracting some of these operations into helper methods for better maintainability.For example, you could extract atlas creation logic:
private async createBlocksAtlas(resources: LoadedResources, request: UpdateAssetsRequest): Promise<{atlas: any, canvas: HTMLCanvasElement}> { console.time('createBlocksAtlas') const blockTexturesChanges = this.getSeasonalTextureChanges() const customBlockTextures = Object.keys(resources.customTextures.blocks?.textures ?? {}) const result = await this.sourceBlocksAtlases.makeNewAtlas( resources.texturesVersion, (textureName) => { if (request.includeOnlyBlocks && !request.includeOnlyBlocks.includes(textureName)) return false const texture = resources.customTextures.blocks?.textures[textureName] return blockTexturesChanges[textureName] ?? texture }, undefined, undefined, customBlockTextures ) console.timeEnd('createBlocksAtlas') return result } private getSeasonalTextureChanges(): Record<string, string> { const blockTexturesChanges = {} as Record<string, string> const date = new Date() if ((date.getMonth() === 11 && date.getDate() >= 24) || (date.getMonth() === 0 && date.getDate() <= 6)) { Object.assign(blockTexturesChanges, christmasPack) } return blockTexturesChanges }
65-72: Use async initialization pattern for loadSourceDataThe method
loadSourceDatacallsloadMcDataand sets a class property, but doesn't handle errors. Consider using a more robust async initialization pattern that properly handles errors and returns the instance.- async loadSourceData (version: string) { - await this.loadMcData(version) - this.sourceBlockStatesModels ??= await importLargeData('blockStatesModels') - } + async loadSourceData (version: string): Promise<void> { + try { + await this.loadMcData(version) + this.sourceBlockStatesModels ??= await importLargeData('blockStatesModels') + } catch (error) { + console.error(`Failed to load source data for version ${version}:`, error) + throw new Error(`Failed to load source data: ${error.message}`) + } + }
166-181: Add a method to check if resources are loadedThe
downloadDebugAtlasmethod throws an error if resources aren't loaded. Consider adding a helper method to check if resources are available to make error checking more consistent across the codebase.+ /** + * Returns the current resources or throws an error if not loaded + */ + getResources(): LoadedResources { + if (!this.currentResources) { + throw new Error('No resources loaded') + } + return this.currentResources + } async downloadDebugAtlas (isItems = false) { - const resources = this.currentResources - if (!resources) throw new Error('No resources loaded') + const resources = this.getResources() const atlasParser = (isItems ? resources.itemsAtlasParser : resources.blocksAtlasParser)! // ... rest of the method }renderer/viewer/three/graphicsBackend.ts (3)
46-46: Use caution with global assignments likewindow.panoramaRenderer.This assignment creates a global variable which could lead to namespace conflicts and makes testing more difficult. Consider removing it or using a namespaced debug object if it's only needed for debugging.
- window.panoramaRenderer = panoramaRenderer + // If needed for debugging + if (process.env.NODE_ENV === 'development') { + window.__mcDebug = window.__mcDebug || {} + window.__mcDebug.panoramaRenderer = panoramaRenderer + }
85-86: Avoid using@ts-expect-errorfor non-standard properties.Using
@ts-expect-errorto add a non-standard property can lead to type safety issues. Either update the interface definition or use a symbol or WeakMap for storing metadata.const backend: GraphicsBackend = { - //@ts-expect-error mark as three.js renderer - __isThreeJsRenderer: true, + // Use a proper type guard function instead NAME: `three.js ${THREE.REVISION}`,At the module level, you could add:
// Type guard to check if a backend is a ThreeJS backend export function isThreeJsRenderer(backend: GraphicsBackend): backend is GraphicsBackend & ThreeJsBackendMethods { return backend.NAME.startsWith('three.js'); }
23-114: Consider using a class instead of a factory function.The current implementation uses a factory function with closures to maintain private state. Consider using a class with private members for better encapsulation and readability.
export class ThreeJsGraphicsBackend implements GraphicsBackend { readonly NAME: string; private documentRenderer: DocumentRenderer; private panoramaRenderer: PanoramaRenderer | null = null; private worldRenderer: WorldRendererThree | null = null; private version = ''; public readonly reactiveState: RendererReactiveState; constructor(private initOptions: GraphicsInitOptions) { this.NAME = `three.js ${THREE.REVISION}`; this.documentRenderer = new DocumentRenderer(initOptions); // Initialize reactiveState with proxy... this.reactiveState = proxy({ // Same implementation as in the original code }); if (process.env.NODE_ENV === 'development') { // For debugging only (window.__mcDebug = window.__mcDebug || {}).threeBackend = this; } } startPanorama(): void { // Implementation } // Other methods as in the original code... get backendMethods(): ThreeJsBackendMethods | undefined { if (!this.worldRenderer) return undefined; return getBackendMethods(this.worldRenderer); } } // Export the factory function that creates the backend export default function createGraphicsBackend(initOptions: GraphicsInitOptions): GraphicsBackend { return new ThreeJsGraphicsBackend(initOptions); }src/watchOptions.ts (2)
43-50: Consider using an enum or object map for statsVisible values.The code uses numeric values (0, 1, 2) for
appViewer.config.statsVisible, which might be unclear to other developers. Consider using an enum or object map for better code readability.+ // At the top of the file or in a constants module + const StatsVisibility = { + NONE: 0, + BASIC: 1, + ADVANCED: 2 + } as const; watchValue(options, o => { const { renderDebug } = o if (renderDebug === 'none' || isCypress()) { - appViewer.config.statsVisible = 0 + appViewer.config.statsVisible = StatsVisibility.NONE } else if (o.renderDebug === 'basic') { - appViewer.config.statsVisible = 1 + appViewer.config.statsVisible = StatsVisibility.BASIC } else if (o.renderDebug === 'advanced') { - appViewer.config.statsVisible = 2 + appViewer.config.statsVisible = StatsVisibility.ADVANCED } })
70-77: Consider debouncing the focus/blur event handlers.Event handlers for window focus/blur might be called frequently in some scenarios (like when a user is switching between applications). Consider debouncing these event handlers to avoid frequent updates to the FPS limit.
const debounce = (fn: Function, delay: number) => { let timeoutId: number; return (...args: any[]) => { clearTimeout(timeoutId); timeoutId = window.setTimeout(() => fn(...args), delay); }; }; const handleFocus = debounce(() => { windowFocused = true; updateFpsLimit(options); }, 100); const handleBlur = debounce(() => { windowFocused = false; updateFpsLimit(options); }, 100); window.addEventListener('focus', handleFocus); window.addEventListener('blur', handleBlur);renderer/viewer/lib/entities.ts (2)
223-234: Commented out getItemUv property type.There is a commented-out type definition for
getItemUv. If this property is no longer needed, the comments should be removed entirely to avoid confusion.- // getItemUv: undefined | (() => { - // texture: THREE.Texture; - // u: number; - // v: number; - // su?: number; - // sv?: number; - // size?: number; - // modelName?: string; - // } | { - // resolvedModel: BlockModel - // modelName: string - // } | undefined)
535-539: Code formatting in getItemMesh method.The destructuring assignment and variable declaration across multiple lines with different indentation makes the code less readable. Consider formatting this in a more consistent way.
- const textureThree = textureUv.renderInfo?.texture === 'blocks' ? this.worldRenderer.material.map! : this.itemsTexture! - // todo use geometry buffer uv instead! - const { u, v, su, sv, texture } = textureUv - const size = undefined - const itemsTexture = textureThree.clone() + // todo use geometry buffer uv instead! + const textureThree = textureUv.renderInfo?.texture === 'blocks' + ? this.worldRenderer.material.map! + : this.itemsTexture!; + const { u, v, su, sv, texture } = textureUv; + const size = undefined; + const itemsTexture = textureThree.clone();src/vr.ts (2)
19-19: Invocation ofenableVr()with no parameters is neat.
Consider passing optional config flags if the VR setup steps expand in the future.
88-88: Appropriately togglingvrSupportoff.
If intended to fully exit VR mode, ensure any post-VR cleanup (e.g., resetting camera or controls) is invoked at call sites.renderer/viewer/lib/worldrendererCommon.ts (5)
39-48: Extended WorldRenderer config fields.
Fields likevrSupport,addChunksBatchWaitTime, and various rendering booleans are helpful.
Just ensure that all new config keys have documented defaults and usage across the codebase.
391-392:updateAssetsData()method expansions.
- Setting
mesherConfig.textureSizefrom the blocks atlas.- Broadcasting mesher data to workers.
These changes unify the resource pipeline. Be sure to handle errors ifitemsRendereris missing or incomplete.Also applies to: 394-394, 397-399
493-510:setBlockStateIdwith chunk queuing logic.
The approach is thorough, deferring block updates until chunk loading is done. Watch for memory usage if many updates queue at once.
515-609:connect(worldView: WorldDataEmitter)method introduction.
Centralizing all event bindings (loadChunk, blockUpdate, unloadChunk, etc.) is a big improvement in maintainability.
One caution: IfWorldRendererThreehas special logic, ensure it doesn’t overshadow standard hooking.
611-657:setBlockStateIdInnerhandles neighbor chunk updates thoroughly.
You do a lot of boundary checks (pos.x & 15). Ensure performance is not impacted by repeated calls on large builds.src/inventoryWindows.ts (1)
224-224: Item rendering logic improved.
- Confirming
itemsRendereris defined withassertDefined.- Fallback to
missing_textureorblock/erroredis a robust approach.
Make sure repeated in-game errors don’t spam logs.Also applies to: 226-227, 230-231
src/appViewer.ts (2)
139-158: Consider adding validation for duplicate world starts.The
startWorldmethod correctly checks if a world is already started, but doesn't ensure that the current display isn't still showing a panorama. This could lead to unexpected behavior if called improperly.startWorld (world, renderDistance: number, playerStateSend: IPlayerState = playerState) { if (this.currentDisplay === 'world') throw new Error('World already started') + if (this.currentDisplay === 'menu') { + this.disconnectBackend() + } this.currentDisplay = 'world' // Rest of the method remains unchanged
193-206: Clean up commented code in initialMenuStart.There's a significant block of commented-out code that appears to be testing/development code. Since this is a major refactoring, consider removing this code to keep the codebase clean, or add a TODO explaining its purpose.
const initialMenuStart = async () => { if (appViewer.currentDisplay === 'world') { appViewer.resetBackend() } appViewer.startPanorama() - // await appViewer.resourcesManager.loadMcData('1.21.4') - // const world = getSyncWorld('1.21.4') - // await appViewer.prepareResources('1.21.4', createNullProgressReporter()) - // world.setBlockStateId(new Vec3(0, 64, 0), 1) - // appViewer.startWorld(world, 3, new Vec3(0, 64, 0), new BasePlayerState()) - // appViewer.backend?.updateCamera(new Vec3(0, 64, 2), 0, 0) - // void appViewer.worldView.init(new Vec3(0, 64, 0)) }renderer/viewer/lib/worldrendererThree.ts (3)
343-352: Clean up commented code in updateCamera.There's commented code related to
freeFlyModethat should be removed or properly implemented. Commented code blocks can make maintenance more difficult, especially in a significant refactoring like this.updateCamera (pos: Vec3 | null, yaw: number, pitch: number): void { - // if (this.freeFlyMode) { - // pos = this.freeFlyState.position - // pitch = this.freeFlyState.pitch - // yaw = this.freeFlyState.yaw - // } if (pos) { new tweenJs.Tween(this.camera.position).to({ x: pos.x, y: pos.y, z: pos.z }, 50).start() - // this.freeFlyState.position = pos } this.camera.rotation.set(pitch, yaw, this.cameraRoll, 'ZYX') }
356-371: Good camera handling in render method.The render method now properly handles size changes and FOV settings, ensuring that the camera is configured correctly before rendering. However, there's another commented reference to
freeFlyModethat should be addressed.- if (this.displayOptions.inWorldRenderingConfig.showHand/* && !this.freeFlyMode */) { + if (this.displayOptions.inWorldRenderingConfig.showHand) { this.holdingBlock.render(this.camera, this.renderer, this.ambientLight, this.directionalLight) this.holdingBlockLeft.render(this.camera, this.renderer, this.ambientLight, this.directionalLight) }🧰 Tools
🪛 ESLint
[error] 363-363: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
363-363: Remove unused ESLint disable comment.The ESLint directive
@typescript-eslint/non-nullable-type-assertion-styleis disabled but never used, according to static analysis.- // eslint-disable-next-line @typescript-eslint/non-nullable-type-assertion-style const cam = this.camera instanceof THREE.Group ? this.camera.children.find(child => child instanceof THREE.PerspectiveCamera) as THREE.PerspectiveCamera : this.camera🧰 Tools
🪛 ESLint
[error] 363-363: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
renderer/viewer/baseGraphicsBackend.ts(1 hunks)renderer/viewer/lib/basePlayerState.ts(3 hunks)renderer/viewer/lib/entities.ts(18 hunks)renderer/viewer/lib/holdingBlock.ts(5 hunks)renderer/viewer/lib/mesher/mesher.ts(1 hunks)renderer/viewer/lib/worldrendererCommon.ts(11 hunks)renderer/viewer/lib/worldrendererThree.ts(12 hunks)renderer/viewer/three/appShared.ts(1 hunks)renderer/viewer/three/graphicsBackend.ts(1 hunks)renderer/viewer/three/panorama.ts(1 hunks)renderer/viewer/three/threeJsMethods.ts(1 hunks)src/appViewer.ts(1 hunks)src/cameraRotationControls.ts(2 hunks)src/customChannels.ts(2 hunks)src/index.ts(15 hunks)src/inventoryWindows.ts(6 hunks)src/mineflayer/items.ts(2 hunks)src/mineflayer/maps.ts(2 hunks)src/mineflayer/playerState.ts(4 hunks)src/react/DebugOverlay.tsx(8 hunks)src/rendererUtils.ts(2 hunks)src/resourcesManager.ts(1 hunks)src/sounds/botSoundSystem.ts(2 hunks)src/vr.ts(5 hunks)src/watchOptions.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- renderer/viewer/three/panorama.ts
- src/sounds/botSoundSystem.ts
- src/cameraRotationControls.ts
- src/mineflayer/playerState.ts
- src/react/DebugOverlay.tsx
🧰 Additional context used
🧬 Code Definitions (12)
src/mineflayer/maps.ts (1)
renderer/viewer/three/threeJsMethods.ts (1) (1)
getThreeJsRendererMethods(4-15)
src/mineflayer/items.ts (3)
renderer/viewer/lib/basePlayerState.ts (1) (1)
ItemSpecificContextProperties(9-9)src/mineflayer/playerState.ts (1) (1)
playerState(201-201)src/appViewer.ts (1) (1)
appViewer(190-190)
src/customChannels.ts (1)
renderer/viewer/three/threeJsMethods.ts (1) (1)
getThreeJsRendererMethods(4-15)
src/watchOptions.ts (3)
src/globalState.ts (1) (1)
miscUiState(109-129)src/optionsStorage.ts (2) (2)
watchValue(213-235)options(186-191)renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(21-249)
renderer/viewer/three/graphicsBackend.ts (4)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-601)src/appViewer.ts (5) (5)
GraphicsBackendLoader(50-50)GraphicsInitOptions(37-42)RendererReactiveState(16-24)DisplayWorldOptions(44-48)GraphicsBackend(53-67)renderer/viewer/three/panorama.ts (1) (1)
PanoramaRenderer(24-206)src/core/progressReporter.ts (1) (1)
ProgressReporter(5-18)
src/index.ts (3)
src/appViewer.ts (2) (2)
loadBackend(87-115)appViewer(190-190)src/globalState.ts (1) (1)
miscUiState(109-129)src/core/progressReporter.ts (1) (1)
createConsoleLogProgressReporter(184-197)
renderer/viewer/lib/holdingBlock.ts (3)
renderer/viewer/lib/basePlayerState.ts (1) (1)
IPlayerState(16-40)renderer/viewer/lib/worldrendererCommon.ts (1) (1)
WorldRendererConfig(51-51)renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-601)
renderer/viewer/lib/entities.ts (4)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-601)renderer/viewer/lib/holdingBlock.ts (1) (1)
getBlockMeshFromModel(908-914)renderer/viewer/lib/entity/armorModels.ts (1) (1)
armorTextures(19-35)renderer/viewer/lib/entity/EntityMesh.ts (1) (1)
getMesh(225-356)
src/appViewer.ts (4)
renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(21-249)renderer/viewer/lib/worldrendererCommon.ts (2) (2)
WorldRendererConfig(51-51)defaultWorldRendererConfig(32-49)src/mineflayer/playerState.ts (1) (1)
playerState(201-201)src/watchOptions.ts (1) (1)
watchOptionsAfterWorldViewInit(110-118)
src/rendererUtils.ts (1)
src/appViewer.ts (1) (1)
appViewer(190-190)
renderer/viewer/lib/worldrendererCommon.ts (5)
src/resourcesManager.ts (1) (1)
ResourcesManager(54-182)src/appViewer.ts (1) (1)
DisplayWorldOptions(44-48)renderer/viewer/lib/mesher/shared.ts (1) (1)
getBlockAssetsCacheKey(55-57)renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(21-249)renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-601)
renderer/viewer/lib/worldrendererThree.ts (4)
renderer/viewer/lib/entities.ts (1) (1)
Entities(209-1037)src/appViewer.ts (3) (3)
GraphicsInitOptions(37-42)DisplayWorldOptions(44-48)RendererReactiveState(16-24)renderer/viewer/lib/basePlayerState.ts (1) (1)
ItemSpecificContextProperties(9-9)renderer/viewer/lib/hand.ts (1) (1)
getMyHand(5-35)
🪛 ESLint
renderer/viewer/lib/worldrendererThree.ts
[error] 363-363: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (82)
renderer/viewer/lib/basePlayerState.ts (3)
25-25: Good addition ofgetPositionin the interface
DeclaringgetPositionensures that all player state implementations must expose positional data, improving clarity across the codebase.
35-38: New reactive properties for environment states
AddinginWater,backgroundColor,ambientLight, anddirectionalLightextends the state’s coverage of environmental details. Make sure relevant logic is updated to use these properties effectively.
44-48: Default property values
Defaulting properties tofalseor0is reasonable. However, verify that these “empty” defaults don’t lead to unexpected behaviors if the actual state should differ at runtime.renderer/viewer/lib/holdingBlock.ts (8)
3-3: ImportingWorldBlockProvider
Adding the named import clarifies usage of type-safe block model transformations within this module.
13-13: ImportingWorldRendererThree
This import indicates that the class now integrates directly with Three.js-based world rendering, aligning with the new architecture.
118-119: New class fields forplayerStateandconfig
Shifting these fields to class-level properties and assigning them fromworldRenderer.displayOptionsconsolidates dependency management.
121-121: Refactored constructor signature
Accepting aWorldRendererThreeinstance and deriving config/state from it simplifies the parameter list and maintains consistency with your centralized design.
123-123: Assigningthis.playerState
Please confirm thatdisplayOptions.playerStateis always defined before usage, or consider adding a fallback to avoid null references.
128-128: Settingthis.configfromdisplayOptions.inWorldRenderingConfig
Ensure thatinWorldRenderingConfigis fully initialized to avoid issues if it’s missing fields or unexpectedly null.
334-334: Adoptingthis.worldRenderer.entities.getItemMesh
Migrating from a direct viewer reference to theworldRendererinstance should improve encapsulation. Good step toward a more modular design.
908-908: ExtendedgetBlockMeshFromModelsignature
PassingblockProvideras an explicit argument clarifies dependencies and allows for more robust block model transformations.renderer/viewer/baseGraphicsBackend.ts (1)
1-11: Well-structured reactive state initializerThis function provides a clean way to initialize the default reactive state for the rendering system. The approach of using a function rather than a constant ensures each caller receives a fresh object instance, preventing shared reference issues.
The state structure logically organizes properties for world loading status, renderer identification, and UI behavior. This aligns well with the PR objective of improving state management in the rendering pipeline.
src/mineflayer/maps.ts (1)
3-3: LGTM: New import for renderer methodsThe import aligns with the PR's goal of separating the application code from direct renderer access.
renderer/viewer/three/threeJsMethods.ts (1)
1-3: LGTM: Proper imports for type definitionsThe import statements correctly bring in the necessary type definitions for the function.
src/customChannels.ts (2)
2-2: LGTM: New import for renderer methodsThe import aligns with the PR's goal of separating the application code from direct renderer access.
60-89:✅ Verification successful
Unused JEI channel registration function
The
registeredJeiChannel()function is defined but never called in the visible code. If this is intentional for future use, consider adding a TODO comment explaining when it will be used, or add the necessary call to activate it if it should be active.registerBlockModelsChannel() + // TODO: Enable JEI channel when feature is ready + // registeredJeiChannel()
🏁 Script executed:
#!/bin/bash # Check if registeredJeiChannel is called elsewhere in the codebase grep -r "registeredJeiChannel()" --include="*.ts" --include="*.js" .Length of output: 70
JEI Channel Registration Function Remains Inactive
After verifying with grep, there are no calls to
registeredJeiChannel()in the codebase. If this function is being reserved for future use, please add a TODO comment indicating under which conditions it should be activated. Otherwise, consider removing it to avoid confusion. For example:registerBlockModelsChannel() +// TODO: Activate JEI channel registration when the JEI feature is implemented +// registeredJeiChannel()renderer/viewer/three/appShared.ts (4)
7-18: Good type definition structureThe return type is well-defined with clear alternatives (either UV coordinates with texture information or a resolved model). This provides good type safety and clear expectations for callers.
19-25: Proper resource validationThe function properly validates that resources are loaded before attempting to access them, and includes a clear error message when the item name can't be found.
57-69: Good error handling with fallbackThe function properly handles errors by catching exceptions, reporting them, and returning default values. This ensures the UI won't crash even if there are issues with textures or rendering information.
31-37:❓ Verification inconclusive
Verify renderSlot function behavior
The
renderSlotfunction is called withfalse, trueparameters, but the meaning of these boolean flags isn't immediately clear. Consider using named parameters or constants to improve readability.
🏁 Script executed:
#!/bin/bash # Description: Check how renderSlot is defined and used elsewhere rg -A 2 "renderSlot(\(|export const renderSlot)" -t ts # Check for other places where renderSlot is used with boolean params rg "renderSlot\(.*,.*(true|false)" -t tsLength of output: 578
Clarify
renderSlotParameter Usage
The call inrenderer/viewer/three/appShared.tsstill passes literal booleans (false, true) whose intent isn’t immediately clear. Notably, another usage insrc/inventoryWindows.tsuses a variable (e.g.debugIsQuickbar), which suggests there’s meaningful control logic behind these parameters. To improve readability and reduce ambiguity, it would be beneficial to refactorrenderSlotcalls by replacing positional booleans with named constants or an options object. Please verify that this change aligns with the expected behavior across all usage sites.src/rendererUtils.ts (2)
77-77: FOV update method changed to use appViewerThe change from directly updating camera FOV to using
appViewer.inWorldRenderingConfig.fovaligns with the PR objective of creating a stable abstraction layer between the app and renderer.
93-93: Camera update method changed to use appViewer backendThe camera update now uses
appViewer.backend?.updateCamerawhich properly encapsulates the renderer implementation details. The optional chaining (?.) handles the case when the backend is not initialized, which is good defensive programming.src/resourcesManager.ts (2)
19-22: Good use of TypedEmitter for type-safe eventsUsing TypedEmitter provides proper type checking for event names and handlers, which helps prevent runtime errors from typos in event names or incorrect handler signatures.
23-45: Well-structured LoadedResources classThe LoadedResources class provides a clear organization of related resources, making it easier to understand what resources are available and how they relate to each other.
src/watchOptions.ts (3)
20-26: Good extraction of common logic to a reusable function.Extracting
updateTouchto a standalone function that can be reused in both initial setup and event listener is a good practice for code organization and reuse.
54-67: Good implementation of FPS limit based on window focus state.The code properly handles FPS limiting based on window focus and user preferences. This is a good power-saving feature that will improve battery life on laptops and mobile devices.
110-117: Updated function signature to includeworldViewparameter.The function now accepts a
worldViewparameter of typeWorldDataEmitter, which aligns with the architectural changes in the PR. This makes the dependencies explicit rather than relying on global state.src/index.ts (4)
118-132: Good modularization of backend loading logic.Extracting the backend loading logic into a separate function improves code organization and reusability. The conditional loading based on build mode is also well-handled.
162-170: Good handling of delayed loading when focus is needed.This code properly handles the case where loading should be delayed until the window has focus, which is important for performance reasons and browser behavior.
692-693: Streamlined renderer startup with appViewer.The code now uses
appViewer.startWorldandappViewer.worldView.listenToBotfor initialization, which is cleaner and more modular than the previous approach. This aligns well with the PR's goal of making the app and renderer more independent.
717-736: Updated chunk loading wait logic.The code now uses the reactive state system to track chunk loading, which is a better pattern than the previous approach. The code properly subscribes to the reactive state and updates the progress reporter accordingly.
renderer/viewer/lib/entities.ts (5)
213-215: Simplified object initialization.The code has been simplified to use a direct object literal for
entitiesOptionsinitialization with the fontFamily property, which is cleaner than the previous approach.
250-250: Updated constructor to use WorldRendererThree instead of Viewer.This change aligns with the PR's goal of refactoring the viewer subsystem. The
Entitiesclass now depends onWorldRendererThreewhich provides a more focused interface specific to rendering.
288-292: Improved rendering configuration access.The code now correctly accesses the rendering configuration through
worldRenderer.worldRendererConfig.renderEntities, which is a more modular approach compared to directly accessing the viewer.
510-510: Fixed getItemMesh method to use worldRenderer.The method now properly uses
this.worldRenderer.getItemRenderDatainstead of a direct reference togetItemUv, which aligns with the new architecture.
1055-1055: Updated addArmorModel function signature.The function now accepts a
worldRendererparameter instead of relying on a global reference, which improves modularity and testability.src/vr.ts (12)
8-8: Import reorganization looks good.
Bringing inWorldRendererThreealigns with the new rendering architecture.
12-14: Initialize VR only if supported.
Good check againstnavigator.xrandvrSupport. Ensures graceful fallback.
23-23: Visibility toggle is correct.
Hides the VR button while the modal is active or VR is disabled.
26-26: Reactive subscription tovrSupport.
This is a clean pattern withsubscribeKey. No issues observed.
29-29: InlineenableVris consistent with the new structure.
Function scope is appropriate for local usage.
31-31: Preventing the escape menu is logical in VR mode.
35-38: Disable VR flow updated properly.
Transitioning fromviewertoworldRendererfor camera override and XR states is consistent.
Consider confirming any references touserto ensure that entity is always defined.
107-108: Attaching the camera touser.
Makes sense to group camera with the VR user anchor. Confirm that removing from the scene (indisableVr) is always called to avoid orphaned objects.
195-196: Camera update logic changed.
Previously used an offset-based approach, now usesbot.entity.yaw&bot.entity.pitch. Confirm if ignoringyawOffsetis intentional.
207-207: Rendering withworldRenderer.render().
Straightforward transition fromviewer.render().
210-210: AssigningcameraObjectOverridetouserupon session start.
Ensures VR session camera is parented properly to the user anchor.
217-217: ClearingcameraObjectOverrideon session end.
Helps revert to standard camera usage post VR session.renderer/viewer/lib/worldrendererCommon.ts (18)
6-6: ReintroducedAtlasParserimport.
This presumably aids in parsing block/item textures. Ensure you’re using the updated parser frommc-assets.
11-11: Utility import for spiral matrix generation.
This is well-aligned for chunk spiral loading logic.
12-12: Proxy import fromvaltio.
Leverages reactivity in your config or state. Looks appropriate.
15-16: New resource-related imports.
IntroducingResourcesManagerandDisplayWorldOptionsfosters better structure. No issue found.
18-18: Integrated references tosharedmesher utilities.
No immediate concerns. Keep an eye on chunk boundary logic for edge cases.
34-34:mesherWorkers: 4.
Increasing the default worker count might improve meshing performance. Validate concurrency overhead.
62-62:worldSizeParamsintroduced.
StoringminYandworldHeightis beneficial for dimension-based logic. Looks fine.
114-114:highestBlocksdeclared as a Map.
This can improve chunk-based computations for terrain rendering.
141-142:itemsTextureandsoundSystemfields.
Integrating audio/texture references directly is a clear step toward more cohesive resource management.
146-159: AdoptingworldRendererConfig& constructor injection of resources.
This approach decouples the world rendering from the environment, letting theResourcesManagerhandle relevant assets.
161-170:init()method defers resource loading.
Prevent re-initializing ifactiveis set. This pattern is good for controlling multiple inits.
175-188:updateCustomBlocklogic.
Storing custom blocks inprotocolCustomBlocksand refreshing if chunk is loaded is well-structured. Watch for race conditions if chunk load is slow.
190-199:getBlockInfo()with custom block model lookups.
Clear caching approach. If the block is in theprotocolCustomBlocksmap, leverage custom rendering.
366-371: Asynchronous resource loading for mesher data.
Implementing version logic atsetVersionto re-download or reuse existing resources is sensible.
420-420: NewworldMinYRenderproperty.
Clamps rendering for chunks below configured thresholds. A neat performance optimization.
447-447: Looping fromthis.worldMinYRender.
Ensures partial chunk sections below the clip line won’t be re-meshed. Good to see.
514-514: Addedconnectmethod reference at constructor’s end.
No immediate issue, but confirm that callingconnectso early doesn’t conflict with worker init states.
749-766: Reviseddestroy()method.
- Properly terminating workers.
- Stopping sound system.
- Clearing event listeners.
Great for preventing memory leaks.
src/inventoryWindows.ts (5)
9-9: Updated import location forversionToNumber.
Reflects the new restructure. No issues found.
23-23: Additional imports frommineflayer/items.
Combines multiple item utilities in one place. This is good for clarity.
47-48: Check foritemsAtlasParseron resource manager.
Prevents referencing uninitialized textures. Good fallback condition.
57-57: Listening toassetsTexturesUpdated.
Moves from the old viewer-based approach to the newresourcesManager. The subscription scheme is consistent with the rest of the code.
141-142: New block/items references for textures.
UsingappViewer.resourcesManager.currentResources!.<atlasParser>.latestImageis more direct than referencing old viewer fields.
Check for potential null if resource loading fails.src/appViewer.ts (5)
16-24: Well-defined reactive state interface.The
RendererReactiveStateinterface provides a clear structure for tracking renderer state. This will make it easier to observe and react to changes in the rendering pipeline.
50-67: GraphicsBackend interface provides a clean abstraction layer.The interface creates a clear separation between the application and rendering code, which aligns well with the PR objective of creating stability between layers. The "no sync methods" comment is helpful for enforcing async operations where needed.
87-115: Enhance error handling in loadBackend.In the
loadBackendmethod, the error handling indisplayCriticalErroronly logs the error and sets a status message. For critical rendering errors, consider implementing more robust fallback strategies.
190-192: GlobalappViewerinstance.Exporting a single global instance is convenient but can lead to hidden state issues if anything else tries to instantiate
AppViewer. Ensure thread-safety if concurrency arises in the future.
209-221: Good use of reactive programming.The
modalStackUpdateChecksfunction and subscription to modal stack changes is a clean implementation of reactive programming. This helps maintain a consistent UI state when modals are shown or hidden.renderer/viewer/lib/worldrendererThree.ts (6)
37-43: Good initialization of Three.js scene components.Initializing scene, lights, and entity management as class properties provides a cleaner architecture. This aligns with the PR objective of improving the rendering system's structure.
53-80: Constructor changes align with new architecture.The constructor now properly integrates with the new AppViewer architecture, taking resourcesManager from initOptions and setting up the necessary components. The error handling for missing resourcesManager is a good safeguard.
94-104: Well-structured scene initialization in resetScene.The
resetScenemethod cleanly configures the scene's properties, including setting up the camera with appropriate parameters. This helps with the PR goal of making resource cleanup and initialization more organized.
106-125: Excellent use of reactive subscriptions.The
watchReactivePlayerStatemethod uses subscriptions to react to player state changes, which is a clean pattern for maintaining rendering consistency with game state changes.
136-144: Clean texture loading implementation.The
updateAssetsDatamethod efficiently loads textures and applies appropriate settings for the Minecraft-style rendering. Good use of async/await pattern.
584-596: Well-implemented camera roll control.The
setCameraRollmethod correctly uses quaternions to handle camera rotation, which is the proper way to handle 3D rotations in Three.js. This will provide smooth camera transitions when needed.
| getPosition (): Vec3 { | ||
| return new Vec3(0, 0, 0) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Returning a constant (0, 0, 0)
The new getPosition method always returns (0, 0, 0). If you plan to track actual player movement, consider switching from this placeholder to dynamic logic reflecting the real player coordinates.
| import { ItemSpecificContextProperties } from 'renderer/viewer/lib/basePlayerState' | ||
| import { getItemDefinition } from 'mc-assets/dist/itemDefinitions' | ||
| import { MessageFormatPart } from '../chatUtils' | ||
| import { playerState } from './playerState' |
There was a problem hiding this comment.
Missing import for appViewer
The function references appViewer (line 103) but doesn't import it. This may cause runtime errors if the module is used in isolation.
Add the missing import:
+import { appViewer } from '../appViewer'
import { ItemSpecificContextProperties } from 'renderer/viewer/lib/basePlayerState'
import { getItemDefinition } from 'mc-assets/dist/itemDefinitions'
import { MessageFormatPart } from '../chatUtils'
import { playerState } from './playerState'📝 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.
| import { ItemSpecificContextProperties } from 'renderer/viewer/lib/basePlayerState' | |
| import { getItemDefinition } from 'mc-assets/dist/itemDefinitions' | |
| import { MessageFormatPart } from '../chatUtils' | |
| import { playerState } from './playerState' | |
| import { appViewer } from '../appViewer' | |
| import { ItemSpecificContextProperties } from 'renderer/viewer/lib/basePlayerState' | |
| import { getItemDefinition } from 'mc-assets/dist/itemDefinitions' | |
| import { MessageFormatPart } from '../chatUtils' | |
| import { playerState } from './playerState' |
| export const getItemModelName = (item: GeneralInputItem, specificProps: ItemSpecificContextProperties) => { | ||
| let itemModelName = item.name | ||
| const { customModel } = getItemMetadata(item) | ||
| if (customModel) { | ||
| itemModelName = customModel | ||
| } | ||
|
|
||
| const itemSelector = playerState.getItemSelector({ | ||
| ...specificProps | ||
| }) | ||
| const modelFromDef = getItemDefinition(appViewer.resourcesManager.itemsDefinitionsStore, { | ||
| name: itemModelName, | ||
| version: appViewer.resourcesManager.currentResources!.version, | ||
| properties: itemSelector | ||
| })?.model | ||
| const model = (modelFromDef === 'minecraft:special' ? undefined : modelFromDef) ?? itemModelName | ||
| return model | ||
| } |
There was a problem hiding this comment.
Verify null safety in appViewer access
The code assumes appViewer.resourcesManager.currentResources is not null (line 105), but only uses the null assertion operator (!) without checking. Consider adding explicit null checks to prevent runtime errors.
- const modelFromDef = getItemDefinition(appViewer.resourcesManager.itemsDefinitionsStore, {
- name: itemModelName,
- version: appViewer.resourcesManager.currentResources!.version,
- properties: itemSelector
- })?.model
+ // Check if resources are loaded
+ if (!appViewer.resourcesManager.currentResources) {
+ console.warn(`Resources not loaded, using default model name: ${itemModelName}`);
+ return itemModelName;
+ }
+
+ const modelFromDef = getItemDefinition(appViewer.resourcesManager.itemsDefinitionsStore, {
+ name: itemModelName,
+ version: appViewer.resourcesManager.currentResources.version,
+ properties: itemSelector
+ })?.model📝 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 const getItemModelName = (item: GeneralInputItem, specificProps: ItemSpecificContextProperties) => { | |
| let itemModelName = item.name | |
| const { customModel } = getItemMetadata(item) | |
| if (customModel) { | |
| itemModelName = customModel | |
| } | |
| const itemSelector = playerState.getItemSelector({ | |
| ...specificProps | |
| }) | |
| const modelFromDef = getItemDefinition(appViewer.resourcesManager.itemsDefinitionsStore, { | |
| name: itemModelName, | |
| version: appViewer.resourcesManager.currentResources!.version, | |
| properties: itemSelector | |
| })?.model | |
| const model = (modelFromDef === 'minecraft:special' ? undefined : modelFromDef) ?? itemModelName | |
| return model | |
| } | |
| export const getItemModelName = (item: GeneralInputItem, specificProps: ItemSpecificContextProperties) => { | |
| let itemModelName = item.name | |
| const { customModel } = getItemMetadata(item) | |
| if (customModel) { | |
| itemModelName = customModel | |
| } | |
| const itemSelector = playerState.getItemSelector({ | |
| ...specificProps | |
| }) | |
| // Check if resources are loaded | |
| if (!appViewer.resourcesManager.currentResources) { | |
| console.warn(`Resources not loaded, using default model name: ${itemModelName}`); | |
| return itemModelName; | |
| } | |
| const modelFromDef = getItemDefinition(appViewer.resourcesManager.itemsDefinitionsStore, { | |
| name: itemModelName, | |
| version: appViewer.resourcesManager.currentResources.version, | |
| properties: itemSelector | |
| })?.model | |
| const model = (modelFromDef === 'minecraft:special' ? undefined : modelFromDef) ?? itemModelName | |
| return model | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
renderer/viewer/lib/entities.ts (1)
494-537:⚠️ Potential issueUpdated item mesh and texture handling.
The method now uses
this.worldRendererto retrieve item rendering data and textures. However, there's a potential issue with settingsizetoundefinedand then referencing it in calculations.The
sizevariable is declared asundefinedbut then used in the subsequent calculations forsizeYandsizeX. This could lead to unexpected behavior. Verify if this is intentional or ifsizeshould have a default value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
renderer/playground/scenes/entities.ts(1 hunks)renderer/viewer/lib/entities.ts(21 hunks)
✅ Files skipped from review due to trivial changes (1)
- renderer/playground/scenes/entities.ts
🧰 Additional context used
🧬 Code Definitions (1)
renderer/viewer/lib/entities.ts (4)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-592)renderer/viewer/lib/holdingBlock.ts (1) (1)
getBlockMeshFromModel(908-914)renderer/viewer/lib/entity/armorModels.ts (1) (1)
armorTextures(19-35)renderer/viewer/lib/entity/EntityMesh.ts (1) (1)
getMesh(225-356)
🔇 Additional comments (14)
renderer/viewer/lib/entities.ts (14)
17-17: Added import for entity attackability check.The new import for
isEntityAttackableis used to filter entities in thegetEntityMeshfunction, which is an important change to the entity rendering logic.
28-28: Key architectural change: Using WorldRendererThree instead of Viewer.This import supports the major architectural shift from using the
Viewerclass to theWorldRendererThreeclass, aligning with the PR objective to separate application code from rendering code.
171-187: Refined entity rendering with attackability check.The function now includes type annotations and an early return for non-attackable entities, which changes which entities will be rendered. This aligns with the refactoring goal but could affect the visibility of certain entities.
Make sure this change was intentional and doesn't inadvertently hide entities that should be visible to the player.
211-220: Updated Entities class configuration.The class now includes properly typed entities options and a rendering status flag. This improves type safety and clarity in the codebase.
237-240: Constructor now takes WorldRendererThree instead of Viewer.This is a fundamental change that propagates throughout the class, replacing all references to
viewerwithworldRendererto support the new rendering architecture.
242-248: Updated scene reference in clear method.The method now properly references
this.worldRenderer.sceneinstead of the previous viewer's scene, maintaining consistency with the architectural change.
262-271: Updated scene management in setRendering method.The method now consistently uses
this.worldRenderer.scenefor adding and removing entities, aligning with the new architecture.
273-307: Refactored render method to use worldRenderer properties.The method now uses
worldRenderer.worldRendererConfig.renderEntitiesfor configuration andworldRenderer.viewerPositionandworldRenderer.finishedChunksfor positioning and visibility checks.
580-607: Added entity lifecycle management.The
updatemethod now includes proper tracking of entity addition/removal through new hooks, enhancing the control over entity lifecycle events.
673-707: Updated entity mesh creation and scene management.The code now properly passes the worldRenderer to getEntityMesh and manages entity addition to the scene.
712-719: Updated armor model handling.Armor models now receive the worldRenderer as a parameter rather than accessing it indirectly, improving the clarity of dependencies.
883-889: Enhanced player equipment management.Added specific handling for player equipment like capes and elytra, improving the visual fidelity of player models.
891-915: Added dedicated entity position update method.The new
updateEntityPositionmethod provides a cleaner separation of concerns by handling position and rotation updates separately from other entity updates.
1083-1156: Updated armor model function to accept worldRenderer parameter.The function now explicitly receives the worldRenderer parameter rather than accessing it indirectly, improving function purity and testability.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
renderer/viewer/lib/entities.ts (2)
213-214: Consider fallback fonts.
You've introduced"mojangles"as the default font. If this font is not loaded or available, the text might degrade. Providing a fallback font could improve reliability.
918-918: Empty hook function.
TheonAddEntityhook is empty. If you don’t plan to override it, consider removing it. Otherwise, add a comment clarifying its intended usage.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
renderer/playground/scenes/entities.ts(1 hunks)renderer/viewer/lib/entities.ts(21 hunks)
✅ Files skipped from review due to trivial changes (1)
- renderer/playground/scenes/entities.ts
🧰 Additional context used
🧬 Code Definitions (1)
renderer/viewer/lib/entities.ts (4)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-592)renderer/viewer/lib/holdingBlock.ts (1) (1)
getBlockMeshFromModel(908-914)renderer/viewer/lib/entity/armorModels.ts (1) (1)
armorTextures(19-35)renderer/viewer/lib/entity/EntityMesh.ts (1) (1)
getMesh(225-356)
🔇 Additional comments (18)
renderer/viewer/lib/entities.ts (18)
17-17: Verify new dependency usage.
The import frommineflayer-mouse/dist/attackableEntitylooks correct. Ensure thatmineflayer-mouseis properly listed in your dependencies and that the version you rely on provides theisEntityAttackablefunction.
28-28: Refactor alignment approved.
ImportingWorldRendererThreecleanly aligns with the new renderer architecture.
171-171: Ensure all call sites have been updated.
Modifying thegetEntityMeshsignature to accept aWorldRendererThree | undefinedparameter is a breaking change. Please verify that every external invocation has been updated accordingly.
187-187: ConfirmloadedDatascope.
CallingisEntityAttackable(loadedData, entity)presupposes thatloadedDatais in scope. If this is a global variable, confirm it's always available. Otherwise, consider passingloadedDataexplicitly as a parameter.
211-211: Refactored Entities class.
Exporting theEntitiesclass in alignment withWorldRendererThreeis a good approach to keep concerns well-separated.
219-219: Simple state tracking.
DefiningcurrentlyRendering = trueclarifies the class’s initial rendering state. The approach appears straightforward.
237-237: Constructor usage looks good.
Switching topublic worldRenderer: WorldRendererThreeis consistent with the new design.
244-244: Scene removal updated.
Replacingthis.viewer.scene.remove(mesh)withthis.worldRenderer.scene.remove(mesh)aligns with the updated architecture.
263-263: Rendering toggle logic.
Usingthis.currentlyRenderingto control adding/removing entities fromthis.worldRenderer.sceneis a clear and maintainable approach.Also applies to: 266-266, 268-268
275-277: Dynamically toggling rendering based on config.
Comparing the config value with the localcurrentlyRenderingand callingthis.setRenderingis a clean solution for toggling entity rendering.
280-280: Check for null reference.
const botPos = this.worldRenderer.viewerPositionassumesviewerPositionis defined. If there’s a chance it could beundefined, consider graceful handling or a safety check.
305-305: Visibility control works.
Conditionally settingentity.visiblebased on distance andthis.worldRenderer.finishedChunks[chunkKey]is consistent with the new world renderer logic.
581-581: Helpful flag for new entities.
StoringjustAddedas!this.entities[entity.id]helps differentiate newly added entities and can improve animation transitions.
894-896: Smooth positioning updates.
TheupdateEntityPositionmethod neatly handles position and rotation tweening. This is a clean approach, though watch for scenarios where multiple concurrent tweens might lead to unexpected behavior.Also applies to: 898-901, 902-904
914-914: Deferred skin rendering.
Callingthis.maybeRenderPlayerSkin(entity)after tweening ensures that skin logic only runs once positioning data is updated. This should help maintain a responsive rendering flow.
1083-1083: Function signature updated.
addArmorModelnow acceptsworldRendereras the first parameter, aligning with the rest of the refactor.
1117-1117: Defensive check on resources.
AccessingworldRenderer.resourcesManager.currentResources!with!can cause runtime errors if the resources aren’t ready. Consider a null check or fallback if this can be undefined.
1138-1138: Consistent usage ofgetMesh.
PassingworldRenderer,texturePath, andarmorModel[slotType]shows consistent usage of the new API.
|
/deploy |
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
renderer/viewer/lib/entities.ts (1)
1131-1139:⚠️ Potential issueUnhandled promise in loadTexture call.
The loadTexture function likely returns a Promise that isn't being properly handled.
Apply this fix to properly handle the Promise:
- loadTexture(texturePath, texture => { - texture.magFilter = THREE.NearestFilter - texture.minFilter = THREE.NearestFilter - texture.flipY = false - texture.wrapS = THREE.MirroredRepeatWrapping - texture.wrapT = THREE.MirroredRepeatWrapping - material.map = texture - }) + void loadTexture(texturePath, texture => { + texture.magFilter = THREE.NearestFilter + texture.minFilter = THREE.NearestFilter + texture.flipY = false + texture.wrapS = THREE.MirroredRepeatWrapping + texture.wrapT = THREE.MirroredRepeatWrapping + material.map = texture + })The
voidoperator explicitly marks that we're ignoring the promise result.🧰 Tools
🪛 GitHub Check: build-and-deploy
[warning] 1131-1131:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with thevoidoperator🪛 GitHub Actions: CI
[warning] 1131-1131: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the
voidoperator @typescript-eslint/no-floating-promises
🧹 Nitpick comments (1)
renderer/viewer/lib/entities.ts (1)
1085-1085: Function has too many parameters.The
addArmorModelfunction has 6 parameters, exceeding the maximum allowed (4) according to the style guidelines.Consider refactoring this to use an options object pattern:
-function addArmorModel (worldRenderer: WorldRendererThree, entityMesh: THREE.Object3D, slotType: string, item: Item, layer = 1, overlay = false) { +function addArmorModel (worldRenderer: WorldRendererThree, entityMesh: THREE.Object3D, slotType: string, item: Item, options: { layer?: number, overlay?: boolean } = {}) { + const { layer = 1, overlay = false } = options; // Function body... }Then update the call sites accordingly:
-addArmorModel(this.worldRenderer, e, 'feet', entity.equipment[2]) +addArmorModel(this.worldRenderer, e, 'feet', entity.equipment[2]) -addArmorModel(this.worldRenderer, e, 'legs', entity.equipment[3], 2) +addArmorModel(this.worldRenderer, e, 'legs', entity.equipment[3], { layer: 2 }) -addArmorModel(this.worldRenderer, e, 'chest', entity.equipment[4]) +addArmorModel(this.worldRenderer, e, 'chest', entity.equipment[4]) -addArmorModel(this.worldRenderer, e, 'head', entity.equipment[5]) +addArmorModel(this.worldRenderer, e, 'head', entity.equipment[5])🧰 Tools
🪛 GitHub Check: build-and-deploy
[warning] 1085-1085:
Function 'addArmorModel' has too many parameters (6). Maximum allowed is 4🪛 GitHub Actions: CI
[warning] 1085-1085: Function 'addArmorModel' has too many parameters (6). Maximum allowed is 4 max-params
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/ci.yml(1 hunks).github/workflows/next-deploy.yml(1 hunks).github/workflows/preview.yml(1 hunks).github/workflows/publish.yml(1 hunks)README.MD(1 hunks)renderer/viewer/lib/entities.ts(21 hunks)
✅ Files skipped from review due to trivial changes (4)
- .github/workflows/ci.yml
- .github/workflows/preview.yml
- .github/workflows/publish.yml
- .github/workflows/next-deploy.yml
🧰 Additional context used
🧬 Code Definitions (1)
renderer/viewer/lib/entities.ts (4)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-592)renderer/viewer/lib/holdingBlock.ts (1) (1)
getBlockMeshFromModel(908-914)renderer/viewer/lib/entity/armorModels.ts (1) (1)
armorTextures(19-35)renderer/viewer/lib/entity/EntityMesh.ts (1) (1)
getMesh(225-356)
🪛 GitHub Check: build-and-deploy
renderer/viewer/lib/entities.ts
[warning] 1085-1085:
Function 'addArmorModel' has too many parameters (6). Maximum allowed is 4
🪛 GitHub Actions: CI
renderer/viewer/lib/entities.ts
[warning] 1085-1085: Function 'addArmorModel' has too many parameters (6). Maximum allowed is 4 max-params
[warning] 1131-1131: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator @typescript-eslint/no-floating-promises
🔇 Additional comments (31)
renderer/viewer/lib/entities.ts (30)
17-18: Appropriate import addition to support entity attack check.The added import from
mineflayer-mouseforisEntityAttackableis necessary to properly determine which entities can be attacked, improving the entity filtering logic.
29-29: Updated import to reflect new architecture.Replacing the Viewer import with WorldRendererThree aligns with the architectural refactoring described in the PR objectives, where the rendering pipeline is being modularized.
172-172: Function signature modernized to use WorldRendererThree.Updated parameter type from the old Viewer to the new WorldRendererThree, correctly implementing the architectural changes.
188-188: Improved entity filtering with isEntityAttackable.Now using a proper utility function instead of implicit assumptions about what entities should be rendered as mesh boxes.
212-221: Entity class constructor updated to use WorldRendererThree.The Entities class now properly uses WorldRendererThree for rendering, aligning with the architectural changes. The
entitiesOptionsalso has been moved to class properties for better organization.
238-241: Updated constructor parameter from viewer to worldRenderer.This change is crucial for the architecture refactoring, ensuring the Entities class works with the new rendering system.
244-248: Updated scene reference in clear method.Now properly uses
this.worldRenderer.sceneinstead ofthis.viewer.scenefor removing entities from the scene.
263-272: Scene management updated in setRendering method.Properly references the worldRenderer's scene instead of the viewer's scene for adding/removing entities.
274-279: Improved rendering control with worldRenderer config.Now rendering decisions are based on the worldRenderer's configuration, providing a more centralized approach to controlling entity rendering.
280-282: Updated bot position reference.Now correctly using
this.worldRenderer.viewerPositioninstead of accessing the bot's position directly.
305-307: Updated chunk reference in visibility check.Now using
this.worldRenderer.finishedChunksinstead ofthis.viewer.world.finishedChunks.
345-346: Updated configuration reference for ear rendering.Now using
this.worldRenderer.worldRendererConfig.renderEarsinstead of a direct viewer reference.
496-497: Updated item rendering data retrieval.Now using
this.worldRenderer.getItemRenderData()for consistent access to item data.
500-501: Updated block mesh generation.Now uses the worldRenderer's resourcesManager to access the worldBlockProvider, improving resource management.
521-525: Fixed texture retrieval in item mesh generation.Now properly uses the worldRenderer's material and itemsTexture for texture mapping.
582-583: Added entity tracking for initialization.Added a
justAddedflag to determine if an entity was just added, which helps with initialization logic.
601-606: Added entity lifecycle hooks.Now calls
onRemoveEntitywhen an entity is removed, providing a hook for cleanup operations.
675-676: Updated entity mesh creation.Now passes the worldRenderer to getEntityMesh instead of viewer.
694-695: Updated scene reference for entity addition.Now uses
this.worldRenderer.scene.add(group)instead of viewer's scene.
701-702: Added entity lifecycle hooks.Added
onAddEntityhook when a new entity is added, providing a cleaner way to handle entity initialization.
707-708: Updated rendering state management.Now uses the entity class's currentlyRendering state variable for consistent state management.
716-720: Updated armor model creation.Now passes the worldRenderer to addArmorModel functions for proper resource management.
884-891: Added player equipment handling.New code to manage player equipment state, especially for cape and elytra visualization.
892-893: Extracted position update logic.Moved entity position updates to a dedicated method for better separation of concerns.
895-916: Implemented dedicated entity position update method.The new
updateEntityPositionmethod provides a cleaner way to handle entity position and rotation updates, with appropriate animation handling.
918-920: Added entity lifecycle hook method.The
onAddEntitymethod provides a hook for custom logic when an entity is added.
938-941: Added entity tracking and cleanup.The
onRemoveEntitymethod properly cleans up entity references by removing them from the loadedSkinEntityIds set.
1119-1120: Updated texture path resolution.Now correctly uses the worldRenderer's resourcesManager to access armor textures.
1140-1141: Updated mesh creation with worldRenderer.Now correctly passes the worldRenderer to getMesh for proper integration with the new architecture.
1157-1158: Updated recursive armor model creation.Now passes the worldRenderer to the recursive call to addArmorModel.
README.MD (1)
26-27: Updated feature list to reflect architectural changes.The README has been updated to remove Google Drive support and add information about the new modular rendering architecture, which aligns with the PR objectives.
The new line "Support for custom rendering 3D engines. Modular architecture." accurately reflects the architectural changes implemented in this PR, where the rendering system has been refactored to be more modular and support different rendering backends.
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
renderer/viewer/lib/entities.ts (1)
1131-1139:⚠️ Potential issuePromise not awaited or handled.
The promise returned by
loadTextureis not awaited or handled, which could lead to unhandled promise rejections. Consider properly awaiting the promise or marking it as intentionally ignored.- loadTexture(texturePath, texture => { - texture.magFilter = THREE.NearestFilter - texture.minFilter = THREE.NearestFilter - texture.flipY = false - texture.wrapS = THREE.MirroredRepeatWrapping - texture.wrapT = THREE.MirroredRepeatWrapping - material.map = texture - }) + // Option 1: Mark as intentionally ignored + void loadTexture(texturePath, texture => { + texture.magFilter = THREE.NearestFilter + texture.minFilter = THREE.NearestFilter + texture.flipY = false + texture.wrapS = THREE.MirroredRepeatWrapping + texture.wrapT = THREE.MirroredRepeatWrapping + material.map = texture + }) + + // Option 2: Handle errors + loadTexture(texturePath, texture => { + texture.magFilter = THREE.NearestFilter + texture.minFilter = THREE.NearestFilter + texture.flipY = false + texture.wrapS = THREE.MirroredRepeatWrapping + texture.wrapT = THREE.MirroredRepeatWrapping + material.map = texture + }).catch(err => console.error('Failed to load texture:', err))🧰 Tools
🪛 GitHub Check: build-and-deploy
[warning] 1131-1131:
Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with thevoidoperator🪛 GitHub Actions: CI
[warning] 1131-1131: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the
voidoperator @typescript-eslint/no-floating-promises
🧹 Nitpick comments (2)
renderer/viewer/lib/entities.ts (2)
267-267: Simplify condition using Array.includes.The condition can be simplified using the includes method.
- if (!this.worldRenderer.scene.children.includes(ent)) this.worldRenderer.scene.add(ent) + if (!this.worldRenderer.scene.children.includes(ent)) { + this.worldRenderer.scene.add(ent) + }
1085-1085: Function has too many parameters (6).The
addArmorModelfunction exceeds the maximum allowed parameters (4) as indicated by the linter. Consider refactoring to use an options object for better maintainability.- function addArmorModel (worldRenderer: WorldRendererThree, entityMesh: THREE.Object3D, slotType: string, item: Item, layer = 1, overlay = false) { + type ArmorModelOptions = { + worldRenderer: WorldRendererThree; + entityMesh: THREE.Object3D; + slotType: string; + item: Item; + layer?: number; + overlay?: boolean; + } + + function addArmorModel (options: ArmorModelOptions) { + const { worldRenderer, entityMesh, slotType, item, layer = 1, overlay = false } = options;Then update all calling sites like:
- addArmorModel(this.worldRenderer, e, 'feet', entity.equipment[2]) + addArmorModel({ + worldRenderer: this.worldRenderer, + entityMesh: e, + slotType: 'feet', + item: entity.equipment[2] + })🧰 Tools
🪛 GitHub Check: build-and-deploy
[warning] 1085-1085:
Function 'addArmorModel' has too many parameters (6). Maximum allowed is 4🪛 GitHub Actions: CI
[warning] 1085-1085: Function 'addArmorModel' has too many parameters (6). Maximum allowed is 4 max-params
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
renderer/viewer/lib/entities.ts(21 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
renderer/viewer/lib/entities.ts (3)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-592)renderer/viewer/lib/entity/armorModels.ts (1) (1)
armorTextures(19-35)renderer/viewer/lib/entity/EntityMesh.ts (1) (1)
getMesh(225-356)
🪛 GitHub Check: build-and-deploy
renderer/viewer/lib/entities.ts
[warning] 1085-1085:
Function 'addArmorModel' has too many parameters (6). Maximum allowed is 4
🪛 GitHub Actions: CI
renderer/viewer/lib/entities.ts
[warning] 1085-1085: Function 'addArmorModel' has too many parameters (6). Maximum allowed is 4 max-params
[warning] 1131-1131: Promises must be awaited, end with a call to .catch, end with a call to .then with a rejection handler or be explicitly marked as ignored with the void operator @typescript-eslint/no-floating-promises
🔇 Additional comments (3)
renderer/viewer/lib/entities.ts (3)
926-929: Fixed player skin loading distance check.The implementation now correctly uses the camera position and world renderer's view distance setting, resolving the previous undefined
botreference issue.
895-916: Good refactor: Extracting position update logic to a separate method.Extracting the entity position update logic into a dedicated method improves code organization and makes the update method more maintainable.
918-941: Good addition: Entity lifecycle methods.Adding dedicated methods for entity addition and removal events enables better extensibility and provides natural extension points for custom behavior.
This reverts commit 5eedb3c.
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-142082mr0-zaro.vercel.app |
Things I noticed while testing it locally:
|
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/appViewer.ts (1)
184-198:⚠️ Potential issueInconsistent backend interface reference.
this.backend?.worldStateis accessed, butworldStateis not declared in theGraphicsBackendinterface. This can cause undefined behavior or runtime errors if the backend lacks this property.- if (this.backend?.worldState.allChunksLoaded) return + // either define worldState in the GraphicsBackend interface + // or remove the referencing code in waitingForChunks
🧹 Nitpick comments (9)
src/react/AppStatusProvider.tsx (4)
68-68: Check safe casting forusingState.Explicitly casting
(isOpen ? currentState : lastState.current) as typeof currentStatemay lead to subtle runtime issues iflastState.currentdiverges in shape fromcurrentState. Verify the shape consistency to prevent hidden type mismatches at runtime.
96-96: Keep the TODO tasks discoverable.You have a “// todo do only if reconnect is possible” comment. If still relevant, consider creating a follow-up ticket or adding a Trello/Jira reference so this won't get lost.
121-142: Refine thebackActionlogic for maintainability.The nested conditions (
if (!wasDisconnected)...else{}) and multiple state resets can obscure readability. Consider extracting the “wasDisconnected” branch into a dedicated helper for clarity and consistent handling of modal closures.Example refactor:
if (maybeRecoverable && (!lockConnect || !wasDisconnected)) { backAction = () => { - if (!wasDisconnected) { - hideModal(undefined, undefined, { force: true }) - return - } - resetAppStatusState() - ... + if (!wasDisconnected) return closeModalImmediately() + handleDisconnectedCase() } }
157-157: Clarify empty error states.Passing
backAction={backAction}as a prop whenbackActionmight remain undefined is harmless, but it can confuse code reading. Consider conditionally passing the prop only ifbackActionexists, improving clarity.src/resourcesManager.ts (1)
107-142: Optimize the creation of multiple atlases.Atlas creation for blocks and items can be heavy. If possible, parallelize them or cache partial results. Also, confirm that repeated calls to
makeNewAtlashandle concurrency gracefully (i.e., no race conditions).src/index.ts (1)
116-130: Ensure backend loading reliability.
loadBackendis defined and then invoked immediately unlessSINGLE_FILE_BUILD_MODEis active. Confirm that the backend is needed early or consider lazy loading once user interaction demands the renderer. This can improve startup performance.renderer/viewer/lib/worldrendererCommon.ts (1)
163-173: Prevent double initialization ininit().There's a guard (
if (this.active) throw new Error...) but concurrency or repeated calls could bypass it. Use a more robust pattern, e.g., an atomic boolean or a dedicated “initialized” state to avoid partial setup.src/appViewer.ts (1)
201-202: Avoid global state on window if possible.
AttachingappViewertowindowcan create hidden dependencies. Consider a more controlled injection or usage pattern for better maintainability and testability.renderer/viewer/lib/worldrendererThree.ts (1)
592-595: Partial disposal in destroy method.
While callingremoveAllStats()and thensuper.destroy()is good, verify that any custom THREE.js resources (e.g., textures, geometries) are fully freed in the chain of destructors.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
renderer/viewer/lib/mesher/shared.ts(1 hunks)renderer/viewer/lib/ui/newStats.ts(1 hunks)renderer/viewer/lib/worldrendererCommon.ts(15 hunks)renderer/viewer/lib/worldrendererThree.ts(12 hunks)renderer/viewer/three/graphicsBackend.ts(1 hunks)renderer/viewer/three/panorama.ts(1 hunks)src/appViewer.ts(1 hunks)src/connect.ts(2 hunks)src/index.ts(12 hunks)src/optionsGuiScheme.tsx(2 hunks)src/react/AppStatus.tsx(1 hunks)src/react/AppStatusProvider.tsx(5 hunks)src/resourcePack.ts(5 hunks)src/resourcesManager.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- renderer/viewer/lib/mesher/shared.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- renderer/viewer/three/panorama.ts
- src/resourcePack.ts
- renderer/viewer/three/graphicsBackend.ts
🧰 Additional context used
🧬 Code Definitions (3)
src/optionsGuiScheme.tsx (1)
src/resourcePack.ts (1) (1)
resourcepackReload(573-575)
src/appViewer.ts (6)
renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(37-266)renderer/viewer/lib/worldrendererCommon.ts (2) (2)
WorldRendererConfig(53-53)defaultWorldRendererConfig(31-51)renderer/viewer/lib/basePlayerState.ts (1) (1)
IPlayerState(17-42)src/mineflayer/playerState.ts (1) (1)
playerState(206-206)renderer/viewer/baseGraphicsBackend.ts (1) (1)
getDefaultRendererState(3-14)src/watchOptions.ts (1) (1)
watchOptionsAfterWorldViewInit(112-120)
renderer/viewer/lib/worldrendererThree.ts (7)
renderer/viewer/lib/holdingBlock.ts (1) (1)
HoldingBlock(94-514)renderer/viewer/lib/entities.ts (1) (1)
Entities(212-1067)src/appViewer.ts (2) (2)
GraphicsInitOptions(39-44)DisplayWorldOptions(46-52)renderer/viewer/lib/basePlayerState.ts (1) (1)
ItemSpecificContextProperties(10-10)renderer/viewer/three/appShared.ts (1) (1)
getItemUv(7-70)renderer/viewer/lib/hand.ts (1) (1)
getMyHand(5-35)renderer/viewer/lib/ui/newStats.ts (1) (1)
removeAllStats(43-48)
🪛 GitHub Check: build-and-deploy
src/index.ts
[failure] 711-711:
'appViewer.worldView' is possibly 'undefined'.
[failure] 701-701:
'appViewer.worldView' is possibly 'undefined'.
🪛 GitHub Actions: CI
src/index.ts
[error] 701-701: 'appViewer.worldView' is possibly 'undefined'.
🪛 ESLint
renderer/viewer/lib/worldrendererThree.ts
[error] 393-393: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (21)
src/react/AppStatus.tsx (1)
86-86: Simplified back button rendering logic.The conditional check for
lockConnecthas been removed, making the "Back" button appear wheneverbackActionis defined. This simplifies the UI logic and ensures the back functionality is consistently available when needed.src/optionsGuiScheme.tsx (1)
184-184: Good addition: Resource pack reload after disabling.Adding a call to
resourcepackReload()after settingenabledResourcepackto null ensures that texture changes take effect immediately, providing better user feedback when disabling a resource pack.renderer/viewer/lib/ui/newStats.ts (2)
43-48: Good addition: Function to remove all statistics.The
removeAllStatsfunction provides a clean way to remove all statistics at once, which is valuable for proper cleanup during disconnection or renderer recreation. This aligns well with the PR's goal of improving resource management during disconnection.
50-54: Good addition: Function to remove individual statistics.The
removeStatfunction provides a safe way to remove individual statistics by checking if the stat exists before attempting removal. This helps prevent potential errors and ensures clean resource management.src/connect.ts (1)
46-46: Simplified function signature aligns with new architecture.Removing the
importBlockstatesModelsparameter fromloadMinecraftDatasimplifies the function and aligns with the PR's goal of moving resource management to a dedicated ResourceManager. This change contributes to the separation of application code from rendering code.src/react/AppStatusProvider.tsx (2)
163-163: Validate guard condition usage.Rendering the button when
{wasDisconnected && lastAutoCapturedPackets}is good. Ensure thatlastAutoCapturedPacketshas a nonzero length or valid data. Otherwise, “Inspect Last 0 Packets” might appear for no-op actions.
9-9:✅ Verification successful
Ensure usage reference or remove redundant import.
The import of
appQueryParamsis now used, which is fine. However, verify there are no other references left behind from prior changes that rely on the old approach. If unused anywhere else, consider removing extraneous imports to maintain clarity.To confirm usage, run:
🏁 Script executed:
#!/bin/bash rg -A 3 "appQueryParams" --type tsLength of output: 17028
Usage Verified:
appQueryParamsImport is Active and Up-to-Date
The grep search confirms thatappQueryParamsis actively used insrc/react/AppStatusProvider.tsx(e.g., for evaluatinglockConnect) as well as in multiple other modules. There are no indications of remnants from a previous approach. No removal or further changes are necessary.src/resourcesManager.ts (3)
69-71: Add error handling forloadMcData.Your
loadMcData(version)lacks a try-catch block. This repeats a prior recommendation. Failure to load data can crash the application. Gracefully handle exceptions and surface errors to the user.async loadMcData (version: string) { + try { await loadMinecraftData(version) + } catch (error) { + console.error('Failed to load McData:', error) + throw new Error('Could not load Minecraft data') + } }
82-104: Reexamine abort handling inupdateAssetsData.You're creating a local
abortControllerbut never referencingthis.abortController. Ifdestroy()is called, the local controller remains unaware and the operation can continue in the background. Coordinate them for consistent cancellation.- const abortController = new AbortController() + const { signal } = this.abortController - if (abortController.signal.aborted) return + if (signal.aborted) return
187-191: Use stable reference toabortController.When you recreate
this.abortControllerindestroy(), ensure that references in asynchronous code do not become invalid. If any pending tasks keep the old reference, it may lead to memory leaks or missed abort signals.src/index.ts (1)
165-174: Check the focus logic for potential user confusion.Waiting for document focus (line 167) can stall the connection if the user remains on a different tab. Confirm that this is the intended experience, or provide a fallback if the user never focuses the tab.
renderer/viewer/lib/worldrendererCommon.ts (1)
701-701: Disable or address overshadowed “distance check.”When
getDistancereturns a distance larger thanviewDistance, the chunk is skipped from meshing. Confirm your intention to drop the chunk or queue it for future meshing if the viewer moves closer. This might cause partial or delayed rendering in edge conditions.src/appViewer.ts (4)
1-16: Imports look clean.
No issues found with the new or updated imports here.
91-119: Ensure defensive method checks for queued actions.
When dispatchingthis.backend[method](...args), consider verifying thatmethodexists on the backend to avoid runtime errors if the method name changes or is removed.Do you want a script to detect all references of
this.backend[anyMethod]calls to ensure these methods are defined inGraphicsBackend?
148-168: Check for potential undefined version in resourcesManager.
this.resourcesManager.currentConfig!may be undefined if resources have not been initialized before callingstartWorld. Consider a null-check to avoid unexpected runtime errors.
204-219: Logic for starting panorama on modal stack changes looks valid.
The sequence for resetting the backend and starting the panorama upon UI changes is coherent. No critical issues identified.Also applies to: 220-234
renderer/viewer/lib/worldrendererThree.ts (5)
6-24: New imports are consistent.
These imports align with the new rendering architecture and show a clear separation of functionality.
35-45: New class properties introduced.
The introduction ofcameraRoll,scene,ambientLight, and others is well-organized. They are logically grouped at the top and straightforward to follow.
190-213: Persisting usage of@ts-expect-errorin demo methods.
This matches a previously flagged concern regarding suppressed TypeScript errors.Consider removing
@ts-expect-erroror clarifying the reasoning behind its necessity, and ensurecursorBlockRelis defined or properly imported.
359-365: Camera override logic appears sound.
Forwarding to a custom camera object or falling back tothis.camerais a flexible strategy. The code is easy to follow and no issues are evident here.
578-590: Camera roll feature.
Allowing a dedicated roll transform is a useful extension for more advanced camera effects, with clear quaternion logic. Looks good.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
renderer/viewer/lib/worldrendererCommon.ts (3)
583-592:⚠️ Potential issueDuplicate event handlers for 'renderDistance'.
There are two nearly identical event handlers for 'renderDistance' events (lines 583-587 and 588-593), which could lead to the same logic being executed twice and potential inconsistencies.
Remove the duplicate event handler:
worldEmitter.on('renderDistance', (d) => { this.viewDistance = d this.chunksLength = d === 0 ? 1 : generateSpiralMatrix(d).length + this.allChunksFinished = Object.keys(this.finishedChunks).length === this.chunksLength }) - worldEmitter.on('renderDistance', (d) => { - this.viewDistance = d - this.chunksLength = d === 0 ? 1 : generateSpiralMatrix(d).length - this.allChunksFinished = Object.keys(this.finishedChunks).length === this.chunksLength - })
505-522: 🛠️ Refactor suggestionPotential race condition in setBlockStateId.
The method creates an async function that's executed but not awaited, which could lead to race conditions with multiple blocks being updated simultaneously.
Consider using a queue system to ensure block updates are processed in order:
setBlockStateId (pos: Vec3, stateId: number | undefined) { - const set = async () => { - const sectionX = Math.floor(pos.x / 16) * 16 - const sectionZ = Math.floor(pos.z / 16) * 16 - if (this.queuedChunks.has(`${sectionX},${sectionZ}`)) { - await new Promise<void>(resolve => { - this.queuedFunctions.push(() => { - resolve() - }) - }) - } - if (!this.loadedChunks[`${sectionX},${sectionZ}`]) { - // console.debug('[should be unreachable] setBlockStateId called for unloaded chunk', pos) - } - this.setBlockStateIdInner(pos, stateId) - } - void set() + const sectionX = Math.floor(pos.x / 16) * 16 + const sectionZ = Math.floor(pos.z / 16) * 16 + const chunkKey = `${sectionX},${sectionZ}` + + if (this.queuedChunks.has(chunkKey)) { + // Add to queue for processing after chunk loads + this.queuedFunctions.push(() => { + this.setBlockStateIdInner(pos, stateId) + }) + } else if (this.loadedChunks[chunkKey]) { + // Process immediately for loaded chunks + this.setBlockStateIdInner(pos, stateId) + } }
145-158: 🛠️ Refactor suggestionConstructor directly calls connect() which may lead to initialization issues.
The constructor immediately connects to the world view (line 157) before the object is fully initialized. This approach could lead to race conditions if connect() relies on properties that are set after construction or in the
init()method.Consider moving the
this.connect(this.displayOptions.worldView)call to the end of theinit()method to ensure all necessary initialization is completed first.constructor (public readonly resourcesManager: ResourcesManager, public displayOptions: DisplayWorldOptions, public version: string) { // this.initWorkers(1) // preload script on page load this.snapshotInitialValues() this.worldRendererConfig = displayOptions.inWorldRenderingConfig this.playerState = displayOptions.playerState this.reactiveState = displayOptions.rendererState this.renderUpdateEmitter.on('update', () => { const loadedChunks = Object.keys(this.finishedChunks).length updateStatText('loaded-chunks', `${loadedChunks}/${this.chunksLength} chunks (${this.lastChunkDistance}/${this.viewDistance})`) }) - - this.connect(this.displayOptions.worldView) } init () { if (this.active) throw new Error('WorldRendererCommon is already initialized') void this.setVersion(this.version).then(() => { this.resourcesManager.on('assetsTexturesUpdated', () => { if (!this.active) return void this.updateAssetsData() }) if (this.resourcesManager.currentResources) { void this.updateAssetsData() } + this.connect(this.displayOptions.worldView) }) }renderer/viewer/three/panorama.ts (1)
133-140: 💡 Verification agent🧩 Analysis chain
Undefined variable reference in worldBlocksPanorama.
The code references
loadedData.blocksArraywhich isn't defined within this file's scope. This will cause a runtime error when executing the panorama rendering.You need to either import this variable or define it within the method:
- const fullBlocks = loadedData.blocksArray.filter(block => { + // Import or define loadedData, for example: + import { loadedData } from '../lib/minecraftData' // Adjust the import path as needed + // Or define it locally: + const loadedData = require('minecraft-data')(version) + const fullBlocks = loadedData.blocksArray.filter(block => {You can also verify if this is defined by checking:
🏁 Script executed:
#!/bin/bash # Search for loadedData definition in the codebase rg "loadedData" -A 2 -B 2Length of output: 13855
Critical Issue: Undefined
loadedDatain panorama.tsAfter running a search through the codebase, it’s confirmed that
loadedDatais referenced in multiple files but isn’t defined or imported inrenderer/viewer/three/panorama.ts. This is likely to cause a runtime error when attempting to accessloadedData.blocksArray.Suggested Fix:
- Option 1: Import the variable from a shared module (e.g., if it’s exported in
lib/minecraftData):- const fullBlocks = loadedData.blocksArray.filter(block => { + import { loadedData } from '../lib/minecraftData'; // Adjust the import path as needed + const fullBlocks = loadedData.blocksArray.filter(block => {- Option 2: Define it locally, if appropriate (for example, by requiring the module):
- const fullBlocks = loadedData.blocksArray.filter(block => { + const loadedData = require('minecraft-data')(version); + const fullBlocks = loadedData.blocksArray.filter(block => {Review the project’s architecture to decide whether
loadedDatashould be imported as a global/shared variable or defined locally in this module.src/resourcesManager.ts (2)
71-78: 🛠️ Refactor suggestionMissing error handling in loadMcData and loadSourceData methods.
These methods should include error handling to gracefully manage failures when loading Minecraft data.
Add try-catch blocks to handle potential errors:
async loadMcData (version: string) { + try { await loadMinecraftData(version) + } catch (error) { + console.error('Failed to load Minecraft data:', error) + throw new Error(`Failed to load Minecraft data for version ${version}: ${error.message}`) + } } async loadSourceData (version: string) { + try { await this.loadMcData(version) this.sourceBlockStatesModels ??= await importLargeData('blockStatesModels') + } catch (error) { + console.error('Failed to load source data:', error) + throw new Error(`Failed to load source data for version ${version}: ${error.message}`) + } }
84-89: 🛠️ Refactor suggestionImprove abort handling in updateAssetsData.
The method creates a new local
abortControllerbut doesn't use the class-levelthis.abortController. This could lead to resources not being properly cleaned up whendestroy()is called.Use the class-level abort controller:
async updateAssetsData (request: UpdateAssetsRequest, unstableSkipEvent = false) { if (!this.currentConfig) throw new Error('No config loaded') - const abortController = new AbortController() + const { signal } = this.abortController await this.loadSourceData(this.currentConfig.version) - if (abortController.signal.aborted) return + if (signal.aborted) returnAlso update subsequent abort checks on lines 136 and 170:
- if (abortController.signal.aborted) return + if (signal.aborted) returnsrc/appViewer.ts (1)
200-212:⚠️ Potential issueInconsistency in waitingForChunks method.
The method references
this.backend?.worldState.allChunksLoaded, but there's noworldStateproperty defined in theGraphicsBackendinterface. This will cause runtime errors.Update to use the correct property path:
async waitingForChunks () { - if (this.backend?.worldState.allChunksLoaded) return + if (this.rendererState.world.allChunksLoaded) return return new Promise((resolve) => { const interval = setInterval(() => { - if (this.backend?.worldState.allChunksLoaded) { + if (this.rendererState.world.allChunksLoaded) { clearInterval(interval) resolve(true) } }, 100) + // Add timeout to prevent memory leaks + const timeout = setTimeout(() => { + clearInterval(interval) + resolve(false) + }, 30000) // 30 second timeout }) }
🧹 Nitpick comments (4)
renderer/viewer/three/panorama.ts (1)
34-51: Missing window resize handling in PanoramaRenderer.The camera is initialized with the current window dimensions, but there's no event listener for window resize events. This could lead to rendering issues when the window size changes.
Add window resize event handling:
constructor (private readonly documentRenderer: DocumentRenderer, private readonly options: GraphicsInitOptions, private readonly doWorldBlocksPanorama = false) { this.scene = new THREE.Scene() this.scene.background = new THREE.Color(this.options.config.sceneBackground) // Add ambient light this.ambientLight = new THREE.AmbientLight(0xcc_cc_cc) this.scene.add(this.ambientLight) // Add directional light this.directionalLight = new THREE.DirectionalLight(0xff_ff_ff, 0.5) this.directionalLight.position.set(1, 1, 0.5).normalize() this.directionalLight.castShadow = true this.scene.add(this.directionalLight) this.camera = new THREE.PerspectiveCamera(85, window.innerWidth / window.innerHeight, 0.05, 1000) this.camera.position.set(0, 0, 0) this.camera.rotation.set(0, 0, 0) + // Add resize listener + window.addEventListener('resize', this.handleResize, { signal: this.abortController.signal }) } + private handleResize = () => { + this.camera.aspect = window.innerWidth / window.innerHeight + this.camera.updateProjectionMatrix() + }renderer/viewer/lib/worldrendererThree.ts (3)
35-44: Alpha test caution for the new mesh material.
DefiningalphaTestonMeshLambertMaterialat 0.1 may lead to sharp or unexpected cutouts where textures have slight transparency. Confirm this is intentional; otherwise consider using alpha blending or adjusting the alpha cutoff for smoother transitions.
131-162: Add optional error handling for asynchronous texture loading.
Currently, a loading failure inloadAsyncmight throw an unhandled exception. Consider wrapping with try/catch or.catch(...)to gracefully handle fetch errors.
393-393: Redundant ESLint disable directive.
Line 393 disables@typescript-eslint/non-nullable-type-assertion-stylewithout needing it anywhere. Consider removing this comment to keep ESLint configs clean.🧰 Tools
🪛 ESLint
[error] 393-393: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
renderer/viewer/lib/worldrendererCommon.ts(15 hunks)renderer/viewer/lib/worldrendererThree.ts(12 hunks)renderer/viewer/three/panorama.ts(1 hunks)src/appViewer.ts(1 hunks)src/customChannels.ts(3 hunks)src/inventoryWindows.ts(6 hunks)src/resourcesManager.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/customChannels.ts
- src/inventoryWindows.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/appViewer.ts (4)
renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(37-266)renderer/viewer/lib/worldrendererCommon.ts (2) (2)
WorldRendererConfig(50-50)defaultWorldRendererConfig(28-48)src/mineflayer/playerState.ts (1) (1)
playerState(206-206)src/watchOptions.ts (1) (1)
watchOptionsAfterWorldViewInit(112-120)
renderer/viewer/lib/worldrendererCommon.ts (4)
src/appViewer.ts (2) (2)
RendererReactiveState(17-26)DisplayWorldOptions(48-54)src/resourcesManager.ts (1) (1)
ResourcesManager(59-201)renderer/viewer/lib/ui/newStats.ts (2) (2)
updateStatText(38-41)removeStat(50-54)renderer/viewer/lib/mesher/shared.ts (1) (1)
getBlockAssetsCacheKey(56-58)
🪛 ESLint
renderer/viewer/lib/worldrendererThree.ts
[error] 393-393: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
🔇 Additional comments (24)
renderer/viewer/lib/worldrendererCommon.ts (1)
766-786: Good implementation of resource cleanup in destroy method.The destroy method properly terminates workers and cleans up the sound system, which helps prevent memory leaks and ensures resources are released when no longer needed.
renderer/viewer/three/panorama.ts (1)
202-206: Good implementation of resource cleanup in dispose method.The dispose method properly cleans up resources by clearing the scene, destroying the world renderer, and aborting ongoing processes with the abort controller.
src/resourcesManager.ts (1)
196-201: Good implementation of resource cleanup in destroy method.The destroy method properly aborts ongoing operations, resets the resources, and reinitializes the abort controller for future use.
src/appViewer.ts (3)
215-217: GlobalappViewerinstance and assignment towindow.Creating a global instance and assigning it to the window object can lead to maintainability issues and potential security concerns.
Consider implementing a more controlled approach to accessing the AppViewer instance:
export const appViewer = new AppViewer() - window.appViewer = appViewer + // Instead of adding to window, export a function to access it + export function getAppViewer() { + return appViewer + }
99-127: Good implementation of backend loading and state management.The
loadBackendmethod properly handles disconnecting an existing backend, initializing a new one, and executing any queued state. This approach provides clean transitions between rendering backends.
129-152: Good implementation of world initialization.The
startWorldmethod properly sets up the world view, establishes event listeners, and creates a clear display configuration for the renderer. The state management is well-organized and provides a clear flow for starting the world rendering.renderer/viewer/lib/worldrendererThree.ts (18)
6-23: Imports look properly organized.
No immediate issues, as each imported symbol appears to be used. Good addition of typed references.
54-70: Consider handling potential Promise rejections from VR initialization.
You’re discarding any async result frominitVR(this)withvoid. IfinitVRreturns or throws, the error may go unhandled. If necessary, catch or await the promise to handle errors gracefully.
72-87: Rotation overrides appear correct for entity updates.
The conditional logic for separate position updates vs. full entity updates is straightforward and seems coherent.
89-99: Scene reset logic looks good.
Matrix auto updates are disabled for performance, and default lighting is properly applied. This is a neat approach to ensure a consistent initial state.
101-120: Neat reactive state updates for environment.
The straightforward subscription mechanism for background color, fog, and ambient/directional light is clear and maintainable.
164-169: Texture readiness checks look solid.
Marking the holding blocks as ready and updating items is logically consistent after texture loading finishes.
186-188: Seamless item render data retrieval.
The delegated call togetItemUvis direct and maintainable, showing clear separation of concerns.
190-201: Unnecessary@ts-expect-errorand undefined function usage (repeat note).
You’re still referencingcursorBlockRelwithout importing or defining it, and suppressing TypeScript errors with//@ts-expect-error. The same comment from previous reviews applies. Consider removing or properly typing this code if it’s only for debugging.
202-213: Same@ts-expect-errorusage indemoItem.
As withdemoModel, the undefinedcursorBlockRelpersists here. Revisit the approach or document its usage.
303-303: Chunk border visibility toggle.
Your logic correctly turns offboxHelperwhenshowChunkBordersis false. This helps declutter the scene for standard gameplay.
359-365: First-person camera offset looks good.
You properly incorporate the eye height for positioning. This is straightforward and likely beneficial for a realistic viewpoint.
368-379: Subtle tween-based camera movement.
Applying a small tween duration (50ms) is a nice touch, though it can introduce tiny camera lag. Ensure it doesn’t conflict with frantic user input or free-fly logic.
381-405: Flexible rendering pipeline.
The code updates FOV/aspect on demand, renders entities and hand blocks, and invokes customonRendercallbacks. This is well-structured for modular rendering.🧰 Tools
🪛 ESLint
[error] 393-393: '@typescript-eslint/non-nullable-type-assertion-style' rule is disabled but never reported.
(eslint-comments/no-unused-disable)
478-483: Light update rename is consistent.
RenamingupdateLighttolightUpdateclarifies the method’s purpose and keeps chunk updates unified with the existing approach.
492-501: Dynamic chunk border toggling.
Assigning the boolean flag toshowChunkBordersthen toggling visibility on existinghelperobjects is concise and efficient.
549-561: Column removal with chunk texture cleanup.
You are disposing of chunk textures, removing the mesh, and disposing the object. This systematically avoids leaks.
578-591: Rolling the camera via quaternions.
The approach is valid, though mixing euler-based pitch/yaw with quaternion-based roll might be tricky. Confirm the final orientation matches gameplay expectations.
592-595: Ensure full resource cleanup upon destroy.
You remove stats and callsuper.destroy(), but confirm that all THREE.js objects (e.g., scene, textures, geometry) are also properly disposed in the parent or here to avoid memory leaks.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/appViewer.ts (1)
199-211:⚠️ Potential issueInconsistency:
this.backend?.worldStatevs. the declared interface.
YourGraphicsBackendinterface doesn't defineworldState, yetwaitingForChunks()usesthis.backend?.worldState.allChunksLoaded. This likely causes runtime undefined property references.Replace references to
worldStatewith actual reactive state usage, for example:-if (this.backend?.worldState.allChunksLoaded) return +if (this.rendererState.world.allChunksLoaded) returnEnsure this fix aligns with your intended architecture for tracking chunk loading.
🧹 Nitpick comments (6)
src/appViewer.ts (3)
74-94: Ensure concurrency safety when sharing global state.
The class maintains shared state across the application (e.g.,resourcesManager,worldView, andrendererState) and also exposes itself globally viawindow.appViewer. While this is acceptable for rapid prototyping, it can introduce complexity and possible side effects if multiple consumers try to modify or re-initialize the state concurrently.Consider introducing a dedicated state management solution (e.g., a store pattern or dependency injection) to isolate global state changes more reliably if concurrency or SSR eventually become relevant.
186-197: Possible confusion indisconnectBackendmethod.
The method resetsthis.backendand setsthis.currentDisplay = null, but a separate call todestroyAll()also resets resources. Double-check that these calls happen in the correct order to avoid partial cleanup or referencing a null backend in other code paths.
216-218: Prevent hidden-state complexities from global assignment.
export const appViewer = new AppViewer()is additionally assigned towindow.appViewer. Storing global singletons can sometimes hamper testability and lead to unforeseen coupling.If practical, consider passing
appViewerreferences explicitly through your codebase to reduce reliance on globally shared objects.src/index.ts (3)
165-174: Wait for focus before continuing.
Delaying the connection until the window is focused (sessionStorage.delayLoadUntilFocus) helps ensure the user is ready for immediate interaction. Just be mindful of edge cases, such as the user never focusing the tab, leading to indefinite stalling.Consider providing a fallback or timeout to connect if the user never focuses the tab.
695-713: Sequential calls after bot spawn and startWorld.
The code elegantly handles resource pack installation, callsappViewer.startWorld(bot.world, renderDistance), then initializes camera tracking. This staged approach is clear. Just be cautious if you need specialized error handling in each stage (e.g., resource pack failing after partial world load).Consider adding minor guard checks to ensure
appVieweris still connected and that the user hasn't cancelled or closed the connection mid-stage.
718-724: Repeated camera updates on bot movement.
appViewer.backend?.updateCamera(...)andappViewer.worldView?.updatePosition(...)insidebot.on('move')might degrade performance if triggered very frequently.Throttle or debounce frequent position updates to reduce overhead, especially for large or busy servers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/appViewer.ts(1 hunks)src/index.ts(11 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
src/appViewer.ts (4)
renderer/viewer/lib/worldDataEmitter.ts (1) (1)
WorldDataEmitter(37-266)renderer/viewer/lib/worldrendererCommon.ts (2) (2)
WorldRendererConfig(50-50)defaultWorldRendererConfig(28-48)src/mineflayer/playerState.ts (1) (1)
playerState(206-206)src/watchOptions.ts (1) (1)
watchOptionsAfterWorldViewInit(112-120)
🔇 Additional comments (5)
src/appViewer.ts (1)
129-152:❓ Verification inconclusive
Verify
worldViewlifecycle and usage.
this.worldViewis set, and then immediately used in the same method for rendering. IfworldView.init()orstartWorldfail halfway, or if the server is disconnected prematurely, you could end up storing a half-initializedworldViewinstance.Please confirm that rapid connect/disconnect scenarios or exceptions during
worldView.init()won't lead to orphaned state. You can check for issues by searching the codebase for quick repeated calls tostartWorld:
🏁 Script executed:
#!/bin/bash rg -A 3 "appViewer\.startWorld"Length of output: 475
Verify Handling of Partial Initialization in
worldViewThe review comment highlights that
this.worldViewis created and immediately used for rendering. In scenarios wherethis.worldView.init(startPosition)orbackend.startWorld(displayWorldOptions)fails—whether due to exceptions or rapid connect/disconnect events—the instance could be left in a half-initialized, orphaned state.
- Confirm that failure paths during
worldView.init()are properly managed (e.g., with cleanup or retry logic).- Verify that any exceptions during
backend.startWorld()or subsequent asynchronous operations do not leaveworldViewin an inconsistent state.- Review related call sites (e.g., the single call in
src/index.ts) to ensure they don’t unintentionally invokestartWorldmultiple times leading to similar issues.src/index.ts (4)
116-130: Conditionally load backend only after filesystem is ready.
This logic ensures that the backend loads aftermiscUiState.fsReadyis true ifSINGLE_FILE_BUILD_MODEis set. This approach avoids partial initialization of the rendering system when the filesystem is unprepared. The design is sound, but watch for possible race conditions iffsReadyis toggled off or on multiple times.
221-221: Retaining the viewer instance for debugging.
SkippingappViewerreset is helpful for debugging, but can leave partial state if subsequent reconnect attempts are made in the same session. Ensure other parts of the code handle that leftover viewer state correctly.
654-670: Efficient chunk-loading checks withappViewer.rendererState.
The newwaitForChunksToLoadfunction improves chunk-loading feedback by subscribing toappViewer.rendererState.world. This approach is consistent with a reactive pattern. Just ensure you clean up any subscriptions if the user disconnects before chunks have fully loaded.
734-748: Conditional chunk waiting logic.
The logic behindoptions.waitForChunksRenderin singleplayer mode ensures chunk loading is optional for performance reasons. This is a neat feature. Double-check that the fallback (if (!appViewer.backend || appViewer.rendererState.world.allChunksLoaded || !waitForChunks)) properly accounts for edge cases — e.g., “renderDistance=0” scenarios or immediate disconnections.
|
@Phoenix616 thank you so much for testing! I fixed renderer issues you reported, and inmy testing everything is fine. The only problem was sometimes I could see green ellipses on my screen , not sure what the issue, will do last code review and then merge i think |
|
/deploy |
|
Deployed to Vercel Preview: https://prismarine-46gp80mav-zaro.vercel.app |
|
merge after I fix this (cursor mesh)... |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
renderer/viewer/three/world/cursorBlock.ts (1)
79-93:⚠️ Potential issueUndefined reference to
bot, causing a runtime error.
This exact issue was flagged in a previous review.- const mergedShape = bot.mouse.getMergedCursorShape(block) - if (!mergedShape) return - const { position, width, height, depth } = bot.mouse.getDataFromShape(mergedShape) + // Please adjust the reference below as appropriate for your architecture: + const { mergedShape, position, width, height, depth } = this.worldRenderer.getCursorShapeData(block) + if (!mergedShape) return
🧹 Nitpick comments (3)
renderer/viewer/three/world/cursorBlock.ts (3)
19-24: Add explicit typing forprevColorto improve clarity.Currently,
prevColorhas no explicit type. Specifying its type (e.g.,string | null) can enhance readability and reduce potential type-related issues.export class CursorBlock { cursorLineMaterial: LineMaterial interactionLines: null | { blockPos; mesh } = null - prevColor + prevColor: string | null = null blockBreakMesh: THREE.Mesh breakTextures: THREE.Texture[] = [] }
25-54: Consider handling texture loading errors.Using
THREE.TextureLoaderwithout an error callback may result in silent failures if a texture file is missing or invalid. Adding an error handler could help diagnose loading issues more quickly.const loader = new THREE.TextureLoader() +loader.load( + destroyStagesImages[i], + (texture) => { + // onLoad + }, + undefined, + (err) => { + console.error('Failed to load texture', destroyStagesImages[i], err) + } +)
125-131: Dispose geometries and materials to prevent potential memory leaks.When removing wireframes or the entire group from the scene, consider calling
.dispose()on geometries and materials to release GPU resources and avoid leaking memory during continuous cursor block updates.group.add(wireframe) // On removal: +wireframe.geometry.dispose() +(wireframe.material as LineMaterial).dispose()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
renderer/viewer/lib/worldDataEmitter.ts(5 hunks)renderer/viewer/three/world/cursorBlock.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
renderer/viewer/three/world/cursorBlock.ts (1)
renderer/viewer/lib/worldrendererThree.ts (1) (1)
WorldRendererThree(26-596)
🔇 Additional comments (7)
renderer/viewer/lib/worldDataEmitter.ts (7)
8-9: Good addition of typed event handling and reactive state managementAdding TypedEmitter provides type safety for your event system, and introducing valtio's proxy allows for reactive state management. This is a good architectural improvement.
18-31: Well-structured event type definitionsThe explicit typing of all possible events with their payload structures through
WorldDataEmitterEventssignificantly improves type safety and code clarity. This will help catch potential event-related bugs at compile time rather than runtime.
37-37: Effective TypedEmitter integrationThe class now extends TypedEmitter while maintaining EventEmitter compatibility through type casting. This provides better type checking for event emissions without breaking existing code.
79-82: Improved entity event handlingThe updated
emitEntityfunction with a customizable event name parameter and the null name check improves both flexibility and robustness. The check fore.nameprevents emitting events for incompletely initialized entities.
105-105: Proper use of parameterized event nameGood use of the updated
emitEntityfunction's second parameter to specify the 'entityMoved' event.
46-49:❓ Verification inconclusive
Good introduction of reactive state
Using valtio's proxy for cursor-related state is a clean approach to manage reactive state. This allows for centralized state management of cursor position and breaking stage.
Make sure any code that previously accessed cursor state directly has been updated to use this reactive property instead. You may want to run:
🏁 Script executed:
#!/bin/bash # Search for direct cursor state access that might need to be updated rg -i "cursorBlock|cursorBlockBreakingStage" --type ts --type js --glob "!**/worldDataEmitter.ts"Length of output: 2514
Please verify that all direct accesses to cursor state are fully migrated to use the new reactive state.
The introduction of the reactive state using valtio's proxy in
renderer/viewer/lib/worldDataEmitter.tslooks clear and is a solid move toward centralized state management. However, the grep results indicate that several files (for example,src/react/DebugOverlay.tsx,src/mineflayer/plugins/mouse.ts, andsrc/optionsStorage.ts) still contain references tocursorBlock(and potentiallycursorBlockBreakingStage) that may bypass the new reactive property.Please ensure that:
- All components and modules previously accessing the cursor state directly are updated to use the reactive property defined in
worldDataEmitter.ts.- Any legacy or direct accesses in files like
src/react/DebugOverlay.tsxandsrc/mineflayer/plugins/mouse.tsare refactored accordingly.Use the grep command below to double-check for any remaining direct references:
#!/bin/bash rg -i "cursorBlock|cursorBlockBreakingStage" --type ts --type js --glob "!**/worldDataEmitter.ts"Once you’ve confirmed that all references have been updated, feel free to remove any deprecated code paths.
52-53:❓ Verification inconclusive
Verify constructor initialization
The eslint-disable comment suggests there might be an initialization issue. Ensure the parent class constructor is properly called.
Check if the TypedEmitter implementation requires specific initialization parameters that should be passed to
super():
🏁 Script executed:
#!/bin/bash # Check the TypedEmitter implementation rg -A 10 "export default" --type ts node_modules/typed-emitter/Length of output: 133
Verify TypedEmitter Constructor Initialization
The current code in
renderer/viewer/lib/worldDataEmitter.tscallssuper()without any arguments. However, our automated check for the TypedEmitter implementation (using the provided script) did not locate anode_modules/typed-emitter/directory, returning a "No such file or directory" error. This absence could mean that the dependency is either not installed, imported differently, or that TypedEmitter is a type-only helper without a constructor requiring parameters.Please ensure that:
- The TypedEmitter implementation (or its proxy/base, such as Node’s EventEmitter if applicable) doesn’t require additional parameters during initialization.
- The dependency is correctly declared in your
package.jsonand properly installed, or that its implementation is verified via an alternative source if it’s type-only.Manual verification of the TypedEmitter dependency in your project setup is recommended before finalizing this change.
User description
Since the resources for the development are very limited and I want to have a client with a full vanilla experience I have to first bring the stability to key aspects of it. Having performant renderer is a top priority and I also want try avoiding regressions in it. I didn’t really touch the rendering pipeline in the last 2 years (only a huge amount of patches to fix old bugs), because it worked mostly fine, and I didn’t have much time to re-rework everything as even a minor change could break everything (which happened a few times anyway).
So with this PR now I’m splitting rendering engine in which I’m not good and the other app code in which I’m much better by introducing another layer between app (mostly mineflayer related) and the graphics code. Eg it won’t be possible to access code from renderer anymore. For now this new layer is called AppViewer (I would call it GraphicsBackendProvider or similar but want to preserve viewer global access that is easy to remember).
fixes #303
Also refactor old viewer class which had following issues:
Goals I set:
I understand that it both can break some existing things and would make any current developing features would resolve into conflicts because of a lot of code changes. That’s why I think it should be merged asap. This would allow me to finally start perf debugging and make big rendering pipeline changes. If you have any other ideas for improvements it would be better to implement them now as I really want to avoid such refactorings in the future.
Plans for the future:
perf +15%
broken functionality +20%
PR Type
Enhancement, Other
Description
Introduced
AppVieweras a new stable layer to manage rendering backends, isolating the app logic from the rendering pipeline.Refactored rendering components (
WorldRendererThree,DocumentRenderer,PanoramaRenderer) for modularity and integration withAppViewer.Added
ResourcesManagerfor centralized asset and texture management, simplifying resource handling across the application.Enhanced performance debugging tools and FPS tracking with new utilities and integrations.
Simplified and modularized various components, including inventory windows, camera controls, and VR handling, to align with the new architecture.
Temporarily disabled or commented out certain logic (e.g., entity-related logic, day cycle) for future refinement.
Deleted obsolete files and replaced them with updated implementations.
Changes walkthrough 📝
38 files
worldrendererCommon.ts
Refactor and enhance world renderer with resource management.renderer/viewer/lib/worldrendererCommon.ts
ResourcesManagerandWorldDataEmitterintegration.defaultWorldRendererConfig.updateAssetsDatato useResourcesManager.WorldDataEmitter.index.ts
Transition to `appViewer` for rendering management.src/index.ts
Viewerinitialization and replaced it withappViewer.createGraphicsBackend.worldrendererThree.ts
Refactor three.js world renderer for modularity.renderer/viewer/lib/worldrendererThree.ts
appViewerandResourcesManager.panorama.ts
Introduce panorama rendering for background visuals.renderer/viewer/three/panorama.ts
PanoramaRendererclass for rendering background panoramas.DocumentRendererfor rendering.resourcesManager.ts
Introduce `ResourcesManager` for centralized asset management.src/resourcesManager.ts
ResourcesManagerclass for managing assets and textures.documentRenderer.ts
Add `DocumentRenderer` for rendering lifecycle management.renderer/viewer/three/documentRenderer.ts
DocumentRendererclass for managing rendering lifecycle.watchOptions.ts
Update option watchers to integrate with `appViewer`.src/watchOptions.ts
appViewerfor rendering configuration.appViewer.ts
Add `AppViewer` for unified rendering backend management.src/appViewer.ts
AppViewerclass for managing rendering backends.entities.ts
Refactor `Entities` class for modular rendering.renderer/viewer/lib/entities.ts
Entitiesclass to integrate withWorldRendererThree.resourcePack.ts
Refactor resource pack handling with `ResourcesManager`.src/resourcePack.ts
ResourcesManagerfor resource pack handling.graphicsBackend.ts
Add `GraphicsBackend` for three.js rendering management.renderer/viewer/three/graphicsBackend.ts
createGraphicsBackendfor initializing three.js rendering.DocumentRendererandWorldRendererThree.devtools.ts
Enhance devtools withappViewerintegration and performance debugging.src/devtools.ts
appViewer.cameraShake.ts
Refactor camera shake to use `appViewer` backend.src/mineflayer/cameraShake.ts
appViewerbackend.threeJsSound.ts
Add `ThreeJsSound` for positional audio management.renderer/viewer/lib/threeJsSound.ts
ThreeJsSoundclass for managing positional audio.WorldRendererThree.worldDataEmitter.ts
Enhance `WorldDataEmitter` with reactive state.renderer/viewer/lib/worldDataEmitter.ts
connect.ts
Simplify connection logic and integrate `ResourcesManager`.src/connect.ts
loadMinecraftData.ResourcesManager.inventoryWindows.ts
Refactor inventory windows to use `ResourcesManager`.src/inventoryWindows.ts
ResourcesManager.progressReporter.ts
Add null progress reporter and enhance console logging.src/core/progressReporter.ts
createNullProgressReporterfor silent progress tracking.createConsoleLogProgressReporterwith optional grouping.rendererUtils.ts
Refactor FOV handling to use `appViewer`.src/rendererUtils.ts
appViewerbackend.globals.d.ts
Update global type definitions with `appViewer`.src/globals.d.ts
appViewerto global declarations.guiRenderer.ts
Simplify GUI renderer imports and utilities.renderer/viewer/lib/guiRenderer.ts
utils.ts
Add utilities for version parsing and mapping.renderer/viewer/common/utils.ts
vr.ts
Refactor VR camera handling to use `appViewer`.src/vr.ts
appViewerbackend.loadSave.ts
Simplify load/save logic with common utilities.src/loadSave.ts
cameraRotationControls.ts
Refactor camera rotation controls to use `appViewer`.src/cameraRotationControls.ts
appViewerbackend.chatUtils.ts
Simplify chat utilities with common imports.src/chatUtils.ts
playerState.ts
Update `PlayerStateManager` constructor visibility.src/mineflayer/playerState.ts
PlayerStateManagerconstructor public.botSoundSystem.ts
Simplify bot sound system with common imports.src/sounds/botSoundSystem.ts
minecraftData.ts
Simplify Minecraft data shims with common imports.src/shims/minecraftData.ts
soundsMap.ts
Simplify sounds map with common imports.src/sounds/soundsMap.ts
botUtils.ts
Simplify bot utilities with common imports.src/botUtils.ts
ping.ts
Simplify ping plugin with common imports.src/mineflayer/plugins/ping.ts
optimizeJson.ts
Simplify JSON optimization with common imports.src/optimizeJson.ts
DebugOverlay.tsx
Refactor debug overlay to use `appViewer`.src/react/DebugOverlay.tsx
appViewerbackend.HotbarRenderApp.tsx
Refactor Hotbar rendering to use `appViewer` resource manager.src/react/HotbarRenderApp.tsx
viewertoappViewerfor resource management.appViewer.resourcesManager.reactUi.tsx
Add browser and device-specific CSS adjustments.src/reactUi.tsx
dynamically.
MinimapProvider.tsx
Update utility import path in MinimapProvider.src/react/MinimapProvider.tsx
versionToNumberutility to reflect newdirectory structure.
BookProvider.tsx
Update utility import path in BookProvider.src/react/BookProvider.tsx
versionToNumberutility to reflect newdirectory structure.
5 files
controls.ts
Add debug keybinding for delayed load toggle.src/controls.ts
entities.ts
Temporarily disable entity logic and update imports.src/entities.ts
mouse.ts
Simplify mouse plugin initialization.src/mineflayer/plugins/mouse.ts
dayCycle.ts
Temporarily disable day cycle logic.src/dayCycle.ts
IndicatorEffectsProvider.tsx
Temporarily disable chunk rendering wait logic.src/react/IndicatorEffectsProvider.tsx
viewer.waitForChunksToRender.dirtyunchanged.5 files
Summary by CodeRabbit
New Features
CursorBlockclass for managing visual representations of cursor blocks in 3D.DocumentRendererclass for rendering graphics using Three.js.PanoramaRendererclass for rendering 3D panorama scenes.getItemUvfunction for retrieving UV coordinates and rendering information for items.registerChannelfunction for managing custom channels and handling incoming data.Improvements
Removed Features
ViewerandViewerWrapperclasses, centralizing rendering functionality under theWorldRendererThreeclass.topRightStatsfunctionality for performance monitoring.