OpenHistoricalMap / issues

File your issues here, regardless of repo until we get all our repos squared away; we don't want to miss anything.
Creative Commons Zero v1.0 Universal
19 stars 1 forks source link

Amenity points missing from Historic style; amenity areas labeled repetitively in Railway style #903

Closed 1ec5 closed 2 weeks ago

1ec5 commented 3 weeks ago

As reported on the forum, the Historical style is no longer labeling amenity=* points, only the centroids of amenity=* areas.

Example

At the following location, the Historic style shows labels for fewer amenities compared to the Railway style, but the Railway style shows multiple labels for the same amenity, which was the issue tracked in #543:

Historical Railway
Bishop Building Cocoa Bites, Bishop’s Quarter, Tano Bistro, Ramsey’s Trailside, Hops and Berry Taproom, The Works
Loveland City Hall Loveland City Hall, Loveland City Hall, Loveland City Hall, Loveland City Hall, Loveland City Hall, Loveland City Hall

Diagnosis

Analogous symbol layers in the Historical and Railway layers are rendering inconsistent source layers:

Layer Historical Railway
points_of_interest_fromareasz14 amenity_areas_centroids amenity_areas
points_of_interest_fromareas amenity_areas_centroids amenity_areas
points_of_interest_amenity_14 amenity_points_centroids amenity_points
points_of_interest_amenity amenity_points_centroids amenity_points

Amenities such as Bishop’s Quarter are mapped as points, so they appear in the amenity_points layer, but others like Loveland City Hall are mapped as areas, so they appear in the amenity_areas and amenity_areas_centroids layers:

amenity_points amenity_areas amenity_areas_centroids

This regression might have been introduced with the fix for #543. As far as I can tell, the Historical style has too many layers referring to amenity_areas_centroids and not enough referring to amenity_points, while the Railway style has too many layers referring to amenity_areas and not enough referring to amenity_areas_centroids.

1ec5 commented 2 weeks ago

I tried locally updating the two layers in the Historical style to use amenity_points instead of amenity_points_centroids using runtime styling, and it seemed to fix the issue.

vknoppkewetzel commented 2 weeks ago

This is now resolved https://openhistoricalmap.github.io/mbgl-timeslider/demo/#15.673/51.51237/-0.10083/1850,-4000-2023 image Note, I didn't delete the centroids layer, like other "centroids" updates, kept both. I am not sure why this instance didn't have 2 copies (And that it was incorrectly labeled ie, it should have had centroids label in name, since the above was pulling from centroids). But possible I missed somehow. Anyway, it's good to go now!

@Rub21 you can deploy the latest.

Rub21 commented 2 weeks ago

Nice, I ma going to use the lates gitsha 14b50d05583dddeb426fa01a8bc375fc8f2258d3 from https://github.com/OpenHistoricalMap/map-styles/tree/staging

Rub21 commented 2 weeks ago

The changes has been deployed in production https://github.com/OpenHistoricalMap/ohm-deploy/pull/381

1ec5 commented 2 weeks ago

I missed this in the comments above, but the time slider is broken (even in the old demo site), because the styles are now using expressions in their filters. #855 tracks updating the time slider. Any style deployments are blocked until then. If anyone needs to view the fix for this bug, they’ll have to do it in the embed site, for example: https://embed.openhistoricalmap.org/#map=15.67/51.51237/-0.10083&date=1850&layer=O_staging

vknoppkewetzel commented 2 weeks ago

@1ec5 ah right shoot. Should we revert back to pre-expressions deployment, or fine to have styles with broken timeslider at the moment, until that is updated? I just saw the slack convo and see it was rolled back ✅