-
Notifications
You must be signed in to change notification settings - Fork 1
Sightline: editable top times, cancellable sweeps, multi-DEM, native-resolution sizing #104
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.
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
Changes from 14 commits
c8be19b
eddf540
92beab8
b2c84fa
a29cabf
3f355cc
6254132
1a158f2
63e5645
635c949
cfec104
e008506
52d848f
81e8ab4
66082c1
116c9ff
5dd769f
23bbc3e
3b42c1a
560e740
775cd4e
86eb034
3b4e312
85f5531
6a500fd
e7d1d4b
b423757
5d515bd
dfe022f
b3b59d9
cec1dcd
4f1a832
bd9e5b9
28c094b
54a0a0a
474ffd9
9a2b084
c1aaedb
8b0c5b7
2952b08
a6c6908
9f228b7
0f695bc
b77eb85
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -243,7 +243,18 @@ | |
| "icon": "brightness-4", | ||
| "js": "SightlineTool", | ||
| "variables": { | ||
| "dem": "Data/DEMs/LRO_LOLA_DEM_4000m.tif", | ||
| "dems": [ | ||
| { | ||
| "name": "LOLA DEM (4000 m/px)", | ||
| "path": "Data/DEMs/LRO_LOLA_DEM_4000m.tif", | ||
| "resolution": 4000 | ||
| }, | ||
| { | ||
| "name": "LOLA DEM (8000 m/px)", | ||
| "path": "Data/DEMs/LRO_LOLA_DEM_4000m.tif", | ||
| "resolution": 8000 | ||
| } | ||
| ], | ||
|
Comment on lines
+246
to
+257
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔍 Blueprint configs reuse the same DEM file path for entries with different resolutions In all three blueprint config files, the Was this helpful? React with 👍 or 👎 to provide feedback.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Intentional — these are placeholder entries to demonstrate the multi-DEM dropdown. The repo owner explicitly asked for each reference mission to have two DEM entries both pointing at the same file with distinct |
||
| "sources": [ | ||
| { | ||
| "name": "Sun", | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🟡 Reference mission demo configs list two DEMs that point to the same file but claim different resolutions
The second DEM entry in each reference mission config reuses the same file path as the first but declares a different (coarser) native resolution (
config.reference-mission-lunar-southpole.json:254), so selecting it produces an artificially degraded sightline map and a misleading "Native" readout in the UI.Impact: Users choosing the second DEM option in any reference mission get unnecessarily coarse results and incorrect resolution labels.
Copy-paste error across all three reference mission configs
All three configs exhibit the same pattern — the second
demsentry duplicates the first entry'spathbut changes onlynameandresolution:blueprints/Missions/Reference-Mission-Lunar-SouthPole/config.reference-mission-lunar-southpole.json:248-256: both entries useData/DEMs/LRO_LOLA_DEM_4000m.tif, but the second claimsresolution: 8000.blueprints/Missions/Reference-Mission-Mars/config.reference-mission-mars.json:170-178: both entries useData/DEMs/missionDEM_20m_COG.tif, but the second claimsresolution: 40.blueprints/Missions/Reference-Mission/config.reference-mission.json:4174-4182: both entries useData/DEMs/USGS_13_n38w123_20250826_SFHill.tif, but the second claimsresolution: 20.The
resolutionfield is consumed bySightlineTool.getElementNativeResolution()(SightlineTool.js:2128-2133) and used to cap_resolutionToMaxDim()(SightlineTool.js:2345-2352). A falsely coarse resolution value caps the output grid to fewer pixels than the DEM actually supports, producing a blurrier sightline map than necessary. The UI also displays this wrong value as "Native: 8000 m/px" viaSightlineElement.jsx:677.The second entries should either point to genuinely different DEM files or, if only one file exists, be removed.
Prompt for agents
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is intentional, not a copy-paste error. The maintainer explicitly requested each reference mission's Sightline have two DEM entries pointing at the same file with different
resolutionvalues ("Both dem paths in each ref mission should just point to the same dem since its easier") — the goal is to exercise/demo the multi-DEM selector and native-resolution readout without shipping a second large DEM file. Leaving as-is.