Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

Add asynchronous loading and removing functionality #38

Merged

Conversation

nertc
Copy link
Contributor

@nertc nertc commented Jul 24, 2024

This PR addresses "Map Data checkbox: perhaps use toggle slider instead" issue mentioned in the openstreetmap/openstreetmap-website#4931

Several changes were made to the data loading functionality:

  1. Rendering process will be asynchronous. Therefore, there will be no more browser or page freezes (see "Render Video").
  2. Removing process will be also asynchronous and there won't be any freezes (see "Remove Video").
  • While objects are being rendered, removing won't be triggered until the rendering process is done. After the process, application will check, and it will start removing rendered objects if necessary. The same applies vice versa.

Connected to the PR openstreetmap/openstreetmap-website#5009
Fixes openstreetmap/openstreetmap-website#4931

Videos:

Render Video

OpenStreetMap.and.28.more.pages.-.Work.-.Microsoft.Edge.2024-07-20.16-55-01.mp4

Remove Video

OpenStreetMap.and.28.more.pages.-.Work.-.Microsoft.Edge.2024-07-20.16-56-42.mp4

@nertc
Copy link
Contributor Author

nertc commented Sep 3, 2024

This is prerequisite PR for openstreetmap/openstreetmap-website#5009. If this PR is not merged, openstreetmap-website PR can't be merged.

Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm quite surprised that it's adding and removing the features that is the slow part rather than parsing the XML to get them details but I guess it's because each feature is a separate layer... Seems it's really leaflet that needs to be faster and this is a somewhat ugly workaround :-(

leaflet-osm.js Outdated
@@ -122,7 +123,12 @@ L.OSM.DataLayer = L.FeatureGroup.extend({
}
}

layer.addTo(this);
if (this.options.asynchronous) {
setTimeout(() => layer.addTo(this));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Incorrect indentation here...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation was updated

@nertc
Copy link
Contributor Author

nertc commented Sep 3, 2024

@tomhughes Thank you for review. I'll change indentation. Idea of this PR is to make rendering asynchronous as now it is freezing app and browser. Currently leaflet takes long time for rendering and deleting SVG elements (calculations done on OSM side are quite fast and doesn't take any noticeable time). To increase speed there will be needed changes in the leaflet. Even this PR overrides some of its functionalities (eachLayer, onAdd adnd onRemove) to make it asynchronous.

@mmd-osm
Copy link
Contributor

mmd-osm commented Sep 3, 2024

Regarding XML parsing time, we also have JSON based endpoints which were helpful in other scenarios to speed up parsing in the browser.

@nertc nertc force-pushed the issues_osm_4931_map_data_checkbox branch from ccd0f53 to d8561c7 Compare September 5, 2024 06:57
Copy link
Member

@tomhughes tomhughes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks - that looks good now.

@tomhughes tomhughes merged commit 8fd130e into openstreetmap:master Sep 5, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Map Data checkbox: perhaps use toggle slider instead
3 participants