search

nptscot / nptscot.github.io

Network Planning Tool for Scotland: front end.
https://www.npt.scot
GNU Affero General Public License v3.0
7 stars 5 forks source link

Placename switch broken #174

Closed mem48 closed 3 months ago

mem48 commented 5 months ago

@mvl22 I spotted this in https://github.com/PlaceBasedCarbonCalculator/PlaceBasedCarbonCalculator.github.io and was going crazy trying to work out what I'd done wrong. But it turns out this bug is in NPT too.

Observed bugs

  1. No place names for any base map except the default grey base map
  2. If you switch to another base map and then back to the default the placenames disappear. If you then toggle the place names switch the console generates loads of errors like this:
Error: Cannot style non-existing layer "country names".
    setLayoutProperty https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    setLayoutProperty https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    placenames https://nptscot.github.io/src/nptui.js:554
    placenames https://nptscot.github.io/src/nptui.js:553
    placenames https://nptscot.github.io/src/nptui.js:551
    promise callback*placenames https://nptscot.github.io/src/nptui.js:541
    createMap https://nptscot.github.io/src/nptui.js:295
    fire https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:31
    _render https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    _frame https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    frame https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:31
    triggerRepaint https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    _update https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    Map https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    fire https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:31
    fire https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:31
    fire https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:31
    fire https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:31
    _tileLoaded https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    a https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    processTask https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:31
    receive https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:31
    EventListener.handleEvent* https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:31
    M https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    se https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    _updateStyle https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    setStyle https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    <anonymous> https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js:35
    createMap https://nptscot.github.io/src/nptui.js:246
    initialise https://nptscot.github.io/src/nptui.js:76
    <anonymous> https://nptscot.github.io/#/#8/55.953/-3.138:75
    EventListener.handleEvent* https://nptscot.github.io/#/#8/55.953/-3.138:74
[maplibre-gl.js:31:10592](https://unpkg.com/maplibre-gl@3.6.2/dist/maplibre-gl.js)

The issue seems to be related to 

// Add placenames support
map.once('idle', function () {
   capUi.placenames(map);
});

Which only triggers during map creation.

A possibly related (but less important) bug is that toggling the antialias switch changed the base map back to the default.

mvl22 commented 5 months ago

Sorry about that - I'll look at this shortly. Clearly there's been a regression and I should be able to find where it is.

mem48 commented 4 months ago

@mvl22 have you made any progress on this?

mvl22 commented 4 months ago

Sorry, this is a priority for this weekend. I had an initial look recently but then ran into other things I had to sort out. Will update shortly.

mvl22 commented 4 months ago

The placenames change is fixed in 1995b936ea426c452a731a2b741f4f579ed46746. The layers were being lost on change, but seemingly the source also sometimes goes missing, which feels like a bug to me.

mvl22 commented 4 months ago

A possibly related (but less important) bug is that toggling the antialias switch changed the base map back to the default.

Did that ever work before? I can't see how, as there wasn't cookie memory, and the basemap wasn't in the URL state either.

We should arguably add this, now we have control of the URL hash.

Robinlovelace commented 3 months ago

@mem44 and @mvl22 is this fixed now, I don't fully understand, would be good to get an update.

Robinlovelace commented 3 months ago

In conversation with @wangzhao0217 I have tested it for various places and it works fine:

image

Robinlovelace commented 3 months ago

Also have discovered that the placenames are filtered to UK or GB, not Scotland. Will open up a separate issue.

mvl22 commented 3 months ago

I believe this is all resolved. Please do close.