Skip to content

feat: improve fog a little #427

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Sep 19, 2025
Merged

feat: improve fog a little #427

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5c82276
try to improve fog!
zardoy Sep 11, 2025
ac47a37
disable debug
zardoy Sep 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 38 additions & 5 deletions renderer/viewer/three/skyboxRenderer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import * as THREE from 'three'
import { DebugGui } from '../lib/DebugGui'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.


export const DEFAULT_TEMPERATURE = 0.75

Expand All @@ -17,11 +18,33 @@ export class SkyboxRenderer {
private waterBreathing = false
private fogBrightness = 0
private prevFogBrightness = 0
private readonly fogOrangeness = 0 // Debug property to control sky color orangeness
private readonly distanceFactor = 2.7

private readonly brightnessAtPosition = 1
Comment on lines +21 to +24
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

debugGui: DebugGui

constructor (private readonly scene: THREE.Scene, public defaultSkybox: boolean, public initialImage: string | null) {
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()
}

async init () {
Expand Down Expand Up @@ -95,6 +118,7 @@ export class SkyboxRenderer {

// Update world time
updateTime (timeOfDay: number, partialTicks = 0) {
if (this.debugGui.visible) return
this.worldTime = timeOfDay
this.partialTicks = partialTicks
this.updateSkyColors()
Expand All @@ -108,19 +132,22 @@ export class SkyboxRenderer {

// Update temperature (for biome support)
updateTemperature (temperature: number) {
if (this.debugGui.visible) return
this.temperature = temperature
this.updateSkyColors()
}

// Update water state
updateWaterState (inWater: boolean, waterBreathing: boolean) {
if (this.debugGui.visible) return
this.inWater = inWater
this.waterBreathing = waterBreathing
this.updateSkyColors()
}

// Update default skybox setting
updateDefaultSkybox (defaultSkybox: boolean) {
if (this.debugGui.visible) return
this.defaultSkybox = defaultSkybox
this.updateSkyColors()
}
Expand Down Expand Up @@ -229,8 +256,15 @@ export class SkyboxRenderer {
if (temperature < -1) temperature = -1
if (temperature > 1) temperature = 1

const hue = 0.622_222_2 - temperature * 0.05
const saturation = 0.5 + temperature * 0.1
// 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
const saturation = 0.5 + temperature * 0.1 + Math.abs(this.fogOrangeness) * 0.3 // Increase saturation with orangeness
const brightness = 1

return this.hsbToRgb(hue, saturation, brightness)
Expand Down Expand Up @@ -305,8 +339,7 @@ export class SkyboxRenderer {
// Update fog brightness with smooth transition
this.prevFogBrightness = this.fogBrightness
const renderDistance = this.viewDistance / 32
const brightnessAtPosition = 1 // Could be affected by light level in future
const targetBrightness = brightnessAtPosition * (1 - renderDistance) + renderDistance
const targetBrightness = this.brightnessAtPosition * (1 - renderDistance) + renderDistance
this.fogBrightness += (targetBrightness - this.fogBrightness) * 0.1

// Handle water fog
Expand Down Expand Up @@ -340,7 +373,7 @@ export class SkyboxRenderer {
const blue = (fogColor.z + (skyColor.z - fogColor.z) * viewFactor) * clampedBrightness * interpolatedBrightness

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)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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.


;(this.skyMesh.material as THREE.MeshBasicMaterial).color.set(new THREE.Color(skyColor.x, skyColor.y, skyColor.z))
;(this.voidMesh.material as THREE.MeshBasicMaterial).color.set(new THREE.Color(
Expand Down
Loading