Modify

Opened 15 years ago

Closed 2 months ago

Last modified 7 weeks ago

#4142 closed defect (fixed)

[PATCH] JOSM does not query API for referring relations when downloading primitives

Reported by: Nakor Owned by: team
Priority: critical Milestone: 24.05
Component: Core Version: latest
Keywords: Cc:

Description (last modified by simon04)

Download data using the following bounding box: http://www.openstreetmap.org/?lat=42.65924&lon=-83.3199&zoom=17

Download all members from the relation

Search for way 43418202. It is shown only as a member of relation 308,352 but if it is also member of another relation (see http://www.openstreetmap.org/browse/way/43418202).

This would cause inconsistencies if the way is split.

Attachments (5)

flags.patch (6.4 KB ) - added by stoecker 3 months ago.
Example private flags patch. Quick and Dirty.
4142.patch (9.7 KB ) - added by taylor.smock 3 months ago.
Extract common serialization/deserialization steps, initial functions for setting whether or not all referrers are downloaded
4142.2.patch (26.6 KB ) - added by taylor.smock 3 months ago.
Use new check method in circle alignment, node deletion, and a few other misc places where it should be a drop-in replacement
4142.3.patch (27.6 KB ) - added by taylor.smock 3 months ago.
Fix i18n, perform check in SplitWayAction
4142.4.patch (25.7 KB ) - added by GerdP 3 months ago.
display status of new flag , improve DatasetMerger (please review, looks rather ugly) and DownloadReferrersTask

Download all attachments as: .zip

Change History (76)

comment:1 by mjulius, 15 years ago

Summary: Incomplete data when downloading all memebers of a relationJOSM does not query API for referring relations when downloading primitives

When downloading primitives from the API (with multi get) the API does not return relations the primitive is member of. There is a special API call for that:

GET /api/0.6/[node|way|relation]/#id/relations

(see http://wiki.openstreetmap.org/wiki/API_v0.6#Relations_for_Element:_GET_.2Fapi.2F0.6.2F.5Bnode.7Cway.7Crelation.5D.2F.23id.2Frelations)

JOSM needs to iterate over all downloaded primitives and check whether they are members of any relation.

Better would be if the API provided a call like

GET /api/0.6/[nodes|ways|relations]/#ids/complete

that returns the same kind of data like the map call.

comment:2 by bastiK, 15 years ago

Ticket #4405 has been marked as a duplicate of this ticket.

comment:4 by Nakor, 14 years ago

Ticket #4734 has been marked as a duplicate of this ticket.

comment:5 by stoecker, 14 years ago

Ticket #5086 has been marked as a duplicate of this ticket.

comment:6 by stoecker, 14 years ago

Priority: majorcritical

comment:7 by simon04, 12 years ago

Description: modified (diff)

Any update on this critically prioritized ticket?

comment:8 by skyper, 12 years ago

Maybe a warning with an option to download parents before splitting could help as these ways are outside of the download area.

comment:9 by verdy_p, 10 years ago

Learn CTRL+ALT+D before splitting or merging nodes !
Or place the icon for the "Download referers" tool just beside the two icons on the toolbar for merging or splitting ways (use the preferences panel to add it to your toolbar).
This "Download referers" tool should be on the status bar by default !

comment:10 by verdy_p, 10 years ago

Note: not just referers of ways, but also referers of nodes should be downloaded to avoid conflicts.

But more critically, referers of relations can also include other relations, and this could recurse at deep levels;

  • However siblings found as members of referer relations do not need to be downloaded: only the fact that they are members of a known relation is enough. These members will also be known with a role in the referer relation, though the role is not strictly needed, we just need the member ID and type (node/way/relation).
  • Recursion will only occur if the referer relation is not alread loaded (at least partially). There should be an API that can recurse over parent relations but loading them with only their IDs (we know theu can only be relations). These relations would be loaded in "incomplete" state, without any of its tags and with partial lists of members with unknown roles (and in the partial list of members, not all of them would be loaded).

This would speed up the download, and would also save memory... provided that JOSM can accept such object state for relations. That "incomplete" relation is NOT locally modifiable (not even just to set new tags) as long as it has not been correctly loaded with its tags and the fll ist of members (but not necessarily their roles). We would continue to use "donwload incomplete" to completely load tags of the relation, compeltely load roles of existing members (with current "<unknown>" role, and get the full list of members as references (but not necessarily their own data, i.e. their own tags or geometry or submembers).

comment:11 by verdyp@…, 7 years ago

Note that frequently, the server cannot return all dependencies for some objects, notably when selecting ways that are part of several/many large relations:

The query fails (visible on the console with an HTTP error status), even when retrying it. Apparently the server has limited ressources for doing it.

It is probably not a bug of JOSM, but of the server trying to perform too costly requests (trying to build some costly temporary index for joining results: incorrect execution plan, or existing index estimated to be insufficiently selective). But...

Workaround: select a node one that way, zoom on it to view a very small area around it (to avoid loading too many unnecessary surrounding objects), and then load the tiny area around this node : it works instantly !

For doing the same thing on relations, you need to recurse down to get at least one node (this may require loading members if the relation is incomplete), and then use that node to perform the download by area.

Suggestion: possibly, JOSM could detect that failure and retry by downloading automatically with a query by area, using such tiny rectangle (it can be as small as a few centimeters around that node).

comment:12 by anonymous, 6 years ago

This or a variant of it (such as for when an Overpass API query only downloads certain objects and not all objects in an area, but it should to avoid editing conflicts when that object is later uploaded) may now be possible as of Overpass API 0.7.55:

https://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL#The_block_statement_complete

It should probably be used both when downloading objects in a bounding box (by default), and at least be allowed as a user-selectable option when downloading objects through Overpass API (e.g. '[x] Also download object dependencies', enabled by default (See https://wiki.openstreetmap.org/wiki/Overpass_API/Sparse_Editing for the use case of that).

comment:14 by michael2402, 4 years ago

We have the same problem when downloading relations: A relation can have parent relations and can therefore be 'incomplete' by itself.

This is especially a problem for route relations (=> route master) or multipolygons (=> their corresponding route, stop_area, … relations)

Instead of trying to download all those relations and recurse into them, I would suggest to have a 'incompleteReferrers' flag. This flag can then be unset when JOSM is sure to have downloaded all referrers of that node/way/relation. That way, we can show a warning when modifying this incomplete data.

comment:15 by GerdP, 4 years ago

See also #18959

in reply to:  15 comment:16 by michael2402, 4 years ago

Replying to GerdP:

See also #18959

Thanks for the hint. This is why I could not find a open ticket for it.

Well, since JOSM does not provide a way to indicate that referrers were downloaded, it seems like I need to do the tracking in the PT plugin on my own then. Because for PT routes, the information if parent relations are downloaded is important (e.g. I don't want to present the user with a 'add to route master' or 'create route area group' dialog if that primitive is already inside one. Otherwise, we will have tons of duplicate relations - I already found those on several stops.

comment:17 by skyper, 4 years ago

The PT-Plugin validator rules need an update, too. At least for nodes and ways it should only warn about new objects or within downloaded area, see #15038 or PT: Stop_position is not part of a way for example.

comment:18 by alt.people.davidcalman@…, 9 months ago

My opinion is that this is not a bug. If you want to download the rest of the relation's members, do so by right-clicking on the relation or when the dialog box tells you to.

comment:19 by GerdP, 9 months ago

Yes, and I think a 14 year old ticket about an API call can be closed anyway. It's not even clear to me what problem this ticket is about.

comment:20 by michael2402 -, 9 months ago

It is not about an API call, but about a user interaction problem:

If you download a street / node / relation, JOSM won't neccessarely download parent relations. And won't know if any parent relations exist.

So the actual problem which seems to regularely destroy relations in actual OSM data (observed by me in public transport):

Download a way that is part of a PT route by other means than "Download Area", split the way, then the relation will only be on the old way and not on the newly created one.

comment:21 by GerdP, 9 months ago

Yes, I've made this mistake as well a few times when I downloaded only highways via overpass api. I think we have many newer tickets about this issue?
I think it would be good to warn at least when the nodes of ways with a positive id are changed and there is no download area.

in reply to:  21 comment:22 by anonymous, 9 months ago

Replying to GerdP:

Yes, I've made this mistake as well a few times when I downloaded only highways via overpass api. I think we have many newer tickets about this issue?
I think it would be good to warn at least when the nodes of ways with a positive id are changed and there is no download area.

This would cause many false-positives and not provide a "solution" to users.

My proposal in #18959 was:

  • Store if all parent relations were downloaded as flag on the node/way/relation.
  • We set this flag if a node/way was downloaded by "Download area" or if parent relations were downloaded automatically

That way, we can warn that the way does not have all parent relations downloaded and directly provide a solution to the user: Download those (=> then flag is set) and retry the operation. That way, we do not only provide a warnig but an automated solution and it will probably be not much harder than checking if a way is in a downloaded area (which is not that trivial with multiple areas, the way ways that extend over the download area are handled, ...)

comment:23 by GerdP, 9 months ago

This would cause many false-positives

Why? In what situation?

in reply to:  23 comment:24 by michael2402 -, 9 months ago

Replying to GerdP:

This would cause many false-positives

Why? In what situation?

I don't think the typical case where we need to warn is for people editing just an area. They will download the area and then edit it.

One use case for splitting ways with relations on them without just downloading an area is public transport. In this case, often areas are not downloaded, but instead one downloads only "Relation members". This is also the case described in this ticket. Imageine bus line 1 and 2 share a lot of the way. Typical workflow:

  • Download members of relation "Bus Line 1"
  • Then download referenced relations of that street
  • Then split the street, e.g. for inserting an end stop for one of the bus lines.

In this case, you would get a warning, wether you forgot to do step2 or not. Therefore, the warning won't help there. And those are the critical cases, where data editing is so "complicated" that such errors happen easily.

comment:25 by GerdP, 9 months ago

I would still prefer that JOSM recommends to download an area around the split node.
My work case was this:

  • Download all major highways in an area to set zone:traffic.
  • Note that some ways have a node with traffic_sign=city_limit which is not an end node but very close to that.
  • Merge the two nodes (without downloading relations first)

I ended up with many small gaps in route relations because I simply forgot that JOSM won't warn in that situation.

in reply to:  25 comment:26 by michael2402 -, 9 months ago

Replying to GerdP:

I would still prefer that JOSM recommends to download an area around the split node.

This sould be: Around the original position of the split node (in case the node was moved). And then this won't work if the way was already modified (e.g. merging the node as you mentioned: If the node is not part of the way on the server, downloading the area won't download the way)

And that's why I meant that adding a flag on the way (like "we are sure all relations are loaded") is much easier than handling all the corner cases for downloading the area.

comment:27 by taylor.smock, 9 months ago

Why not add a flag to the DataSource? If a way is not inside fully downloaded bounds, ask the user if we can get referring objects. We could add a flag to the way (and that is arguably the right place to put the flag), but that would increase the memory size of the IPrimitive hierarchy. For reference, AbstractPrimitive has a short flags field instead of booleans.

With that said, now I'm wondering if the short flags still saves memory. I assume it did once, but the JVM has advanced significantly since at least 2011 (when the short flags got moved from OsmPrimitive to AbstractPrimitive). Time for some heap dumps...

EDIT: Using short bits is still more space efficient. Tested with sixteen boolean fields in a class (both record and traditional), a class with a boolean[] field, and a class with a short field. 20k instances of the boolean field class was 640kb. 20k instances of the short field instance was 320kb. The boolean[] field array class was the worst at 960kb. All values were randomly generated to avoid any efficiency shortcuts from the JVM.

Just for the heck of it, I also took a look at JEP 401 value objects. I don't know what is going on, but (as of the Valhalla JDK 20 EA build), it was not as efficient as the short field (it was as efficient as the more standard classes). This should probably be double-checked using real data in production once we move to a JVM with value classes.

Last edited 9 months ago by taylor.smock (previous) (diff)

in reply to:  27 comment:28 by micahel2402, 9 months ago

Replying to taylor.smock:

Why not add a flag to the DataSource? If a way is not inside fully downloaded bounds, ask the user if we can get referring objects. We could add a flag to the way (and that is arguably the right place to put the flag), but that would increase the memory size of the IPrimitive hierarchy. For reference, AbstractPrimitive has a short flags field instead of booleans.

As I stated: The current position of the node and the current ways that belong to the node may have changed after download. And therefore the "inside fully download bounds" check does not reliabely indicate if the node was fully downloaded.

Yes, I mean such a flag on the AbstractPrimitive. I would make it universal: put it on every primitive. Set it when downloaded using "download area" and it was in the orignal bounds. Set it when related objects were downloaded. Read it during merge ways.

Name: "FLAG_PARENTS_COMPLETE". Which means: All ways (for nodes) + relations this primitive is a member of are fetched.

With that said, now I'm wondering if the short flags still saves memory. I assume it did once, but the JVM has advanced significantly since at least 2011 (when the short flags got moved from OsmPrimitive to AbstractPrimitive). Time for some heap dumps...

EDIT: Using short bits is still more space efficient. Tested with sixteen boolean fields in a class (both record and traditional), a class with a boolean[] field, and a class with a short field. 20k instances of the boolean field class was 640kb. 20k instances of the short field instance was 320kb. The boolean[] field array class was the worst at 960kb. All values were randomly generated to avoid any efficiency shortcuts from the JVM.

Just for the heck of it, I also took a look at JEP 401 value objects. I don't know what is going on, but (as of the Valhalla JDK 20 EA build), it was not as efficient as the short field (it was as efficient as the more standard classes). This should probably be double-checked using real data in production once we move to a JVM with value classes.

Object layout has not changed significantly. A boolean is still one byte and byte-aligned, e.g. for performance reasons and for some atomic write operations. Your example did not even get the full benefit, since shorts are still padded and that way your short took more than 16 bit ;-)

Memory layout of the AbstractPrimitive should be to my knowledge (have not checked by docs...):

12 byte object header
2 bytes flags
2 bytes mappaintCacheIdx
8 byte id
4 byte user-reference (8 bytes for large heap)
4 byte changesetId
4 byte timestamp
(potentially 4 byte padding, if no other fields are present in subclass)

comment:29 by skyper, 8 months ago

+1 for a flag

An incomplete list of ticket which would benefit: #10999, #16803, #17400, #19008, #19394, #21211, #22820, #22847

comment:30 by Emvee, 3 months ago

I did run into this issue recently updating the missing maxspeed. By spare editing, querying only the ways without maxspeed and then splitting a way halfway where there is a traffic sign you break the route using that way. Yes, I know how to prevent it but this can be handled better.

Reading this topic I wonder why not just do a

GET /api/0.6/way/#id/relations

on the way before doing the split as suggested by comment 1. For a merge do two of these calls before the actual merge.

If what comment 11 indicates is (still) true use that suggested method.

comment:31 by stoecker, 3 months ago

That's easy to answer. JOSM isn't an online editor. By design it cannot do a download before an operation.

comment:32 by Emvee, 3 months ago

A design can be changed, if there is a good reason for it ;-)

There is also "continuous download"...

comment:33 by taylor.smock, 3 months ago

continuousDownload is a plugin; it is not part of core (and I don't think it ever will be). We do support it (insofar as bug fixes only is considered "support").

Realistically, our only real option is what I investigated in comment:27; adding another bit to our bit field (which is used to decrease the memory usage with lots of primitives).

The problem is that we would have to increase the base memory size of OSM primitives (which is where the vast bulk of our static memory usage is going to be when large areas are loaded).

For reference, we currently use a short for the bit field. A short (per java specification) has 16 bits (2 bytes). The next smallest primitive is an int with 32 bits (4 bytes).

For the "largest" download allowed by the OSM server (10k nodes, figure 5k ways and 5k relations for a total of 20k objects):

  • short: 320,000 bits (or 0.305 Mib)
  • int: 640,000 bits (or 0.610 Mib).

This doesn't sound like much, until you realize that some workflows involve loading massive amounts of data. Think millions of objects. If at all possible, we don't want to significantly increase the memory usage of the OSM primitive classes.

comment:34 by Emvee, 3 months ago

Not a Java programmer or familiar with the josm internals but from comment 28 I understand what is needed is a new flag added to the AbstractPrimitive class.

I opened src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java and it looks to me one could add:

protected static final short FLAG_PARENTS_COMPLETE = 1 << 14;

And things can be implemented without a data size increase.

comment:35 by GerdP, 3 months ago

Right, I also see no memory problem with a new flag, we just have to check / modify lots of code to make sure that the flag is really used and updated. We probably need a routine to fake an empty download result for some existing unit tests.

in reply to:  34 ; comment:36 by taylor.smock, 3 months ago

Replying to Emvee:

I opened src/org/openstreetmap/josm/data/osm/AbstractPrimitive.java and it looks to me one could add:
[...]
And things can be implemented without a data size increase.

I could have sworn something was using bit 14 and 15 somewhere. I'm not finding it right now though.
The problem we have here is that AbstractPrimitive and OsmPrimitive are not sealed classes (sealed classes require Java 15+). Downstream consumers can extend either one of them. Even if they aren't supposed to (for OsmPrimitive).

So adding another flag to AbstractPrimitive won't happen, since extenders of that class may add their own arbitrary flags.

Realistically, we will want to put the additional flag in OsmPrimitive and (maybe) PrimitiveData, since those are supposed to be extended by JOSM core classes only, so there should be minimal breakage in plugins. I'd be happier if we could mark OsmPrimitive and PrimitiveData as sealed classes, but we can't until we drop support for Java 11, which I think is currently kept for Debian Bullseye (EOL in ~3 months) and Ubuntu 22.04 (EOL in ~3 years).

in reply to:  36 comment:37 by stoecker, 3 months ago

Replying to taylor.smock:

So adding another flag to AbstractPrimitive won't happen, since extenders of that class may add their own arbitrary flags.

Hmm. While we care a bit about plugins, there is no need to restrict the core for this. We have no fixed external API. Core needs are more important.

comment:38 by taylor.smock, 3 months ago

The problem is I think I saw one of our (core) subclasses using additional bits. But it might have been something else.

We can try adding it as a bit and see what happens (AKA: what does it break).

If so, now is a good time, since that will hopefully give josm-latest users a chance to tell us that something broke.

comment:39 by GerdP, 3 months ago

I really don't fear that some plugin adds its own meaning to bits in such a basic field. I think that would be very error prone.

comment:40 by stoecker, 3 months ago

When I set "flags" to private I get no error indicating any additional use.

Also as some flags are mutual exclusive we could create virtual flags, e.g. FLAG_INCOMPLETE|FLAG_MODIFIED could be a new flag or FLAG_VISIBLE|FLAG_DELETED. But I'd use such a mechanism only as last resort.

comment:41 by taylor.smock, 3 months ago

I think that would be very error prone.

The last time we added to the flags was back in r13309 (~5 years ago). Yes, you would think it would be very error prone, but the bits we have used have remained relatively stable, so it would "just work".

Of note, the javadocs (from 2011) for flags indicates that "[other] bits of this field are used in subclasses".

I haven't been able to find those other bits though. And I've checked all usages of updateFlags and writes of flags.

What I think I'll do is:

  • Set the flag when we know that there has been a full download (do we want to do FULL_DOWNLOAD or ALL_REFERRERS_DOWNLOADED? I think the latter.)
    • Standard OSM download
    • Specific primitive download with Download referrers
    • Maybe some known-good overpass query patterns?
  • Add the necessary supporting methods (setReferrersDownloaded, isReferrersDownloaded)
  • Commit the changes before Friday so that hopefully someone with a plugin that adds their own flag(s) notices and reports it before the next tested release.

When I set "flags" to private I get no error indicating any additional use.

I'm not certain how you got no error -- it is specifically referenced by several different subclasses.

comment:42 by GerdP, 3 months ago

It would probably be good to change flags to private.
The protected methdod updateFlags() could be changed to ignore flags which are undocumented for core or throw an exception.

in reply to:  41 comment:43 by stoecker, 3 months ago

Replying to taylor.smock:

When I set "flags" to private I get no error indicating any additional use.

I'm not certain how you got no error -- it is specifically referenced by several different subclasses.

I thought I was specific enough: no error indicating any additional use

comment:44 by taylor.smock, 3 months ago

I thought I was specific enough: no error indicating any additional use

I thought you were talking about any additional use outside of AbstractPrimitive. My bad.

It would probably be good to change flags to private.

We'll need a new method (e.g. flags()) since it is used by OsmPrimitive and VectorPrimitive to do bit math. Although I think some of those overriden methods can be moved to AbstractPrimitive instead. I'll want to check why they aren't there already.

comment:45 by stoecker, 3 months ago

I think they all can move to AbstractPrimitive. And probably a copyFlagsFrom(oldPrimitive) function is needed.

by stoecker, 3 months ago

Attachment: flags.patch added

Example private flags patch. Quick and Dirty.

comment:46 by taylor.smock, 3 months ago

We don't need copyFlagsFrom -- we can reuse cloneFrom (and get rid of some of the other sets, but not all). I noticed the setIncomplete as well. It shouldn't be necessary. Maybe it is from a time when it was a separate field?

comment:47 by stoecker, 3 months ago

We can try with something like this in latest and later drop the store and restore function and thus the io stuff and make the field protected again in tested? That should be enough to find bad actors ;-)

comment:48 by taylor.smock, 3 months ago

That might work, but I'd rather not depend upon someone remembering to change stuff back.

in reply to:  46 comment:49 by stoecker, 3 months ago

Replying to taylor.smock:

We don't need copyFlagsFrom -- we can reuse cloneFrom (and get rid of some of the other sets, but not all).

Feel free to improve. Seemed easy so I tried instead of talking ;-)

I noticed the setIncomplete as well. It shouldn't be necessary. Maybe it is from a time when it was a separate field?

Or there is somewhere an Overridden function which does a bit more. Needs to be checked.

That might work, but I'd rather not depend upon someone remembering to change stuff back.

New Ticket for 24.05 should help ;-)

comment:50 by taylor.smock, 3 months ago

Feel free to improve. Seemed easy so I tried instead of talking ;-)

