Conversation
|
orrery preview: https://pr-262.orrery-c4f.pages.dev/ |
There was a problem hiding this comment.
The new Mars appearance features are solid, but there are a couple of composition hazards: both material.onBeforeCompile and the per-frame update logic can be overwritten by future features, creating ordering-dependent bugs. The generic atmosphere shader also uses a normal transform that will be wrong under non-uniform scaling, which is worth correcting now since this is a reusable layer. Finally, consider clamping atmosphere config inputs defensively to avoid unstable rendering when data is malformed or extended.
Additional notes (1)
- Maintainability |
apps/orrery/src/scene/BodyMesh.ts:191-199
defaultRoughnessis computed fromtextureKind, but the actual roughness source is nowsurface.roughness ?? defaultRoughness. That means any body withsurface.roughnessset will ignore the special-casing for the Sun (which may be intended), but more importantly it also meansdefaultRoughnessis effectively a constant fallback and could drift from the material intent if more cases appear.
Consider making the fallback logic more explicit (and centralized) so it’s clear what the canonical default is per kind/body, and so future additions don’t silently change behavior.
Summary of changes
Summary
This PR updates Mars’ visual appearance and adds generic rendering hooks to support non-Earth bodies:
BodySurfaceStyleextended with optionalroughnessandnightSidedarkening controls.- New layer kind:
atmosphereviaAtmosphereAppearanceLayerStyleandisAtmosphereAppearanceLayer(). createBodyMesh()enhancements:- Uses
surface.roughness ?? defaultinstead of a hardcoded roughness. - Adds generic night-side darkening (shader patch on
MeshStandardMaterial) when not using Earth layers. - Adds generic additive atmosphere shell as a
ShaderMateriallayer. - Installs a minimal per-frame
updateto keepuSunDirWorldcurrent for these effects.
- Uses
- Mars registry tuning:
- Slightly reduced roughness.
- Night-side ambient suppression.
- Thin, warm atmosphere shell.
- Snapshot updated to reflect new rendering output.
|
@CharlieHelps the codebase has updated significantly since you cut this PR. Please do the following:
|
|
Checked this today — the branch is significantly behind \ (GitHub shows 19 commits) and the conflicts overlap with the recent refactors. Rather than fight the conflicts, I’m going to re-port the appearance changes on top of a fresh \ and update the PR branch. Also confirmed: I’ll switch the Mars texture to the USGS Viking colorized mosaic (public domain) and add proper attribution. I’ll push an updated commit + refreshed snapshot soon. |
|
Quick correction: the branch is behind |
|
Checked: this PR is now quite behind Plan: I’m going to re-port/re-implement the Mars appearance tweaks on top of fresh I’ll also swap the Mars texture to the USGS Viking colorized global mosaic (public domain) and add attribution. Updated commit + refreshed snapshot coming shortly. |
db28938 to
08cf875
Compare
|
Update on this PR:
|
Mars Rendering Enhancements (Three.js)This document outlines a focused set of high-impact visual enhancements for rendering Mars in Three.js, building on a high-quality albedo base (USGS Astropedia Viking Colorized Global Mosaic). Multi-map PBR from Real Mars DatasetsUse the Viking mosaic strictly as albedo, then layer physically motivated maps. Albedo
Normal Map (from elevation)
Roughness MapMars roughness varies significantly:
Approaches:
Ambient Occlusion
Material Guidance
Macro Detail Without Tiling ArtifactsHigh-resolution global maps still look flat up close. Detail Albedo Overlay
Avoid:
Detail Normal Overlay
Implementation Notes
Atmosphere (Thin Haze, High Impact)Mars’ atmosphere is thin but visually critical. Single-Scattering Model
Terminator Softening & Night-Side BehaviorMars should not have a hard day/night cutoff. Terminator Softening
Night-Side Treatment
This prevents the planet from looking like a CG sphere. Volumetric Dust & Atmospheric VeilsMars dust is subtle but adds life. Dust Layer Shell
Masking
Visual Goal
Closing NotesKey realism drivers:
Mars looks best when:
|
|
Thanks for the spec doc update (issuecomment-3857951461) — I aligned this PR with it and focused on getting the Mars material/shading behavior closer to the intended model. Implemented in this update
Not implemented yet (planned / next steps)
Questions
|
|
@CharlieHelps thank you, this looks great so far! Keep going on your next steps work.
DEM: Global MOLA gridded DEM, used to derive normals and optional AO. |
|
Thanks for confirming the dataset direction. I’ll proceed with the Global MOLA gridded DEM for normals/AO, and a synthetic roughness proxy derived from DEM slope + albedo luminance. To keep the repo size reasonable, I’ll start with a downsampled MOLA/MEGDR-derived texture and wire it up as explicit normal/roughness textures (linear color space). Re: resolution + workflow — do you have a preference for 2k vs 4k, and are you OK with checking the generated textures into the repo, or would you rather they be generated at build time? (Replying to #262 (comment)) |
|
Pushed updates for normal + roughness texture support (wiring/handling in appearance pipeline). Remaining:
|
|
Pushed updates for Mars appearance:\n\n- Added DEM-derived normal map + roughness proxy (2k) + enabled for Mars\n- Added generator script: |
|
@CharlieHelps thanks, great work. Can you please fix CI? Also, was the atmosphere and dust shell/aerosol layer from this comment implemented as well? |
|
@rybosome CI is green now — the failing Prettier check on is fixed in e60e6fa (https://github.com/rybosome/tspice/commit/e60e6faec8531fbe4a2c9a67cbed4e33f732fc4b).\n\nRe: atmosphere/dust shell — we do have an atmosphere layer implemented as a stylized rim-glow “atmosphere shell” (not physically-based scattering): (layer type), (shell mesh), (Mars config).\n\nWe don’t currently have a separate dust/aerosol shell layer type; right now it’s just the atmosphere shell. |
|
@rybosome Follow-up (my last comment got mangled by shell quoting): CI is green now — the failing Prettier check on Re: atmosphere/dust shell
|
@CharlieHelps ok, this all looks really good as is. My only feedback is that we should drop the default "sun light" rendering setting down to 2.1 (globally) because the planets are starting to look a little too bright. Can you also go ahead and implement this dust/aerosol shell layer? |
|
Replying to #262 (comment) Pushed the requested tweaks:
This is intentionally a stylized rim-glow shell (no physical scattering model) — the look is driven by tweakable parameters so we can dial it in as needed. |
There was a problem hiding this comment.
Key issues to address are: (1) the normal/roughness maps may not be applied immediately after async load (currently only applied in ready), and (2) the atmosphere/aerosol shaders use a fragile/incorrect world-normal reconstruction via transpose(viewMatrix). The e2e “force renders” approach is also coupled to a perf-counter API and may still be flaky. There’s also significant shader duplication between atmosphere and aerosol that should be factored to prevent drift.
Additional notes (2)
- Readability |
apps/orrery/src/scene/BodyMesh.ts:241-286
applySurfaceMaps()setsmaterial.normalScaleeven whennormalMapisnull. That’s mostly harmless, but it can be misleading (it suggests normals are active) and can also overwrite any earlier tuning that expectsnormalScaleto be preserved when no normal map is used.
Additionally, normalScale defaults to (1,1) which may be too strong if a normal map is later attached asynchronously and surface.normalScale is omitted; you’ll effectively enable full-strength normals by default.
- Maintainability |
apps/orrery/src/scene/BodyMesh.ts:448-452
stableHash01(options.bodyId ?? '')means bodies withoutbodyIdwill all share the samedetailNoiseSeed(0 for empty string hash outcome depends but will be constant), producing identical noise patterns. IfbodyIdis optional for a reason, this can create surprising “same detail” across multiple procedurally-generated bodies.
Also, detailNoiseSeed is used as a float uniform and added as vec3(uDetailNoiseSeed). Very large seeds (if user-provided) can cause precision banding in the noise due to float mantissa limits.
Summary of changes
What changed
Mars appearance + PBR inputs
- Added Mars albedo texture
mars-viking-colorized-4k.jpgand attribution inapps/orrery/public/textures/planets/README.md. - Added DEM-derived normal and roughness proxy textures (
mars-mola-normal-2k.png,mars-roughness-proxy-2k.png). - Updated
apps/orrery/src/scene/BodyRegistry.tsMars entry to use:normalTexture+normalScaleroughnessTextureroughness: 1.0,metalness: 0.0,bumpScale: 0.0nightAlbedo,terminatorTwilight, and optionaldetailNoise
New appearance features
- Extended
BodySurfaceStyleinapps/orrery/src/scene/SceneModel.tswith:normalTexture,roughnessTexture,normalScaledetailNoise
- Added new non-Earth layer kinds:
kind: "atmosphere"(isAtmosphereAppearanceLayer)kind: "aerosol"(isAerosolAppearanceLayer)
- Implemented generic atmosphere and aerosol/dust rim-glow shells in
apps/orrery/src/scene/BodyMesh.ts.
Rendering + update-loop composition improvements
- Added
composeUpdate()helper and switched multiple features to compose update handlers instead of clobbering them. - Added deterministic
stableHash01()to seed procedural detail noise without breaking e2e determinism.
Tooling / tests
- Added generator script
apps/orrery/scripts/generate-mars-maps.js(+scripts/data/.gitignore) and new dev depsjpeg-js+pngjs. - Improved Playwright e2e stability via
waitForViewerReadyForScreenshot(). - Reduced default sun light intensity in
apps/orrery/src/SceneCanvas.tsxfrom3.5to2.1.
|
@CharlieHelps I need your help preparing this PR to merge. You should read the diff, related issues/docs, and PR reviews closely and then ensure ALL of the following is true:
It's your job to push commits to the PR to complete all of these tasks. You MUST wait to confirm that ALL of the above is true before stopping. I expect to be able to merge the PR after you are done. |
|
Pushed updates to
|
There was a problem hiding this comment.
Main remaining risks are around defaults/robustness rather than compilation: applySurfaceMaps() currently forces normalScale even when normals aren’t used (and defaults to full strength), detailNoiseSeed collapses to a constant when bodyId is missing, and the e2e renderNTimes() hook improves stability but still can’t guarantee texture upload completion. The rim-glow shell helper is good, but could be slightly more efficient by sharing geometry if many shells are added later.
Additional notes (2)
- Maintainability |
apps/orrery/src/scene/BodyMesh.ts:453-457
detailNoiseSeedfalls back tostableHash01(options.bodyId ?? ''). WhenbodyIdis missing, all bodies will share the same seed, producing identical noise patterns across different procedurally-defined bodies.
That’s surprising behavior if this API is used for ad-hoc bodies or future dynamic registries.
- Compatibility |
apps/orrery/scripts/generate-mars-maps.js:332-339
Generator note: roughness proxy uses albedo luminance computed directly from decoded JPEG byte values (implicitly gamma-encoded), but the comment says the texture is “effectively sRGB” and this is “heuristic.” This is acceptable for a proxy, but the current approach makes the resulting roughness sensitive to color grading/gamma in the source mosaic.
If the goal is “broad dust vs rock behavior,” converting to linear first (approx) would better match physical intuition and reduce dependence on baked contrast.
Summary of changes
Summary
This diff ports and finalizes the Mars appearance upgrade and supporting infrastructure across the orrery app:
-
E2E screenshot stability
- Adds
window.__tspice_viewer__e2e.renderNTimes(n)and uses it insun-postprocessing.spec.tsviawaitForViewerReadyForScreenshot(). - Documents the new hook in
apps/orrery/e2e/README.md.
- Adds
-
Global lighting tweak
- Lowers the default sun light intensity from
3.5→2.1inapps/orrery/src/SceneCanvas.tsx.
- Lowers the default sun light intensity from
-
Mars assets + attribution
- Adds new Mars textures:
mars-viking-colorized-4k.jpg(USGS Viking mosaic)mars-mola-normal-2k.png(DEM-derived normal)mars-roughness-proxy-2k.png(slope + albedo-derived roughness proxy)
- Updates
apps/orrery/public/textures/planets/README.mdwith licensing/source notes.
- Adds new Mars textures:
-
Generator tooling
- Adds
apps/orrery/scripts/generate-mars-maps.jsto download/validate PDS MOLA DEM + generate normal/roughness maps. - Adds
apps/orrery/scripts/data/.gitignoreto keep downloaded inputs out of git. - Adds
jpeg-jsandpngjsdev dependencies.
- Adds
-
Rendering pipeline enhancements
- Extends
BodySurfaceStylewithnormalTexture,roughnessTexture,normalScale, anddetailNoise. - Introduces new appearance layer kinds in
SceneModel.ts:kind: "atmosphere"andkind: "aerosol". - Updates
BodyMesh.tsto:- Load/apply normal + roughness maps eagerly as they arrive (not only at
ready). - Add deterministic procedural
detailNoisevia shader patching. - Add shared rim-glow shell helper used for Earth atmosphere and non-Earth atmosphere/aerosol.
- Ensure sun direction uniforms stay updated via composed
update.
- Load/apply normal + roughness maps eagerly as they arrive (not only at
- Extends
-
Mars registry configuration
- Wires Mars to use the new USGS albedo + MOLA normal/roughness textures and enables
atmosphere+aerosollayers inapps/orrery/src/scene/BodyRegistry.ts.
- Wires Mars to use the new USGS albedo + MOLA normal/roughness textures and enables
|
@CharlieHelps I need your help preparing this PR to merge. You should read the diff, related issues/docs, and PR reviews closely and then ensure ALL of the following is true:
It's your job to push commits to the PR to complete all of these tasks. You MUST wait to confirm that ALL of the above is true before stopping. I expect to be able to merge the PR after you are done. |
|
Summary of fixes for review:
Status:
Remaining gate:
|
There was a problem hiding this comment.
Most of the prior review hazards (shader/update composition, normal-space correctness, eager map application) appear addressed, but a few robustness/perf issues remain: detailNoiseSeed collapses to a constant when bodyId is missing, disposeNormalMap() still clobbers normalScale despite the new non-clobber policy in applySurfaceMaps(), rim-glow shells duplicate identical geometry allocations, and the e2e gl.finish() stabilization should be wrapped defensively to avoid platform-specific failures.
Summary of changes
Mars appearance + rendering pipeline enhancements
- New Mars textures added under
apps/orrery/public/textures/planets/:mars-viking-colorized-4k.jpg(USGS Viking colorized mosaic)mars-mola-normal-2k.png(DEM-derived normal map)mars-roughness-proxy-2k.png(DEM slope + albedo luminance roughness proxy)
- Texture attribution documented in
apps/orrery/public/textures/planets/README.md. - Generator script added:
apps/orrery/scripts/generate-mars-maps.js(downloads PDS MOLA DEM + generates normal/roughness textures), withapps/orrery/scripts/data/.gitignoreto keep downloaded inputs out of git. - Mars registry config updated (
apps/orrery/src/scene/BodyRegistry.ts) to:- Use USGS albedo (
mars-viking-colorized-4k.jpg) - Enable
normalTexture,roughnessTexture,normalScale,detailNoise - Add new
layers:kind: "atmosphere"andkind: "aerosol"
- Use USGS albedo (
New appearance model capabilities
BodySurfaceStyleextended (apps/orrery/src/scene/SceneModel.ts) with:normalTexture,roughnessTexture,normalScaledetailNoiseblock
- Added new layer types and predicates:
AtmosphereAppearanceLayerStyle+isAtmosphereAppearanceLayer()AerosolAppearanceLayerStyle+isAerosolAppearanceLayer()
BodyMesh rendering behavior changes
apps/orrery/src/scene/BodyMesh.ts:- Adds eager async loading for
normalTextureandroughnessTexturevialoadTextureCached(). - Centralizes map assignment in
applySurfaceMaps()and applies maps immediately after async load. - Adds deterministic procedural
detailNoiseshader patch. - Adds a shared
createRimGlowShell()helper for Earth atmosphere and non-Earth atmosphere/aerosol shells. - Adds
ensureSunDirWorldUpdate()to compose per-frame update behavior.
- Adds eager async loading for
E2E stability improvements
- Adds an explicit e2e hook
window.__tspice_viewer__e2e.renderNTimes(n)implemented inapps/orrery/src/SceneCanvas.tsx. - Updates
apps/orrery/e2e/sun-postprocessing.spec.tsto wait forrenderNTimes()before screenshots. - Documents the hook in
apps/orrery/e2e/README.mdand types it inapps/orrery/src/e2eHooks/globals.d.ts.
Global lighting tweak
- Lowers default sun light intensity in
apps/orrery/src/SceneCanvas.tsxfrom3.5to2.1.
Tooling deps
- Adds
jpeg-jsandpngjstoapps/orrery/package.json(and lockfile).
|
@CharlieHelps I need your help preparing this PR to merge. You should read the diff, related issues/docs, and PR reviews closely and then ensure ALL of the following is true:
It's your job to push commits to the PR to complete all of these tasks. You MUST wait to confirm that ALL of the above is true before stopping. I expect to be able to merge the PR after you are done. |
|
Update pushed in
Status:
@rybosome when you’re happy with this set, could you give final approval so this can be merged? |
There was a problem hiding this comment.
Most of the prior blocking review hazards (shader/update composition, normal-space correctness, eager map application, and e2e render hook) are addressed well. Remaining concerns are mostly robustness and future-proofing: detailNoiseSeed fallback via JSON.stringify(options.appearance) is brittle/expensive, renderNTimes() needs more defensive guards around WebGL sync primitives, and createRimGlowShell() can avoid repeated identical geometry allocations. None of these are guaranteed to break today, but they are likely maintenance/perf traps as the appearance system grows.
Additional notes (4)
- Maintainability |
apps/orrery/e2e/sun-postprocessing.spec.ts:28-43
waitForViewerReadyForScreenshot()always waits forrenderNTimesto exist, but it doesn’t enforce any minimum meaningful render count at the call site. If a caller passes{ renders: 0 }(orNaN) it will still call through;renderNTimes()clamps internally, but the test helper should be explicit about its own contract.
Also, consider adding a comment clarifying why the third test uses renders: 8 (it reads like a magic number).
- Maintainability |
apps/orrery/src/SceneCanvas.tsx:1164-1212
renderNTimes()does a WebGL2fenceSyncand then loops up to 30 frames / 500ms waiting. This is better thangl.finish(), but it can still introduce nondeterminism across slow CI hosts (timing/RAF scheduling variability) and may mask real readiness signals.
At minimum, consider making the behavior observable from the test side (e.g. returning whether the fence signaled) so failures can be diagnosed, or add a hard error when GPU sync never signals in CI (optional).
- Maintainability |
apps/orrery/src/scene/BodyMesh.ts:246-294
applySurfaceMaps()setsmaterial.normalScalewhen a normal map is present, defaulting to0.25. That’s good, butnormalScaleis not clamped.
A malformed registry value (negative or very large) can create extreme shading artifacts or even NaNs in some shader paths. You’re already clamping many other appearance inputs—this should be similarly defensive.
- Readability |
apps/orrery/src/scene/BodyRegistry.ts:203-252
In Mars registry config, you setroughness: 1.0while also providing aroughnessTexture. In Three.js,roughnessmultiplies the map, so with the roughness proxy already near ~0.9–1.0 this likely compresses variation and may make the map’s contribution hard to see.
If the intent is “mostly matte but still spatially varying,” consider using roughness as a global multiplier less than 1 to reintroduce headroom, or adjust the generator’s base/amp accordingly. Right now it’s easy for future tuning to become confusing: is the visual change coming from the map, the scalar, or the generator constants?
Summary of changes
Summary
This PR upgrades Mars rendering and supporting tooling/tests across the orrery app.
Rendering pipeline
- Extends
BodySurfaceStylewith PBR map inputs:normalTexture,roughnessTexture, andnormalScale- optional procedural
detailNoiseblock
- Adds new appearance layers in
SceneModel.ts:kind: "atmosphere"(isAtmosphereAppearanceLayer())kind: "aerosol"(isAerosolAppearanceLayer())
- Updates
BodyMesh.tsto:- async-load and eagerly apply albedo/normal/roughness maps via
loadTextureCached() - centralize assignment via
applySurfaceMaps() - add deterministic
detailNoiseshader patching (with// tspice:*idempotency markers) - add reusable
createRimGlowShell()for atmosphere/aerosol shells - compose per-frame updates via
ensureSunDirWorldUpdate()
- async-load and eagerly apply albedo/normal/roughness maps via
Mars assets + registry
- Adds new Mars textures under
apps/orrery/public/textures/planets/:mars-viking-colorized-4k.jpgmars-mola-normal-2k.pngmars-roughness-proxy-2k.png
- Updates
BodyRegistry.tsMars entry to use the new textures plus tuned parameters (roughness,nightAlbedo,terminatorTwilight,detailNoise, and newatmosphere/aerosollayers). - Updates texture attribution docs in
public/textures/planets/README.md.
Tooling
- Adds generator script
apps/orrery/scripts/generate-mars-maps.js:- downloads/validates PDS MOLA DEM inputs
- generates normal + roughness-proxy PNGs
- uses an sRGB→linear conversion for albedo luminance
- Adds
apps/orrery/scripts/data/.gitignoreto keep downloaded PDS inputs out of git. - Adds dev deps
jpeg-jsandpngjs.
E2E stabilization + defaults
- Adds
window.__tspice_viewer__e2e.renderNTimes(n)and documents it. - Updates sun postprocessing Playwright spec to call
renderNTimes()before screenshots. - Lowers global default
SUN_LIGHT_INTENSITY_DEFAULTfrom3.5→2.1.
@CharlieHelps please prepare a detailed report with your findings on all of the issues mentioned. This should include:
|
|
Responding to #262 (comment) — here’s a breakdown of each concern with a concrete fix direction + impact analysis. 1)
|
Fixes #240
Summary: tuned surface/roughness; night-side darkening; subtle warm atmosphere layer; snapshot update.