feat: brand new default skybox with fog, better daycycle and colors #425
Conversation
…Inspred from js-minecraft
|
Caution Review failedThe pull request is closed. WalkthroughAdds biome-awareness to world data emission and wires new biome events through renderer layers; expands renderer configuration and replaces legacy dayCycle with a new Skybox system that responds to time, temperature, view distance, and water state. README updates external references. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Bot
participant WorldDataEmitter as WorldDataEmitter
participant RendererCommon as WorldRendererCommon
participant RendererThree as WorldRendererThree
participant Skybox as SkyboxRenderer
Bot->>WorldDataEmitter: updatePosition(pos)
WorldDataEmitter->>WorldDataEmitter: udpateBiome(pos)\n(read biome, compare lastBiomeId)
alt biome changed & known
WorldDataEmitter-->>RendererCommon: biomeUpdate({ biome })
else unknown biome
WorldDataEmitter-->>RendererCommon: biomeReset()
end
RendererCommon->>RendererThree: biomeUpdated(biome) / biomeReset()
RendererThree->>Skybox: updateTemperature(biome.temperature) / DEFAULT_TEMPERATURE
Bot-->>RendererCommon: timeUpdated(timeOfDay)
alt dayCycle enabled
RendererCommon->>RendererThree: timeUpdated(timeOfDay)
RendererThree->>Skybox: updateTime(timeOfDay)
else dayCycle disabled
note right of RendererCommon: time update ignored
end
RendererThree->>Skybox: update(cameraPos, viewDistance)
RendererThree->>Skybox: updateWaterState(inWater, waterBreathing)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
renderer/viewer/three/skyboxRenderer.ts (1)
56-77: Remove gradient meshes when switching to a textured skybox.Avoid double-drawing by disposing gradient planes once a texture skybox is used.
// Create or update the skybox if (this.mesh) { // Just update the texture on the existing material this.mesh.material.map = this.texture this.mesh.material.needsUpdate = true } else { + // Remove gradient sky if present + if (this.skyMesh) { + this.scene.remove(this.skyMesh) + this.skyMesh.geometry.dispose() + ;(this.skyMesh.material as THREE.Material).dispose() + this.skyMesh = null + } + if (this.voidMesh) { + this.scene.remove(this.voidMesh) + this.voidMesh.geometry.dispose() + ;(this.voidMesh.material as THREE.Material).dispose() + this.voidMesh = null + } // Create a large sphere geometry for the skybox const geometry = new THREE.SphereGeometry(500, 60, 40)
🧹 Nitpick comments (9)
renderer/viewer/lib/worldDataEmitter.ts (2)
10-10: Use a type-only import to avoid bundling minecraft-data at runtime.-import { Biome } from 'minecraft-data' +import type { Biome } from 'minecraft-data'
388-396: Call the renamed method and pass the floored position to match biome column semantics.-const posFloored = pos.floored() +const posFloored = pos.floored() if (!force && this.lastPosCheck && this.lastPosCheck.equals(posFloored)) return this.lastPosCheck = posFloored - -this.udpateBiome(pos) +this.updateBiome(posFloored)renderer/viewer/three/skyboxRenderer.ts (2)
196-211: Dead code in celestial angle calculation.
angle += (angle - angle) / 3is a no-op; remove or implement the intended smoothing.- angle = 1 - ((Math.cos(angle * Math.PI) + 1) / 2) - angle += (angle - angle) / 3 + angle = 1 - ((Math.cos(angle * Math.PI) + 1) / 2)
279-328: Fog parameters look like density; consider FogExp2 if exponential falloff was intended.
new THREE.Fog(color, 0.0025, far)uses 0.0025 as near distance (very close). If the goal is density, useTHREE.FogExp2(color, density).renderer/viewer/lib/worldrendererCommon.ts (3)
35-73: Config surface expansion is solid; consider clearer naming and light docs for new flags.
- Nit: foreground is ambiguous (UI overlay? always-on-top scene layer?). Consider renderForeground or add a short JSDoc.
- Nit: vrSupport vs vrPageGameRendering would benefit from a brief comment on when each should be used.
- Optional: add a note that volume is 0..1 and clamp at the setter site.
512-515: Avoid any for biome hooks; expose a minimal Biome-like shape.Use a small shared type instead of any to keep contracts clear without pulling heavyweight types.
Apply:
@@ -export abstract class WorldRendererCommon<WorkerSend = any, WorkerReceive = any> { +export type BiomeLike = { temperature?: number; id?: number; name?: string } +export abstract class WorldRendererCommon<WorkerSend = any, WorkerReceive = any> { @@ - biomeUpdated? (biome: any): void + biomeUpdated? (biome: BiomeLike): void @@ - biomeReset? (): void + biomeReset? (): void
837-847: Set timeOfTheDay before invoking timeUpdated to avoid subtle stale reads.If any override reads this.timeOfTheDay, updating first prevents racey/stale access.
- if (!this.worldRendererConfig.dayCycle) return - this.timeUpdated?.(timeOfDay) - - this.timeOfTheDay = timeOfDay + if (!this.worldRendererConfig.dayCycle) return + this.timeOfTheDay = timeOfDay + this.timeUpdated?.(timeOfDay)If “freeze time when dayCycle=false” is intended, current early return is correct. Confirm that behavior is desired for both skylight and skybox updates.
renderer/viewer/three/worldrendererThree.ts (2)
6-6: Type import for Biome is likely fragile; prefer a local minimal shape or shared BiomeLike.minecraft-data may not export a Biome TS type. Use a small local type or import the BiomeLike from worldrendererCommon.
-import { Biome } from 'minecraft-data' +import type { BiomeLike } from '../lib/worldrendererCommon'And update the signature below accordingly.
736-737: Guard viewDistance passed to skybox update.viewDistance initializes as -1; confirm SkyboxRenderer.update handles negatives. If not, clamp.
- this.skyboxRenderer.update(cameraPos, this.viewDistance) + this.skyboxRenderer.update(cameraPos, Math.max(0, this.viewDistance))
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
README.MD(1 hunks)renderer/viewer/lib/worldDataEmitter.ts(3 hunks)renderer/viewer/lib/worldrendererCommon.ts(4 hunks)renderer/viewer/three/skyboxRenderer.ts(3 hunks)renderer/viewer/three/worldrendererThree.ts(5 hunks)src/dayCycle.ts(0 hunks)src/index.ts(0 hunks)src/watchOptions.ts(1 hunks)
💤 Files with no reviewable changes (2)
- src/index.ts
- src/dayCycle.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/vars-usage.mdc)
src/**/*.ts: You may use the global variablebotdirectly in any file under thesrc/directory (e.g.,src/mineflayer/playerState.ts).
Insrc/code, you may use the global variableappViewerfromsrc/appViewer.tsdirectly. Do not importappVieweror usewindow.appViewer; use the globalappViewervariable as-is.
Files:
src/watchOptions.ts
renderer/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/vars-usage.mdc)
renderer/**/*.ts: Do not use the global variablebotdirectly in any file under therenderer/directory or its subfolders (e.g.,renderer/viewer/three/worldrendererThree.ts).
In renderer code, all bot/player state and events must be accessed via explicit interfaces, state managers, or passed-in objects, never by referencingbotdirectly.
In renderer code (such as inWorldRendererThree), use theplayerStateproperty (e.g.,worldRenderer.playerState.gameMode) to access player state. The implementation forplayerStatelives insrc/mineflayer/playerState.ts.
Files:
renderer/viewer/lib/worldDataEmitter.tsrenderer/viewer/three/worldrendererThree.tsrenderer/viewer/three/skyboxRenderer.tsrenderer/viewer/lib/worldrendererCommon.ts
🧠 Learnings (2)
📚 Learning: 2025-07-27T13:24:58.186Z
Learnt from: CR
PR: zardoy/minecraft-web-client#0
File: .cursor/rules/vars-usage.mdc:0-0
Timestamp: 2025-07-27T13:24:58.186Z
Learning: Applies to renderer/**/*.ts : In renderer code (such as in `WorldRendererThree`), use the `playerState` property (e.g., `worldRenderer.playerState.gameMode`) to access player state. The implementation for `playerState` lives in `src/mineflayer/playerState.ts`.
Applied to files:
renderer/viewer/three/worldrendererThree.ts
📚 Learning: 2025-06-23T13:33:14.776Z
Learnt from: zardoy
PR: zardoy/minecraft-web-client#373
File: renderer/viewer/three/entities.ts:1120-1120
Timestamp: 2025-06-23T13:33:14.776Z
Learning: In the minecraft-web-client project, files under renderer/ directory must not access the global `bot` variable directly according to .cursor/rules/vars-usage.mdc. The updateNameTagVisibility method in renderer/viewer/three/entities.ts currently violates this rule by accessing bot.teamMap and bot.username. Team information should be passed through entity update events from the world data emitter instead.
Applied to files:
renderer/viewer/three/worldrendererThree.ts
🧬 Code graph analysis (2)
src/watchOptions.ts (1)
src/appViewer.ts (1)
appViewer(305-305)
renderer/viewer/three/worldrendererThree.ts (1)
renderer/viewer/three/skyboxRenderer.ts (1)
DEFAULT_TEMPERATURE(3-3)
🪛 LanguageTool
README.MD
[grammar] ~236-~236: Ensure spelling is correct
Context: ...done clone from the graphical side that inspred many features here
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~236-~236: There might be a mistake here.
Context: ...cal side that inspred many features here
(QB_NEW_EN)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-deploy
🔇 Additional comments (5)
src/watchOptions.ts (1)
131-132: Mapping verified:options.dayCycleAndLightingis defined in defaultOptions.ts and optionsGuiScheme.tsx, andinWorldRenderingConfig.dayCycleexists on AppViewer and is properly set in watchOptions.ts.renderer/viewer/lib/worldrendererCommon.ts (1)
849-856: Emitter typing verified for biome events
WorldDataEmitterEvents interface declares both biomeUpdate and biomeReset, and WorldDataEmitterWorker extends TypedEmitter, so the new events are correctly typed.renderer/viewer/three/worldrendererThree.ts (3)
176-181: Correctly synchronizing inWater and waterBreathing with the skybox.Passing both flags avoids stale combinations and matches the guidelines (uses playerState, no global bot).
271-273: Driving skybox time from renderer time updates is correct.Alignment with dayCycle gating in WorldRendererCommon looks consistent.
275-284: Biome-driven temperature updates and reset default are sensible.Guarding on undefined preserves legitimate 0 temperatures; resetting to DEFAULT_TEMPERATURE on unknown biome is reasonable.
| lastBiomeId: number | null = null | ||
|
|
||
| udpateBiome (pos: Vec3) { | ||
| try { | ||
| const biomeId = this.world.getBiome(pos) | ||
| if (biomeId !== this.lastBiomeId) { | ||
| this.lastBiomeId = biomeId | ||
| const biomeData = loadedData.biomes[biomeId] | ||
| if (biomeData) { | ||
| this.emitter.emit('biomeUpdate', { | ||
| biome: biomeData | ||
| }) | ||
| } else { | ||
| // unknown biome | ||
| this.emitter.emit('biomeReset') | ||
| } | ||
| } | ||
| } catch (e) { | ||
| console.error('error updating biome', e) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Undefined identifier ‘loadedData’; biome lookup will throw. Also fix method name typo.
loadedData is not defined; use the bot registry (already used elsewhere in this file) for biome metadata. Rename udpateBiome → updateBiome.
-lastBiomeId: number | null = null
-
-udpateBiome (pos: Vec3) {
+lastBiomeId: number | null = null
+
+updateBiome (pos: Vec3) {
try {
- const biomeId = this.world.getBiome(pos)
+ const biomeId = this.world.getBiome(pos)
if (biomeId !== this.lastBiomeId) {
this.lastBiomeId = biomeId
- const biomeData = loadedData.biomes[biomeId]
+ const biomeData =
+ // mineflayer registry varies by version
+ (bot as any)?.registry?.biomes?.[biomeId] ??
+ (bot as any)?.registry?.biomesById?.[biomeId]
if (biomeData) {
this.emitter.emit('biomeUpdate', {
biome: biomeData
})
} else {
// unknown biome
this.emitter.emit('biomeReset')
}
}
} catch (e) {
console.error('error updating biome', e)
}
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In renderer/viewer/lib/worldDataEmitter.ts around lines 366 to 386, the method
name is misspelled and it references an undefined identifier: rename udpateBiome
to updateBiome and replace the undefined loadedData lookup with the existing bot
registry biome metadata used elsewhere in this file (e.g. use this.botRegistry
or the local botRegistry variable to fetch biomes) so biomeId maps to
registry.biomes[biomeId]; preserve the existing lastBiomeId change and emitter
calls, and keep the try/catch and null/undefined check for missing biome data to
emit 'biomeReset'.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
1 participant
Inspred from js-minecraft
Summary by CodeRabbit
New Features
Changes
Documentation