Fair enough. I was doing the same thing at the same time. What I have looks a little different from what you uploaded (mostly order).

Or there is somewhere an Overridden function which does a bit more

Fair enough. I'm not seeing anything anyway (at least for OsmPrimitive).

New Ticket for 24.05 should help ;-)

You'd think so...

adds check for temporary commits in milestone tickets prior to release for wiki:DevelopersGuide/Releasing

comment:51 by stoecker, 3 months ago

BTW: In C++ there is a friend feature which would allow to access protected or private fields from another class. Didn't find this for Java. Not there or did I overlook?

adds check for temporary commits in milestone tickets prior to release for wiki:DevelopersGuide/Releasing

These are guidelines, not laws ;-)

Last edited 3 months ago by stoecker (previous) (diff)

comment:52 by stoecker, 3 months ago

BTW: a question is: Will this cause binary incompatibility (i.e. need plugins be recompiled)?

comment:53 by taylor.smock, 3 months ago

allow to access protected fields from another class

I think the only way to do so would be if any of the following are true:

  • The class in question is a subclass of the originating class
  • The class in question makes another instance of the originating class that happens to be a subclass, and that subclass makes the protected field available.

These are guidelines, not laws ;-)

Yep, but if it isn't there, I'm probably not going to remember to do it.

BTW: a question is: Will this cause binary incompatibility (i.e. need plugins be recompiled)?

