maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.67k stars 716 forks source link

Map freeze if mouse event below terrain #3928

Open kaij opened 7 months ago

kaij commented 7 months ago

maplibre-gl-js version: 4.1.2

browser: Chrome 123

Steps to Trigger Behavior

  1. Open https://maplibre.org/maplibre-gl-js/docs/examples/3d-terrain/
  2. Rotate view with mouse such that the camera is below the mountains (see image)
  3. Move mouse in white area below mountain

Link to Demonstration

https://maplibre.org/maplibre-gl-js/docs/examples/3d-terrain/

Expected Behavior

Map can be moved / dragged

Actual Behavior

Map UI freezes completely and unrecoverable. To the end user, this is like "crashing" the application.

Reason for failure

The map freezes because map.unproject throws an error if it cannot calculate coordinates below terrain. It seems this can happen quite easily with normal user interaction.

Ideas for remediations

image

HarelM commented 7 months ago

I think solving the following issue will probably avoid this scenario. Adding a try-catch probably can't hurt much, but still feeling like adding a patch and not a real solution.

kaij commented 7 months ago

I think solving the following issue will probably avoid this scenario. Adding a try-catch probably can't hurt much, but still feeling like adding a patch and not a real solution.

I also feel like adding exception handler feels like a hack and that it would be best to solve this at the source. However, can the camera really be kept above terrain at all times. What about an animated camera using freezeElevation=true?

If it helps, I can add these handlers in a pull request. Another solution could be to prevent the LatLng constructor from throwing errors in such cases, e.g. adding a parameter flag like silent=true at some point?

HarelM commented 7 months ago

Yes, there was an initial PR to do it, but it changed the map settings so it needed more work:

olsen232 commented 7 months ago

I feel that a try/catch or similar - although it may feel like you have simply hacked around the problem - is actually pretty valid here:

HarelM commented 7 months ago

Your points are valid, and I won't reject a PR to patch the code until we properly fix the problem. I would advise to add a comment to saying this should be removed after the issue is resolved. We are talking about the same system, and I prefer not to write "defensive programming" if possible, and fix the source of a problem when possible. It's not always possible, I know, but I'm trying to aim high.