incomplete ways after restoring

[tyrasd]

After #2135 the graph can contain incomplete ways (i.e. ways where one or more of its nodes are not present in the graph - just like incomplete relations) under certain circumstances. For example:

1. Have a multipolygon (or route, …) relation that spans over two data tiles A and B (way w1 is completely in A)
2. while editing in region A, split way w1
3. abort the mapping session, restore from region B
4. the graph will now contain the multipolygon relation, way w1 and the split remainder (say way w-1), but not any of the nodes of w1 and w-1.

I noticed that this causes problems with the current multipolygon code: it causes entity nxxx not found exceptions during graph.childNodes() because some nodes are in fact missing.

I'm not sure how to fix this properly. A possible fix (for the multipolgon case) would be to add something like the following to the relation member filter:

… && graph.entity(member.id).nodes.every(function(node) { graph.hasEntity(node.id); });

Another fix would be to include all involved (but most of the time unchanged) childNodes in the localStorages saves. But this would potentially quite blow up the amount of data to be stored.

Also, I'm not sure if similar exceptions could come up in other modules, too?

[jfirebaugh]

Oh, damn, I forgot about the assumed invariant that a way always comes with all its child nodes. It's a pretty widespread assumption in the code, so it will be tricky to try and relax it.

I agree that including child nodes in localStorage is also unattractive, but I think that's the only way to preserve the child nodes invariant.

I thought about a different fix for #2134, like if an entity kept track of whether it was new or not, so that it would be possible to determine modified vs created without keeping track of the base entity. But that still leaves modified versions of restored ways without their child nodes.

[pnorman]

It's a pretty widespread assumption in the code, so it will be tricky to try and relax it.

I gave some thought to it, and it's also a requirement for all sorts of operations with the ways (e.g. rendering them)

The nodes and way/full calls are pretty fast now that they're running on cgimap. Could we just fetch whatever we need to complete what's incomplete?

[jfirebaugh]

Yeah, that's another idea. The difficulties there are:

• iD must block the UI while the nodes are downloaded.
• Both nodes and way/full are "current state only" -- there's no way to request particular versions or points in time. So we'd need to think about a conflict resolution strategy for cases where nodes were added or deleted. Although I suppose any conflicts that happen at restore time would currently end up being conflicts when you try to save, and there's currently no recourse for that anyway.

[pnorman]

Although I suppose any conflicts that happen at restore time would currently end up being conflicts when you try to save, and there's currently no recourse for that anyway.

If we're going to punt on conflict resolution, it's much better to do it early before the user does additional work, so I'd consider this a plus.

iD must block the UI while the nodes are downloaded.

Although it sucks to block, is it better than storing everything in localStorage?

I don't think it'd be blocking too long, I consider both nodes and way/full to be faster than map these days, even though they do different tasks and aren't directly comparable.
I suggested relation/full for that reason in #1201 (comment) to fix MP rendering issues

[tyrasd]

Isn't conflict resolution a more general problem and not particularly related to this issue here? I mean, conflicts could come up even after "regular" restores, when fresh (and possibly conflicting) data is loaded via the map call. Or simply on a normal editing session during upload…

[bhousel]

from @pnorman

The nodes and way/full calls are pretty fast now that they're running on cgimap. Could we just fetch whatever we need to complete what's incomplete?

I'm planning to fix this with nodes call to fetch the missing nodes.

from @jfirebaugh

iD must block the UI while the nodes are downloaded.

I found with conflict resolution that nodes calls are pretty fast (certainly faster than loading tiles, which is what iD would otherwise be doing). Also I think having iD pause a few seconds after the user chooses "restore" is acceptable.

Both nodes and way/full are "current state only" -- there's no way to request particular versions or points in time. So we'd need to think about a conflict resolution strategy for cases where nodes were added or deleted.

Existing conflict resolution will handle added nodes (the user just won't see them until they save and see the conflict resolution UI).

If the nodes call returns that the node has been deleted, I can replace it with a local new node, so the renderer doesn't choke on "entity not found" errors. Conflict resolution will catch this too and prompt the user whether to keep the node or remove it.

[bhousel]

Today I realized:

1. I never actually implemented the "iD must block the UI while the nodes are downloaded." part of this..
2. Adding temp nodes to avoid Entity Not Found errors doesn't work because the temp nodes won't have loc, which is essential in a lot of other parts of the code (area/extent calculation)
3. Blocking UI alone is insufficient because the missing childnodes are loaded asynchronously and in batches, triggering map redraws. So I needed to block both UI and redrawing while these missing child nodes are loaded asynchronously.

This was implemented in a393248, 970a5de and fixes a lot of the Entity Not Found errors that can occur after the user restores their history.