As long as they didn't use the flags field, I don't think so. We aren't changing any method definitions anyway.

by taylor.smock, 3 months ago

Attachment: 4142.patch added

Extract common serialization/deserialization steps, initial functions for setting whether or not all referrers are downloaded

comment:54 by stoecker, 3 months ago

Annotations going back to to r2578 show that the additional setIncomplete() is really from ancient times and can be dropped.

Only thing I don't like about your patch is the IO stuff moving. As said we should revert that later. But for now it's ok.

comment:55 by taylor.smock, 3 months ago

additional setIncomplete() is really from ancient times and can be dropped

As suspected, it was from a time when it was a separate field. But it is always good to know.

Only thing I don't like about your patch is the IO stuff moving

Fair. I moved the most of the IO stuff to AbstractPrimitive since it dealt with fields in AbstractPrimitive. If we have to have another method in AbstractPrimitive in order to serialize/deserialize the flags field, I felt it would be better to move all the serialization/deserialization items that dealt with AbstractPrimitive fields to AbstractPrimitive (it would also make it "easier" to implement serialization/deserialization in additional child classes).

by taylor.smock, 3 months ago

Attachment: 4142.2.patch added

Use new check method in circle alignment, node deletion, and a few other misc places where it should be a drop-in replacement

comment:56 by taylor.smock, 3 months ago

