Skip to content
New issue

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

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

Already on GitHub? # to your account

3D map freezes when camera is adjusted against map bounds (in dev environment only (?)) #4537

Closed
larsmaxfield opened this issue Aug 10, 2024 · 4 comments
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed

Comments

@larsmaxfield
Copy link
Contributor

larsmaxfield commented Aug 10, 2024

maplibre-gl-js version: 4.5.1

browser: Edge 126.0.2592.102 (64-bit), Firefox 128.0 (64-bit)

Development environment on Windows 10 WSL

Steps to Trigger Behavior

  1. In a development environment, open a 3D terrain map (npm run start; localhost:XXXX/test/examples/3d-terrain.html).
  2. Enable 3D and pitch the map.
  3. Pan to the map's latitude bounds.
  4. Adjust the camera (pan, zoom, pitch, rotate) against the bounds.
  5. Map freezes due to uncaught error in CanonicalTileID with zxy out of bounds.

map-freeze

uncaught-error

Link to Demonstration

I can't seem to trigger this in the online docs — only in a local development environment.

Expected Behavior

The map should not freeze when adjusting the camera against the map bounds.

Actual Behavior

The map freezes.

@larsmaxfield
Copy link
Contributor Author

larsmaxfield commented Aug 10, 2024

The issue is occurring because getElevationForLngLatZoom is making requests for OverscaledTileID with LngLat outside the map bounds:

getElevationForLngLatZoom(lnglat: LngLat, zoom: number) {
const {tileID, mercatorX, mercatorY} = this._getOverscaledTileIDFromLngLatZoom(lnglat, zoom);
return this.getElevation(tileID, mercatorX % EXTENT, mercatorY % EXTENT, EXTENT);
}

This can be fixed by first checking if the LngLat and zoom are in bounds before the request to the OverscaledTileID. If it's out of bounds, the function returns an elevation of 0:

getElevationForLngLatZoom(lnglat: LngLat, zoom: number) {
const {x, y} = MercatorCoordinate.fromLngLat(lnglat.wrap());
if (y < 0 || y >= 1 || zoom < 0 || zoom > 25 || x < 0 || x >= 1) return 0;
const {tileID, mercatorX, mercatorY} = this._getOverscaledTileIDFromLngLatZoom(lnglat, zoom);
return this.getElevation(tileID, mercatorX % EXTENT, mercatorY % EXTENT, EXTENT);
}

The function getDEMElevation employs a similar check and return of 0 for invalid xy tile values, so adding a check in getElevationForLngLatZoom seems like an appropriate fix to me.

@larsmaxfield larsmaxfield changed the title 3D map freezes when panned/zoomed against map bounds (in dev environment only (?)) 3D map freezes when camera is moved against map bounds (in dev environment only (?)) Aug 10, 2024
@larsmaxfield larsmaxfield changed the title 3D map freezes when camera is moved against map bounds (in dev environment only (?)) 3D map freezes when camera is adjusted against map bounds (in dev environment only (?)) Aug 10, 2024
@HarelM
Copy link
Collaborator

HarelM commented Aug 11, 2024

Feel free to submit a PR.
If there's a check already, I would prefer to have it somewhere centralized (utils.ts maybe) so that changing it once will work for all the relevant places.
Especially if we ever decide to change the max zoom or min zoom.

@larsmaxfield
Copy link
Contributor Author

larsmaxfield commented Aug 11, 2024

Good point.

I centralized the new constants MAX_TILE_ZOOM = 25 and MIN_TILE_ZOOM = 0 in util.ts. I found a couple instances where those were hardcoded and replaced them accordingly.

However, I kept the bounds check functions in tile_id.ts. The check isInBoundsForZoomLngLat (which is used to fix this issue) relies on LngLat. If I put that in util.ts, it caused a circular dependency.

*I moved the bounds checks isInBoundsForZoomXY and isInBoundsForZoomLngLat to a new src/util/world_bounds.ts.

@HarelM HarelM closed this as completed in c550d85 Aug 12, 2024
@larsmaxfield
Copy link
Contributor Author

Thanks for reviewing!

alexandresoro pushed a commit to alexandresoro/ouca that referenced this issue Sep 22, 2024
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [maplibre-gl](https://maplibre.org/) ([source](https://github.com/maplibre/maplibre-gl-js)) | dependencies | minor | [`4.5.2` -> `4.7.0`](https://renovatebot.com/diffs/npm/maplibre-gl/4.5.2/4.7.0) |

---

### Release Notes

<details>
<summary>maplibre/maplibre-gl-js (maplibre-gl)</summary>

### [`v4.7.0`](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#470)

[Compare Source](maplibre/maplibre-gl-js@v4.6.0...v4.7.0)

##### ✨ Features and improvements

-   Support multiple layers in `map.on`, `map.once` and `map.off` methods ([#&#8203;4570](maplibre/maplibre-gl-js#4570))
-   Ensure GeoJSON cluster sources emit a console warning if `maxzoom` is less than or equal to `clusterMaxZoom` since in this case you may see unexpected results. ([#&#8203;4604](maplibre/maplibre-gl-js#4604))

##### 🐞 Bug fixes

-   Heatmap Fix for 3D terrain ([#&#8203;4571](maplibre/maplibre-gl-js#4571))
-   Fix Map#off to not remove listener with layer(s) registered with Map#once ([#&#8203;4592](maplibre/maplibre-gl-js#4592))
-   Improve types a bit for `addSource` and `getSource` ([#&#8203;4616](maplibre/maplibre-gl-js#4616))
-   Fix the color near the horizon when terrain is enabled without any sky ([#&#8203;4607](maplibre/maplibre-gl-js#4607))
-   Fix bug where `fitBounds` and `cameraForBounds` would not display across the 180th meridian (antimeridian)
-   Fix white flickering on map resize ([#&#8203;4158](maplibre/maplibre-gl-js#4158))
-   Fixed a performance regression related to symbol placement ([#&#8203;4599](maplibre/maplibre-gl-js#4599))
-   Fix a bug where cloning a Transform instance didn't include the `lngRange`. This caused a bug where
    using `transformCameraUpdate` caused the `maxBounds` to stop working just for east/west bounds. ([#&#8203;4625](maplibre/maplibre-gl-js#4625))

### [`v4.6.0`](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#460)

[Compare Source](maplibre/maplibre-gl-js@v4.5.2...v4.6.0)

##### ✨ Features and improvements

-   Prefer local glyph rendering for all CJKV characters, not just those in the CJK Unified Ideographs, Hiragana, Katakana, and Hangul Syllables blocks. ([#&#8203;4560](maplibre/maplibre-gl-js#4560)))

##### 🐞 Bug fixes

-   Fix right-to-left layout of labels that contain characters in the Arabic Extended-B code block. ([#&#8203;4536](maplibre/maplibre-gl-js#4536))
-   Fix 3D map freezing when camera is adjusted against map bounds. ([#&#8203;4537](maplibre/maplibre-gl-js#4537))
-   Fix `getStyle()` to return a clone so the object cannot be internally changed ([#&#8203;4488](maplibre/maplibre-gl-js#4488))
-   Fix issues with setting sky to `undefined` ([#&#8203;4587](maplibre/maplibre-gl-js#4587)))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC40Ny4wIiwidXBkYXRlZEluVmVyIjoiMzguODYuMCIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiZGVwZW5kZW5jaWVzIl19-->

Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/52
Reviewed-by: Alexandre Soro <code@soro.dev>
Co-authored-by: renovate <renovate@git.tristess.app>
Co-committed-by: renovate <renovate@git.tristess.app>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed
Projects
None yet
Development

No branches or pull requests

2 participants