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

Don't automatically download selected relations #6668

Closed
BjornRasmussen opened this issue Jul 21, 2019 · 8 comments
Closed

Don't automatically download selected relations #6668

BjornRasmussen opened this issue Jul 21, 2019 · 8 comments
Labels
performance Optimizing for speed and efficiency
Milestone

Comments

@BjornRasmussen
Copy link
Contributor

When editing in this area, I somehow selected a huge multipolygon representing the English Channel. Then, the entire relation, including all of its coastline members, started downloading, and iD froze up.

Instead of automatically downloading the members, iD should include a message similar to a warning, potentially like this:

This relation isn't fully downloaded
📥 Download all members - when clicked, this downloads all members.
❌ Ignore for now - removes the note.

This would prevent iD from downloading a ton of data unless the mapper actually wants this data.
If the relation only has a few undownloaded members (less than 20?), it might make more sense to just download them and not bother the mapper.

=============

I really love the new features for downloading all relation members and visualizing them! It lets me visualize relations better than previously possible with iD. I just don't think that it's really a good idea to make this feature something that could ruin editing sessions for some people.

@slhh
Copy link
Contributor

slhh commented Jul 22, 2019

Requiring to press a button to download the members of large selected relations seems to be acceptable for me in general, but it needs to work for multiselections and when the assisant is closed. This makes the validation like style probably inapplicable.

The button isn't a full soultion anyway, offering a button to freeze iD unintentionally is still bad.
I don't think the download is the real issue, but putting the huge amount of data in the graph.
Maybe #6669 can help here.

@quincylvania
Copy link
Collaborator

So iD isn't downloading all members, just the first 450. I mentioned that we can play with this number in #4903 (comment). Maybe 300 or 150? iD chunks the requests into 150-entity increments.

@quincylvania
Copy link
Collaborator

Here's another thought. We know what type the entites are before we download them, so we can probably afford to fetch many more nodes than ways. Downloading ways is slower since it kicks off additional calls to download and merge the ways' nodes.

@BjornRasmussen
Copy link
Contributor Author

That makes me wonder... Would it be possible to just download 500 members, and then keep merging that data into the graph until 5000 (or another number as appropriate) nodes have been added, and then stop? If less than that is downloaded, 500 more members could be downloaded, and the process could restart.

This would be nice to have, but I'm sure that it's not the #1 priority right now.

@bhousel
Copy link
Member

bhousel commented Aug 12, 2019

Downloading ways is slower since it kicks off additional calls to download and merge the ways' nodes.

Oh wow, this is very inefficient. We can't do it this way.

If we are using the Multi-Fetch GET call, maybe we can put in a request upstream for a full option to return their nodes also.

(btw, the only other place iD uses this call is in the save code when trying to resolve conflicts, and we know it's slow there:)

iD/modules/modes/save.js

Lines 160 to 161 in 489aa37

// Because loadMultiple doesn't download /full like loadEntity,
// need to also load children that aren't already being checked..

cc @mmd-osm for thoughts on this.

@quincylvania
Copy link
Collaborator

If we are using the Multi-Fetch GET call, maybe we can put in a request upstream for a full option to return their nodes also.

Yes, this is how we're doing it since the only other option I found was to use a separate request for each individual entity, which is worse.

To be clear, we're not doing a separate request for the nodes of each individual way. We request 150 features, then send a combined request for all the unloaded member nodes of the returned ways. It'd still be way better to get all the member nodes along with the ways!

@mmd-osm
Copy link
Contributor

mmd-osm commented Aug 12, 2019

I'd say the requirement sounds reasonable. Can you please open an issue on https://github.com/openstreetmap/openstreetmap-website/issues to discuss some further details, like if/how an existing endpoint could be extended, or if it makes more sense to create a new endpoint. As the API is "owned" by the Rails port, we need some guidance by the OSM website maintainers.

@quincylvania
Copy link
Collaborator

Closing this since it only concerns the v3-prototype branch at the moment. See #4903 and #6656 instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Optimizing for speed and efficiency
Projects
None yet
Development

No branches or pull requests

5 participants