Milestone: 24.05
Summary: JOSM does not query API for referring relations when downloading primitives[PATCH] JOSM does not query API for referring relations when downloading primitives

comment:57 by Emvee, 3 months ago

Nice to see a patch!

With the same disclaimer as earlier (no experience with Java or josm code) I had a look at it and it looks to me join/merge has been handled but split not.

comment:58 by taylor.smock, 3 months ago

@Emvee: IIRC, that was covered by the changes to other classes (specifically in JosmAction and Command). I think I tested it anyway. I'd have to pull the patch back down and double-check.

comment:59 by GerdP, 3 months ago

I think we also need a method to use the flag in mapcss rules similar to in-downloaded-area

comment:60 by stoecker, 3 months ago

Your DeleteAction i18n doesn't work. You forgot that there are languages where surrounding text changes with the words. Extract the first sentence and make a new sentence for each variant. Then join again (Duplicating the whole block seems a bit too much).

by taylor.smock, 3 months ago

Attachment: 4142.3.patch added

Fix i18n, perform check in SplitWayAction

comment:61 by GerdP, 3 months ago

It doesn't work yet.

  1. Download way 263221396 with referrers (Download object dialog)
  2. Select a way node (e.g. 1318619874) and "Download parents ways/relations"
  3. Select way 263221396 and the node and try to split

