Rebuild the independent globe from the screenshot reference set - #12
Rebuild the independent globe from the screenshot reference set#12Occumed79 wants to merge 3 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Occumed79 has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughThe runtime style is updated for photo-reference rendering with globe projection, calibrated layer styling, metadata markers, and no Mapbox resource references. A new validation script checks these properties and is added to the runtime validation command. ChangesPhoto-reference runtime
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54fd29b173
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (layer.type === 'background') { | ||
| paint['background-color'] = palette.waterLow; | ||
| paint['background-opacity'] = 1; | ||
| continue; |
There was a problem hiding this comment.
The generated land layer is a background layer, so this branch colors it ocean blue and then continue prevents the later id === 'land' block from assigning the intended pale land properties. After prepare:style, check-globe-parity.mjs rejects the changed background color and the newly added check-photo-reference.mjs also fails because fill-color was never set, making the runtime validation in .github/workflows/validate.yml fail on every build.
Useful? React with 👍 / 👎.
| 6.5, | ||
| 0 | ||
| ], | ||
| 'raster-saturation': 0.32, |
There was a problem hiding this comment.
This calibration lowers raster-saturation to 0.32, but the existing check-viewer-quality.mjs requires the generated relief saturation to be at least 0.55. Running that check after prepare:style deterministically reports “The low-zoom terrain palette is too desaturated,” so the repository's check:runtime and CI build cannot pass with this value.
Useful? React with 👍 / 👎.
| 12, | ||
| palette.waterHigh | ||
| ]; | ||
| paint['fill-opacity'] = 1; |
There was a problem hiding this comment.
Replacing the water opacity expression with scalar 1 makes the water fully opaque at low zoom, hiding the bathymetry that the relief layer is meant to expose. It also unconditionally fails the existing check-cartography-parity.mjs, which requires water.paint['fill-opacity'] to remain a zoom expression, so check:runtime fails after this calibration pass.
Useful? React with 👍 / 👎.
![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
🧹 Nitpick comments (5)
scripts/apply-photo-reference.mjs (4)
200-200: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant condition.
id === 'national-park'is already covered byid.includes('national-park').🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/apply-photo-reference.mjs` at line 200, Remove the redundant id === 'national-park' check from the conditional, leaving the existing id.includes('national-park') check to preserve the same behavior.
99-156: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWholesale
layer.paintreplacement discards upstream calibration.Lines 101 and 133 replace the entire paint object, so anything set by the earlier scripts in the
prepare:stylechain (notablyapply-globe-parity.mjs) on these two layers is dropped. Also makes thepaintbinding from Line 91 dead for these branches. If the reset is intentional, a short comment would prevent future confusion; otherwise merge into the existing object.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/apply-photo-reference.mjs` around lines 99 - 156, Preserve upstream paint calibration in the occumed-shaded-relief and occumed-hillshade branches by merging the new properties into each layer’s existing paint object instead of replacing it wholesale. Update the paint binding introduced before these branches so it remains used, while retaining all current layer-specific overrides and behavior.
300-304: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueLoosen-proof the water-label regex. Unanchored substrings match unintended ids (
seainseamark,bayinbayou,riverinriverside-road-label), and this block unconditionally overrides the label colors assigned just above.♻️ Tighter match
- if (/water|ocean|sea|bay|strait|river/.test(id)) { + if (/(^|[-_])(water|ocean|sea|bay|strait|river)([-_]|$)/.test(id)) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/apply-photo-reference.mjs` around lines 300 - 304, Update the water-label condition using a token-aware, anchored regex so only ids representing water features match, excluding substrings such as “seamark,” “bayou,” and “riverside-road-label.” In the same block, preserve label colors already assigned above by applying the water palette only when the relevant color has not already been set.
158-162: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winId-matched branches write
fill-*without the type guard their source-layer counterparts use. Both sites match purely on layer id and then set fill paint properties, so any non-fill layer namedlandorwaterreceives invalid paint and fails MapLibre style validation. The source-layer disjuncts in the same conditions already gate onlayer.type === 'fill', confirming the intent.
scripts/apply-photo-reference.mjs#L158-L162: wrap thefill-color/fill-opacitywrites inif (layer.type === 'fill'), matching the sibling branches at Lines 165, 201, and 219.scripts/apply-photo-reference.mjs#L254-L268: hoist the type check out of the second disjunct so it applies to theid === 'water'case too, e.g.if (layer.type === 'fill' && (id === 'water' || isSourceLayer(layer, 'water'))).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/apply-photo-reference.mjs` around lines 158 - 162, Guard the id-matched land branch in scripts/apply-photo-reference.mjs#L158-L162 with layer.type === 'fill' before writing fill-color and fill-opacity. Also update the water condition at scripts/apply-photo-reference.mjs#L254-L268 so the type check applies to both id === 'water' and isSourceLayer(layer, 'water') cases, preserving the existing fill paint behavior only for fill layers.scripts/check-photo-reference.mjs (1)
26-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the palette into a shared module instead of duplicating hex literals.
These values are copies of the
paletteobject inapply-photo-reference.mjs. Retuning a color there leaves this check asserting a stale value, and the failure message points at "missing" styling rather than at the real drift. A smallscripts/photo-reference-palette.mjsimported by both keeps them in lockstep.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check-photo-reference.mjs` around lines 26 - 41, Extract the shared color values from the palette object in apply-photo-reference.mjs into a scripts/photo-reference-palette.mjs module, then import and reuse those palette symbols in both apply-photo-reference.mjs and the assertions in check-photo-reference.mjs. Replace the duplicated hex literals with references to the shared palette while preserving the existing checks and failure behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/apply-photo-reference.mjs`:
- Around line 278-298: Update the text-font assignments in the label handling
branches of apply-photo-reference.mjs to use font stacks available from the
OpenFreeMap glyphs endpoint, preserving the existing semibold versus regular
label intent. Alternatively, extend check-runtime.mjs to validate these
rewritten stacks and fail when the endpoint cannot serve them; ensure every
stack emitted by the country-label, state/continent-label,
settlement-major-label, and settlement-minor/subdivision-label branches is
covered.
In `@scripts/check-photo-reference.mjs`:
- Around line 53-54: Update the runtime sprite and glyph assertions in the
check-photo-reference script so each rejects both Mapbox URL patterns, mapbox://
and api.mapbox.com. Apply the checks to the serialized sprite value so
array-form sprite configurations are covered, while preserving the existing
no-Mapbox validation for runtime.glyphs.
---
Nitpick comments:
In `@scripts/apply-photo-reference.mjs`:
- Line 200: Remove the redundant id === 'national-park' check from the
conditional, leaving the existing id.includes('national-park') check to preserve
the same behavior.
- Around line 99-156: Preserve upstream paint calibration in the
occumed-shaded-relief and occumed-hillshade branches by merging the new
properties into each layer’s existing paint object instead of replacing it
wholesale. Update the paint binding introduced before these branches so it
remains used, while retaining all current layer-specific overrides and behavior.
- Around line 300-304: Update the water-label condition using a token-aware,
anchored regex so only ids representing water features match, excluding
substrings such as “seamark,” “bayou,” and “riverside-road-label.” In the same
block, preserve label colors already assigned above by applying the water
palette only when the relevant color has not already been set.
- Around line 158-162: Guard the id-matched land branch in
scripts/apply-photo-reference.mjs#L158-L162 with layer.type === 'fill' before
writing fill-color and fill-opacity. Also update the water condition at
scripts/apply-photo-reference.mjs#L254-L268 so the type check applies to both id
=== 'water' and isSourceLayer(layer, 'water') cases, preserving the existing
fill paint behavior only for fill layers.
In `@scripts/check-photo-reference.mjs`:
- Around line 26-41: Extract the shared color values from the palette object in
apply-photo-reference.mjs into a scripts/photo-reference-palette.mjs module,
then import and reuse those palette symbols in both apply-photo-reference.mjs
and the assertions in check-photo-reference.mjs. Replace the duplicated hex
literals with references to the shared palette while preserving the existing
checks and failure behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 05ce425c-2d64-41cd-897c-517c9bba5eea
📒 Files selected for processing (3)
package.jsonscripts/apply-photo-reference.mjsscripts/check-photo-reference.mjs
| if (id.includes('country-label')) { | ||
| paint['text-color'] = palette.label; | ||
| paint['text-halo-color'] = palette.halo; | ||
| paint['text-halo-width'] = 1.15; | ||
| layout['text-font'] = ['Open Sans Semibold', 'Noto Sans Regular']; | ||
| } else if (id.includes('state-label') || id.includes('continent-label')) { | ||
| paint['text-color'] = palette.mutedLabel; | ||
| paint['text-halo-color'] = 'rgba(244, 243, 233, 0.72)'; | ||
| paint['text-halo-width'] = 0.9; | ||
| layout['text-font'] = ['Open Sans Regular', 'Noto Sans Regular']; | ||
| } else if (id.includes('settlement-major-label')) { | ||
| paint['text-color'] = palette.label; | ||
| paint['text-halo-color'] = palette.halo; | ||
| paint['text-halo-width'] = 1.1; | ||
| layout['text-font'] = ['Open Sans Semibold', 'Noto Sans Regular']; | ||
| } else if (id.includes('settlement-minor-label') || id.includes('settlement-subdivision-label')) { | ||
| paint['text-color'] = '#4d555b'; | ||
| paint['text-halo-color'] = 'rgba(244, 243, 233, 0.86)'; | ||
| paint['text-halo-width'] = 0.95; | ||
| layout['text-font'] = ['Open Sans Regular', 'Noto Sans Regular']; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# What font stacks does the normalizer allow, and what glyph endpoint is configured?
fd -t f 'normalize-runtime-fonts.mjs' -E node_modules --exec cat -n {}
rg -n '"glyphs"' -g '!node_modules' public scripts
rg -n 'text-font' -g '!node_modules' scripts public | head -40Repository: Occumed79/Map
Length of output: 3615
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Tracked files matching glyph/style references:"
git ls-files | rg '(^package\.json$|scripts/apply-photo-reference\.mjs$|scripts/normalize-runtime-fonts\.mjs$|scripts/build-runtime-style\.mjs$|scripts/check-runtime\.mjs$|public/style|scripts/.+\.mjs$)' | sed -n '1,120p'
echo
echo "Package scripts:"
if [ -f package.json ]; then jq '.scripts' package.json; fi
rg -n '"glyphs"|glyphs|fonts|fontstack|text-font|apply-photo-reference|normalize-runtime-fonts|check-runtime' -g '!node_modules' -g '!dist' -g '!build' package.json scripts public 2>/dev/null | sed -n '1,200p'
echo
echo "Relevant apply-photo-reference section:"
sed -n '260,310p' scripts/apply-photo-reference.mjs 2>/dev/null || trueRepository: Occumed79/Map
Length of output: 4305
🌐 Web query:
OpenMapTiles font files Open Sans Semibold Noto Sans Regular tiles.openfreemap.org fonts
💡 Result:
OpenFreeMap provides vector tiles and font files (glyphs) that are compatible with mapping libraries like MapLibre GL JS [1][2]. Regarding the specific font files Open Sans Semibold and Noto Sans Regular: 1. Compatibility and Usage: OpenFreeMap utilizes these fonts in its style definitions [3]. When using OpenFreeMap styles in your own applications, it is recommended to explicitly set the text-font property to "Noto Sans Regular" (or the appropriate variant) within your style JSON layers [1][4]. 2. Technical Context: MapLibre and other similar libraries often default to a font stack containing "Open Sans Regular" and "Arial Unicode MS Regular" [1][4]. Since OpenFreeMap may not host "Arial Unicode MS Regular," applications that do not explicitly override this default may encounter 404 errors when attempting to fetch these files [1][4]. 3. Resource Availability: The font files are served in PBF (Protocol Buffer) format, which is standard for MapLibre/Mapbox GL style specifications [5][6]. The OpenFreeMap project maintains a repository of its styles and font configurations, which indicates that a variety of font variants—including multiple weights of Open Sans and Noto Sans—are utilized across its different map styles [3][7]. If you are encountering issues where labels do not appear, ensure that the glyphs URL in your style JSON is correctly configured to point to the OpenFreeMap font endpoint (typically https://tiles.openfreemap.org/fonts/{fontstack}/{range}.pbf) and that your text-font property matches the fonts available on that server [1][5].
Citations:
- 1: Missing PBF font? hyperknot/openfreemap#10
- 2: https://openfreemap.org/quick_start/
- 3: hyperknot/openfreemap-styles@d63c2c1
- 4: No layers when using OpenFreeMap traccar/traccar-web#1281
- 5: Additional fonts hyperknot/openfreemap#53
- 6: https://github.com/maplibre/demotiles/tree/gh-pages/font
- 7: https://github.com/tordans/openfreemap-styles
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "build-runtime-style relevant sections:"
sed -n '1,25p;260,315p;380,430p' scripts/build-runtime-style.mjs
echo
echo "normalize-runtime-fonts relevant sections:"
sed -n '1,75p' scripts/normalize-runtime-fonts.mjs
echo
echo "apply-photo-reference relevant sections:"
sed -n '260,330p' scripts/apply-photo-reference.mjs
echo
echo "check-photo-reference relevant sections:"
sed -n '1,130p' scripts/check-photo-reference.mjs
echo
echo "check-runtime relevant sections:"
sed -n '20,110p' scripts/check-runtime.mjsRepository: Occumed79/Map
Length of output: 16275
Align label font stacks with the OpenFreeMap glyphs endpoint.
apply-photo-reference.mjs runs after font normalization and rewrites text-font to Open Sans Semibold / Open Sans Regular / Noto Sans Regular; check-runtime.mjs only catches DIN Pro / Arial Unicode MS stacks. Use fontstacks available at https://tiles.openfreemap.org/fonts/{fontstack}/{range}.pbf, or add a check that fails when these stacks cannot be served.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/apply-photo-reference.mjs` around lines 278 - 298, Update the
text-font assignments in the label handling branches of
apply-photo-reference.mjs to use font stacks available from the OpenFreeMap
glyphs endpoint, preserving the existing semibold versus regular label intent.
Alternatively, extend check-runtime.mjs to validate these rewritten stacks and
fail when the endpoint cannot serve them; ensure every stack emitted by the
country-label, state/continent-label, settlement-major-label, and
settlement-minor/subdivision-label branches is covered.
| assert(!/mapbox:\/\//i.test(runtime.sprite || ''), 'Runtime sprite must not use Mapbox.'); | ||
| assert(!/api\.mapbox\.com/i.test(runtime.glyphs || ''), 'Runtime glyphs must not use Mapbox.'); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sprite and glyph Mapbox checks are asymmetric — each misses one pattern.
The sources loop tests both mapbox:// and api.mapbox.com, but Line 53 only tests mapbox:// for the sprite and Line 54 only tests api.mapbox.com for glyphs. So a sprite at https://api.mapbox.com/... or glyphs at mapbox://fonts/... both pass, defeating the no-Mapbox guarantee this script exists to enforce.
🐛 Proposed fix
-assert(!/mapbox:\/\//i.test(runtime.sprite || ''), 'Runtime sprite must not use Mapbox.');
-assert(!/api\.mapbox\.com/i.test(runtime.glyphs || ''), 'Runtime glyphs must not use Mapbox.');
+const MAPBOX_REF = /mapbox:\/\/|api\.mapbox\.com/i;
+assert(!MAPBOX_REF.test(JSON.stringify(runtime.sprite ?? '')), 'Runtime sprite must not use Mapbox.');
+assert(!MAPBOX_REF.test(JSON.stringify(runtime.glyphs ?? '')), 'Runtime glyphs must not use Mapbox.');JSON.stringify also covers the array form of sprite ([{ id, url }, …]).
📝 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.
| assert(!/mapbox:\/\//i.test(runtime.sprite || ''), 'Runtime sprite must not use Mapbox.'); | |
| assert(!/api\.mapbox\.com/i.test(runtime.glyphs || ''), 'Runtime glyphs must not use Mapbox.'); | |
| const MAPBOX_REF = /mapbox:\/\/|api\.mapbox\.com/i; | |
| assert(!MAPBOX_REF.test(JSON.stringify(runtime.sprite ?? '')), 'Runtime sprite must not use Mapbox.'); | |
| assert(!MAPBOX_REF.test(JSON.stringify(runtime.glyphs ?? '')), 'Runtime glyphs must not use Mapbox.'); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/check-photo-reference.mjs` around lines 53 - 54, Update the runtime
sprite and glyph assertions in the check-photo-reference script so each rejects
both Mapbox URL patterns, mapbox:// and api.mapbox.com. Apply the checks to the
serialized sprite value so array-form sprite configurations are covered, while
preserving the existing no-Mapbox validation for runtime.glyphs.
1 participant
Controlled screenshot-based reconstruction
This replaces the current Mapbox-dependent main build with the last independent MapLibre baseline and adds a deliberate photo-calibration pass.
Uses the supplied screenshots as the visual specification
Independent runtime
Regression protection
Adds screenshot-reference checks for globe atmosphere, land opacity, water colors, landcover colors, bathymetry, hillshade, and the absence of Mapbox runtime endpoints.
The original root style remains the layer-order and zoom-rule blueprint; the active runtime is rebuilt against the open schema and calibrated to the supplied photographs.
Summary by CodeRabbit
New Features
Bug Fixes
Chores