feat: improve fog a little #427
Conversation
|
/deploy |
|
Caution Review failedThe pull request is closed. WalkthroughAdded a DebugGui to SkyboxRenderer with seven controls (temperature, worldTime, inWater, waterBreathing, fogOrangeness, brightnessAtPosition, distanceFactor). New public Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant DebugGui
participant Skybox as SkyboxRenderer
participant RenderLoop as Render Loop
User->>DebugGui: Tweak controls
DebugGui-->>Skybox: Update parameter values
loop each frame
RenderLoop->>Skybox: update()
alt debugGui.visible
Skybox-->>RenderLoop: skip updateTime/updateTemperature/updateWaterState/updateDefaultSkybox
else
Skybox->>Skybox: run updateTime/updateTemperature/updateWaterState/updateDefaultSkybox
end
Skybox->>Skybox: compute sky color (fogOrangeness)
Skybox->>Skybox: set target brightness = brightnessAtPosition
Skybox->>Skybox: set fog end = viewDistance * distanceFactor
Skybox-->>RenderLoop: render with updated params
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks (3 passed)✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Deployed to Vercel Preview: https://prismarine-2vh50gvkv-zaro.vercel.app |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
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)
386-405: Debug GUI cleanup missing in dispose methodThe
disposemethod cleans up Three.js resources but doesn't dispose of the debug GUI, which could lead to memory leaks.dispose () { + if (this.debugGui) { + this.debugGui.destroy() + } if (this.texture) { this.texture.dispose() }
🧹 Nitpick comments (2)
renderer/viewer/three/skyboxRenderer.ts (2)
121-125: Consider a cleaner pattern for debug GUI override checksThe repeated pattern of checking
this.debugGui.visibleat the start of each update method could be consolidated using a decorator pattern or a wrapper method to reduce code duplication and improve maintainability.Add a helper method:
private shouldSkipUpdate(): boolean { return this.debugGui?.visible ?? false; }Then update the methods:
updateTime (timeOfDay: number, partialTicks = 0) { - if (this.debugGui.visible) return + if (this.shouldSkipUpdate()) return this.worldTime = timeOfDay this.partialTicks = partialTicks this.updateSkyColors() }Apply the same pattern to
updateTemperature,updateWaterState, andupdateDefaultSkybox.Also applies to: 135-138, 142-146, 150-153
259-267: Simplify the fog orangeness hue calculationThe ternary operator for calculating the hue could be simplified for better readability. Also, the magic numbers (0.8, 0.1) should be documented or extracted as named constants.
- // Apply debug fog orangeness to hue - positive values make it more orange, negative make it less orange - const baseHue = 0.622_222_2 - temperature * 0.05 - // Orange is around hue 0.08-0.15, so we need to shift from blue-purple (0.62) toward orange - // Use a more dramatic shift and also increase saturation for more noticeable effect - const orangeHue = 0.12 // Orange hue value - const hue = this.fogOrangeness > 0 - ? baseHue + (orangeHue - baseHue) * this.fogOrangeness * 0.8 // Blend toward orange - : baseHue + this.fogOrangeness * 0.1 // Subtle shift for negative values + // Apply debug fog orangeness to hue - positive values make it more orange, negative make it less orange + const baseHue = 0.622_222_2 - temperature * 0.05 + const orangeHue = 0.12 // Orange hue value + const ORANGE_BLEND_FACTOR = 0.8 + const NEGATIVE_SHIFT_FACTOR = 0.1 + + const hueShift = this.fogOrangeness > 0 + ? (orangeHue - baseHue) * this.fogOrangeness * ORANGE_BLEND_FACTOR + : this.fogOrangeness * NEGATIVE_SHIFT_FACTOR + const hue = baseHue + hueShift
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
renderer/viewer/three/skyboxRenderer.ts(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
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/three/skyboxRenderer.ts
🧬 Code graph analysis (1)
renderer/viewer/three/skyboxRenderer.ts (1)
renderer/viewer/lib/DebugGui.ts (1)
DebugGui(10-174)
⏰ 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
| @@ -1,4 +1,5 @@ | |||
| import * as THREE from 'three' | |||
| import { DebugGui } from '../lib/DebugGui' | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider making the debug GUI integration optional
The debug GUI is being activated by default for all instances of SkyboxRenderer. This could expose internal tuning parameters in production builds, which is typically undesirable. Consider making it opt-in through a constructor parameter or environment variable.
- constructor (private readonly scene: THREE.Scene, public defaultSkybox: boolean, public initialImage: string | null) {
+ constructor (private readonly scene: THREE.Scene, public defaultSkybox: boolean, public initialImage: string | null, enableDebugGui = false) {
this.debugGui = new DebugGui('skybox_renderer', this, [
'temperature',
'worldTime',
'inWater',
'waterBreathing',
'fogOrangeness',
'brightnessAtPosition',
'distanceFactor'
], {
brightnessAtPosition: { min: 0, max: 1, step: 0.01 },
temperature: { min: 0, max: 1, step: 0.01 },
worldTime: { min: 0, max: 24_000, step: 1 },
fogOrangeness: { min: -1, max: 1, step: 0.01 },
distanceFactor: { min: 0, max: 5, step: 0.01 },
})
if (!initialImage) {
this.createGradientSky()
}
- this.debugGui.activate()
+ if (enableDebugGui) {
+ this.debugGui.activate()
+ }
}Also applies to: 25-25, 28-47
🤖 Prompt for AI Agents
In renderer/viewer/three/skyboxRenderer.ts (noting usages around lines 2, 25 and
28-47), the DebugGui is imported/activated unconditionally; change the class to
make debug GUI integration opt-in by removing module-level/automatic
initialization and adding a constructor option (e.g., enableDebug?: boolean) or
respect an environment flag; only import or instantiate DebugGui when that flag
is true, guard all GUI-related calls behind that flag, and ensure any created
GUI is cleaned up in the renderer's dispose/destroy method.
| private readonly fogOrangeness = 0 // Debug property to control sky color orangeness | ||
| private readonly distanceFactor = 2.7 | ||
|
|
||
| private readonly brightnessAtPosition = 1 |
There was a problem hiding this comment.
Debug parameters should be mutable when GUI is active
The fogOrangeness, distanceFactor, and brightnessAtPosition properties are marked as readonly, which prevents the DebugGui from modifying them. Remove the readonly modifier to allow runtime adjustments through the GUI.
- private readonly fogOrangeness = 0 // Debug property to control sky color orangeness
- private readonly distanceFactor = 2.7
-
- private readonly brightnessAtPosition = 1
+ private fogOrangeness = 0 // Debug property to control sky color orangeness
+ private distanceFactor = 2.7
+
+ private brightnessAtPosition = 1📝 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.
| private readonly fogOrangeness = 0 // Debug property to control sky color orangeness | |
| private readonly distanceFactor = 2.7 | |
| private readonly brightnessAtPosition = 1 | |
| private fogOrangeness = 0 // Debug property to control sky color orangeness | |
| private distanceFactor = 2.7 | |
| private brightnessAtPosition = 1 |
🤖 Prompt for AI Agents
In renderer/viewer/three/skyboxRenderer.ts around lines 21 to 24, the properties
fogOrangeness, distanceFactor, and brightnessAtPosition are declared readonly
which prevents the DebugGui from changing them at runtime; remove the readonly
modifier from these three property declarations so they become mutable class
fields (retain their current initial values and types) to allow the GUI to
update them during execution.
|
|
||
| this.scene.background = new THREE.Color(red, green, blue) | ||
| this.scene.fog = new THREE.Fog(new THREE.Color(red, green, blue), 0.0025, viewDistance * 2) | ||
| this.scene.fog = new THREE.Fog(new THREE.Color(red, green, blue), 0.0025, viewDistance * this.distanceFactor) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate distanceFactor to prevent rendering issues
The fog end distance calculation uses this.distanceFactor directly without validation. If the debug GUI allows values close to 0, it could cause rendering issues with very short fog distances.
- this.scene.fog = new THREE.Fog(new THREE.Color(red, green, blue), 0.0025, viewDistance * this.distanceFactor)
+ const safeFogDistance = Math.max(viewDistance * Math.max(0.1, this.distanceFactor), 1)
+ this.scene.fog = new THREE.Fog(new THREE.Color(red, green, blue), 0.0025, safeFogDistance)📝 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.
| this.scene.fog = new THREE.Fog(new THREE.Color(red, green, blue), 0.0025, viewDistance * this.distanceFactor) | |
| const safeFogDistance = Math.max(viewDistance * Math.max(0.1, this.distanceFactor), 1) | |
| this.scene.fog = new THREE.Fog(new THREE.Color(red, green, blue), 0.0025, safeFogDistance) |
🤖 Prompt for AI Agents
In renderer/viewer/three/skyboxRenderer.ts around line 376, the fog end distance
uses this.distanceFactor without validation which can produce extremely small or
zero distances; validate and sanitize this.distanceFactor before using it:
coerce to a finite number, clamp it to a sensible minimum (e.g.
Math.max(Number(this.distanceFactor) || 1, 0.01)) or a configured lower bound,
and ensure the computed end distance is at least slightly greater than the start
distance (add a small epsilon) to avoid rendering issues.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
| private readonly fogOrangeness = 0 // Debug property to control sky color orangeness | ||
| private readonly distanceFactor = 2.7 | ||
|
|
||
| private readonly brightnessAtPosition = 1 |
There was a problem hiding this comment.
Bug: Debug GUI Fails to Modify Readonly Properties
The fogOrangeness, distanceFactor, and brightnessAtPosition properties are declared as readonly but are included in the DebugGui controls. This prevents the debug GUI from modifying these values, making the debug controls non-functional for these properties.
1 participant
Summary by CodeRabbit
New Features
Improvements
Public API