Expected: Split is done since parents of way and node should be known
Result: Popup "You are about to split..." asking to download the parents

I try to find out what's wrong...

comment:62 by GerdP, 3 months ago

BTW: Maybe only for debugging: It would be nice to be able to check the status of the new flag using the "Advanced object info" dialog.

in reply to:  61 comment:63 by GerdP, 3 months ago

I try to find out what's wrong...

DatasetMerger is not yet aware of the new flag. Each merge() is potentially resetting the flag when a freshly downloaded object is preferred and that object doesn't have the flag set. With 4142.3.patch DownloadReferrersTask changes the flag in the "wrong" data set

I'll add a patch which seems to fix this and also changes the "Advanced object info" dialog.

by GerdP, 3 months ago

Attachment: 4142.4.patch added

display status of new flag , improve DatasetMerger (please review, looks rather ugly) and DownloadReferrersTask

comment:64 by GerdP, 3 months ago

Reg. bbox download: I don't yet understand why relations are not marked as having all referrers downloaded when they are completely inside the bbox. My understanding is that there parent relations should in the downloaded data. On the other hand it probably makes no difference for most actions?

comment:65 by taylor.smock, 3 months ago

I don't yet understand why relations are not marked as having all referrers downloaded [...]

  1. From /api/0.6/map documentation,

    All relations that reference one of the nodes, ways or relations included due to the above rules. (Does not apply recursively, see explanation below.)

