Fix vector map route offset and marker sync race condition#6731
Fix vector map route offset and marker sync race condition#6731SinghSwayam wants to merge 2 commits intoopenstreetmap:masterfrom
Conversation
|
To echo what @gravitystorm said earlier in #6712 (comment) adding fixed delays is rarely a good solution and that goes even more for the main code than it does for the tests. I'm sure those delays have worked for you, on your machine today, for however many times you've tried it but I'm equally sure there will be circumstances where they don't work. What is needed is a way to actually synchronise things properly rather than adding delays to try and make things work. |
|
@tomhughes Thanks for the feedback. I've updated the PR to replace the |
tomhughes
left a comment
There was a problem hiding this comment.
I wonder if this is something that should be more general? That OSM.loadSidebarContent should be listening for the transition it starts to finish and then poking the map and firing an event that the routing code can listen to?
Oddly I can't reproduce the behaviour that we see with routing with other things like object browse URLs than also open the sidebar and display overlays on the map but I don't know why!
| map.eachLayer((layer) => { | ||
| if (layer._glMap) { | ||
| layer._glMap.resize(); | ||
| } | ||
| }); | ||
| map.invalidateSize({ animate: false }); |
There was a problem hiding this comment.
So the maplibre-gl plugin for leaflet doesn't respond correctly to size invalidation I assume, hence the need to manually poke those layers? Has that been reported to maplibre-gl-leaflet upstream?
There was a problem hiding this comment.
Re: Upstream Issue
You are spot on. Ideally, the plugin should listen for invalidateSize and handle the context resize automatically. I saw that PR #76 (Fix resize issue of vector maps) was merged upstream, but it clearly didn't cover this specific race condition involving CSS transitions. I haven't found an open issue tracking this specific "transition desync" scenario, so I can certainly open one to track it.
Re: Generalization
That sounds like a much more robust approach. Moving the transitionend listener into OSM.loadSidebarContent (or the sidebar controller) would solve this class of race condition for all sidebar interactions, not just routing.
Would you prefer I update this PR to refactor OSM.loadSidebarContent to fire a "sidebarReady" event that we can listen to? Or should we keep this PR focused on the routing fix and handle the architecture change separately?
There was a problem hiding this comment.
@SinghSwayam are you reviewing and verifying LLM content before posting it as comments or PR code?
There was a problem hiding this comment.
@SinghSwayam are you reviewing and verifying LLM content before posting it as comments or PR code?
@matkoniecz I use tools to assist with drafting my comments, but I verify the content myself. The code changes here were manually written and tested I experienced locally.
Fixes #6684
The Issue
When using Vector Tile layers (e.g., Shortbread, MapTiler), reloading the page with an active route caused the route line to appear significantly offset from the map features.
The Cause
The issue is caused by a race condition between the Sidebar CSS animation (approx. 300ms) and the WebGL Vector Tile renderer. When the sidebar opens, it resizes the map container. While Leaflet detects this, the internal MapLibre/GL instance often fails to update its viewport coordinates in time, causing the SVG route layer and the WebGL background to desynchronize.
The Fix
This PR modifies
getRouteto ensure strict synchronization without relying on fragile fixed timeouts:setTimeoutdelays with aPromisethat listens for thetransitionendevent on the map container._glMap.resize()on any vector layers to ensure the WebGL context matches the Leaflet coordinate system.This approach ensures the map is stable before drawing, regardless of the user's machine speed or animation timing.
Screen Recording
Before :
OpenStreetMap.before.mp4
After :
OpenStreetMap.after.mp4