Effectively, if a relation has no child relation, we can safely consider that relation to have all parent referrers. If, on the other hand, the relation has a child relation, it may not have all parent referrers. I could probably do some additional processing (does the relation have any node/way inside the download box? then mark as fully downloaded).

  1. Our "full" overpass download is not the same as a standard OSM download. I don't know if we want to try and fix that, but we probably should. It reuses the bounding box downloader, so any "full" download will also have the parent referrers flag set.

in reply to:  36 comment:66 by skyper, 3 months ago

Replying to taylor.smock:

I'd be happier if we could mark OsmPrimitive and PrimitiveData as sealed classes, but we can't until we drop support for Java 11, which I think is currently kept for Debian Bullseye (EOL in ~3 months) and Ubuntu 22.04 (EOL in ~3 years).

Both, Debian Bullseye and Ubuntu 22.04 (jammy), include openjdk-17-jre. It is not the default-jre, though.

Last edited 3 months ago by skyper (previous) (diff)

comment:67 by GerdP, 3 months ago

As expected some unit tests are failing, e.g. JoinAreasActionTest, DeleteCommandTest
What's the best way to fix them?

comment:68 by taylor.smock, 3 months ago

For those unit tests, something like ds.allPrimitives().forEach(p -> p.setReferrersDownloaded(true)); should do it.

I think that would keep the "meaning" of the tests anyway, since those tests aren't really concerned with the download status of the primitives.

comment:69 by taylor.smock, 2 months ago

Resolution: fixed
Status: newclosed

In 19078/josm:

Fix #4142: Track fully downloaded objects (patch by stoecker, GerdP, and myself)

The serialization move from PrimitiveData to AbstractPrimitive should be
reverted prior to 24.05 (see #23677).

The serialization move was required since we want to ensure that all downstream
users of AbstractPrimitive were not using the flags field, which was done by
making the field private instead of protected. They may still be using that
field (via updateFlags) which would not be caught by compile-time or runtime
errors.

Additionally, a good chunk of common functionality was moved up from
OsmPrimitive, even though much of it wasn't useful for PrimitiveData.

comment:70 by aceman, 7 weeks ago

I get strange results out of this. I get warnings about possible references on objects where a large surrounding area was downloaded too. I see no obvious reason why there should be some unknown references yet. The patch does allow splitting ways, but warns very often. But it does not allow joining ways (C) which is very weird.

comment:71 by GerdP, 7 weeks ago

Please open a new ticket with full status report and detailed steps how to reproduce

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain team.
as The resolution will be set.
The resolution will be deleted. Next status will be 'reopened'.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.