Modify

Opened 5 weeks ago

Closed 2 weeks ago

Last modified 2 weeks ago

#18566 closed defect (fixed)

[Patch] Download object: member of relations always downloaded

Reported by: skyper Owned by: GerdP
Priority: normal Milestone: 20.02
Component: Core Version:
Keywords: template_report download object relation member regression Cc: Don-vip

Description

What steps will reproduce the problem?

  1. open download object dialog
  2. choose "relation" as object type and enter id of relation
  3. make sure the second checkbox Download relations members is not checked

What is the expected result?

Only one object, the relation, is downloaded

What happens instead?

Despite the uncheck checkbox all members are downloaded

Please provide any additional information below. Attach a screenshot if possible.

Relative:URL: ^/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2020-01-15 18:25:57 +0100 (Wed, 15 Jan 2020)
Revision:15713
Build-Date:2020-01-16 02:30:53
URL:https://josm.openstreetmap.de/svn/trunk

Attachments (7)

josm_download_object.png (22.7 KB) - added by skyper 4 weeks ago.
screenshot
18566-alpha.patch (11.9 KB) - added by GerdP 4 weeks ago.
18566-wip.patch (33.6 KB) - added by GerdP 4 weeks ago.
Work in progress
18566-v1.patch (36.1 KB) - added by GerdP 4 weeks ago.
rel-editor.PNG (26.3 KB) - added by GerdP 4 weeks ago.
18566.2.patch (37.1 KB) - added by GerdP 3 weeks ago.
overpass api behaves a bit strange. Brakets are needed for ways but not for relations.
18566.3.patch (42.5 KB) - added by GerdP 3 weeks ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 5 weeks ago by skyper

Keywords: regression added

comment:2 Changed 5 weeks ago by skyper

If run with remote control it even downloads the members of child relations.

comment:3 in reply to:  2 Changed 5 weeks ago by skyper

Replying to skyper:

If run with remote control it even downloads the members of child relations.

This happens also if you download relations as members with download (selected) members in the relation dialog

comment:4 Changed 5 weeks ago by Don-vip

Owner: changed from team to skyper
Status: newneedinfo

I don't reproduce. Please share object id, screenshots and .osm data layer.

comment:5 Changed 5 weeks ago by GerdP

Maybe "Use overpass for object downloads" is activated?

comment:6 in reply to:  5 Changed 5 weeks ago by skyper

Replying to GerdP:

Maybe "Use overpass for object downloads" is activated?

Yes, you are right for the general problem and the download from relation editor.

Still without overpass enabled:

  • if you download a type=route_master relation (r69202) its members are downloaded.
  • if you use remote control (69202) on a type=route_master relation its grandchildren are downloaded.

Did not test superroute.

comment:7 Changed 5 weeks ago by GerdP

Cc: Don-vip added

OK, I see. This is not related to the type of the relation. I think it is
The link in the 2nd example says relation_members=true so I think both problems have the same reason.
@Vincent: Maybe a regression of r10129? I wanted to try it but I can't compile the old source.
This small change seems to solve the issue:

  • src/org/openstreetmap/josm/gui/io/DownloadPrimitivesTask.java

     
    5252        super(tr("Download objects"), progressMonitor, layer);
    5353        this.ids = ids;
    5454        setZoom(true);
    55         setDownloadRelations(true, fullRelation);
     55        setDownloadRelations(false, fullRelation);
    5656    }
    5757
    5858    @Override

The javadoc for method setDownloadRelations() looks as if you planed to add something more but forgot it:

    /**
     * Sets whether .
     * @param downloadRelations {@code true} if
     * @param fullRelation {@code true} if a full download is required,
     *                     i.e., a download including the immediate children of a relation.
     * @return {@code this}
     */
    public final AbstractPrimitiveTask setDownloadRelations(boolean downloadRelations, boolean fullRelation) {
        this.downloadRelations = downloadRelations;
        this.fullRelation = fullRelation;
        return this;
    }

comment:8 Changed 5 weeks ago by GerdP

Probably not a regression in r10129. I tested with josm-snapshot-10123.jar and got the same result.

Changed 4 weeks ago by skyper

Attachment: josm_download_object.png added

screenshot

comment:9 Changed 4 weeks ago by skyper

Owner: changed from skyper to team
Status: needinfonew

screenshot of example:
screenshot

The behaviour is different depending on something between r69202 and r1080376.

  • without overpass r1080376 is fine but r69202 not.
  • with overpass both are treated the same with ignoring the checkbox.
Last edited 4 weeks ago by skyper (previous) (diff)

comment:10 Changed 4 weeks ago by GerdP

r69202 has relations as members, r1080376 doesn't.

comment:11 Changed 4 weeks ago by skyper

So we have two problems:

  • relations as members are always downloaded.
  • if use overpass to download objects is enabled the checkbox is not used.

comment:12 Changed 4 weeks ago by Don-vip

I guess it is intentional when I fixed #10388 in r7432. I don't remember the details.

comment:13 Changed 4 weeks ago by GerdP

Owner: changed from team to GerdP
Status: newassigned

Besides the unclear code above another is that MultiFetchOverpassObjectReader.buildRequestString() always recurses down.
Also sometimes the standard api is used even when overpass api was selected.
I'll try to find a fix for these issues.

comment:14 Changed 4 weeks ago by GerdP

@Skyper: When you download relation 69202 "with members", you want to get the relation 69202 and its two sub relations 1080376 and 1080375, but not any node or way, right? This would be a big change, because now the checkbox "download relation members" means something like "load all until nothing is incomplete"

Last edited 4 weeks ago by GerdP (previous) (diff)

comment:15 in reply to:  14 Changed 4 weeks ago by skyper

Replying to GerdP:

@Skyper: When you download relation 69202 "with members", you want to get the relation 69202 and its two sub relations 1080376 and 1080375, but not any node or way, right?

Yes, I want to modify the tags of the relation(s), e.g. I am not interested in members at all (no members) or only in the members of the mentioned relation 69202 (download members) but no escalating down.

This would be a big change, because now the checkbox "download relation members" means something like "load all until nothing is incomplete"

The text for the check box says: Download relation members
Note the singular form of relation.

Could be a long time ago that it was changed but I think it did work differently years ago or am I off and I did always use "Download Location".

comment:16 Changed 4 weeks ago by GerdP

I think it should work like this (no matter if overpass or OSM api is used):
1) If you download relation r69202 with "download relation members" enabled you should get the complete relation, because "download relation members" always downloads node members and way members completely. It should also download relation members completely to be consistent. I don't understand your hint reg. "singular form". The dialog also allows to download two or more relations and members is plural.
2) If you download relation 69202 with "download relation members" disabled you should get only the incomplete relation.

I am not yet sure what I would want to get when I use update selection when a relation like r69202 is selected. I was surprised to see that lots of nodes and ways are downloaded when the selected relation is complete. I expected to see the same downloads as with 1)

comment:17 in reply to:  12 ; Changed 4 weeks ago by GerdP

Replying to Don-vip:

I guess it is intentional when I fixed #10388 in r7432. I don't remember the details.

Yes, likely. With snapshot r7431 I see the expected (1) behaviour, with r7435 it works like now (overpass api wasn't yet implemented)
(1) as described in comment:16

Last edited 4 weeks ago by GerdP (previous) (diff)

comment:18 in reply to:  17 Changed 4 weeks ago by skyper

Replying to GerdP:

I think it should work like this (no matter if overpass or OSM api is used):
1) If you download relation r69202 with "download relation members" enabled you should get the complete relation, because "download relation members" always downloads node members and way members completely. It should also download relation members completely to be consistent. I don't understand your hint reg. "singular form". The dialog also allows to download two or more relations and members is plural.
2) If you download relation 69202 with "download relation members" disabled you should get only the incomplete relation.

I can life with this though a new checkbox Only download 1st level members would be nice.
Do not want to get picky. There are relations with only one member. The correct plural would be: Download relations members'

Replying to GerdP:

Yes, likely. With snapshot r7431 I see the expected (1) behaviour, with r7435 it works like now (overpass api wasn't yet implemented)
(1) as described in comment:16

Overpass was a plugin before getting into core. Think the option to use overpass to download objects existed already, five years ago, with the plugin.

comment:19 Changed 4 weeks ago by GerdP

BTW: For me, "Use overpass for object downloads" is always much slower compared to OSM api, so I only use it for testing. What's your reason to use it?

comment:20 Changed 4 weeks ago by skyper

Did not check the speed, lately. There was a time when it was faster to download from overpass and when the OSM-api had some hick ups. Still, you can save resources on the OSM-api server.

comment:21 Changed 4 weeks ago by GerdP

I fear I cannot solve this problem without a complex rewrite of the existing classes. We have lots of different classes which download a list of primitives. Some are aware of the Overpass api, some are not.
For example, "Download parent ways/relations" always uses the OSM api to find the parents but then uses Overpass to download the details.
With Overpass api I would expect a single call with something like this for node 26165886:

(
  // node with parent ways/relations
  node(id:26165886) -> .n;
  .n;way(bn);>;	
  .n;rel(bn);>>;
);
out meta;

to retrieve the node, its complete parent ways and is complete parent relations in one api call.
Instead I see multiple calls, some using Overpass, some OSM api.

Overpass query: node(id:26165886);out meta;
https://api.openstreetmap.org/api/0.6/node/26165886/ways
https://api.openstreetmap.org/api/0.6/node/26165886/relations 
https://api.openstreetmap.org/api/0.6/way/314042995/full
https://api.openstreetmap.org/api/0.6/way/208055465/full
https://api.openstreetmap.org/api/0.6/way/30503471/full
https://api.openstreetmap.org/api/0.6/way/30503468/full
Overpass query: node(id:1703187559,3861801992,1701375246,30893809,1150742558,3861801990,3861801991,3200768061,3861801989,26291274,4538502920,1150742825);out meta;

One big problem with the existing Overpass api integration is that Overpass doesn't like frequent calls from the same internet address, so if we use it we should probably prefer to create one complex query instead of many rather simple ones.

Last edited 4 weeks ago by GerdP (previous) (diff)

Changed 4 weeks ago by GerdP

Attachment: 18566-alpha.patch added

comment:22 Changed 4 weeks ago by GerdP

The attached patch implements the following changes:
1) When downloading relations with "Download Object" the settings for "Download relation members" are applied.
"Download relation members" means that the complete relation is wanted, including complete sub relations.
2) When updating a selection only the selected objects are updated, not their children. With the unpatched version all known members are also updated, including sub relations and their complete members down to all nodes.
Not sure if the old behaviour is wanted.
3) MultiFetchOverpassObjectReader now uses "Recurse down relations (>>)" to fetch complete sub relations. A new method setRecurseDownRelations() allows to switch off the "recurse down" for relations. The generated overpass query was optimized based on the hint https://wiki.openstreetmap.org/wiki/Overpass_API/Overpass_QL#By_element_id

Performance hint: Try to bundle multiple id queries into one statement, i.e. instead of writing (way(3998029);way(3998240);way(4065354);); use way(id:3998029,3998240,4065354);  

4) Optimization: When overpass api is used with a selection of ways and nodes, only those nodes are downloaded which are not referenced by the downloaded ways as those are already complete. (we always recurse down when downloading ways with the overpass api)

Other problems:

  • "download members" doesn't use overpass api, it always use the OSM api with the /full suffix like this:
    https://api.openstreetmap.org/api/0.6/relation/69202/full
    
  • "download incomplete members" is typically much slower than "download members", maybe both should use the faster api call to retrieve the complete relation and later filter what's really wanted.

Edit: Forgot to mention that "https://api.openstreetmap.org/api/0.6/relation/69202/full" doens't download the members of the sub relations.

Last edited 4 weeks ago by GerdP (previous) (diff)

Changed 4 weeks ago by GerdP

Attachment: 18566-wip.patch added

Work in progress

comment:23 Changed 4 weeks ago by GerdP

In additon to 18566-alpha.patch the new 18566-wip.patch implements

  • new methods setRecurseDownRelations()and setRecurseDownAppended() in MultiFetchServerObjectReader to give more control to the caller and allow to get equal results from Overpass api and OSM api. Also new method appendIds() was added.
  • use of MultiFetchServerObjectReader in more places
  • fixes an EDT violation in class ChildRelationBrowser
  • "Update sellection" completes the updated elements for both APIs in the same way. Maybe I should add a new action like "Update and complete selection" so that user can decide what they want?

Changed 4 weeks ago by GerdP

Attachment: 18566-v1.patch added

Changed 4 weeks ago by GerdP

Attachment: rel-editor.PNG added

comment:24 Changed 4 weeks ago by GerdP

Milestone: 20.02
Summary: Download object: member of relations always downloaded[Patch] Download object: member of relations always downloaded

Various changes to improve performance and reliability of download actions. I am not yet sure what to do with the download ... buttons in the relation editer dialog:

Should the button "download button" work like "Update selected" or like "Download Object..."? In the first case all known objects would be downloaded, else just the complete Relation. The result would be the same unless the relation was changed on the server, e.g. if a member was deleted.

comment:25 Changed 4 weeks ago by skyper

Wow, never used that tab. Downloads nicely, without members, incomplete child relations .

Last edited 4 weeks ago by skyper (previous) (diff)

comment:26 Changed 3 weeks ago by GerdP

I also never used it. Thought you would like it ;)

comment:27 in reply to:  24 Changed 3 weeks ago by skyper

Replying to GerdP:

Should the button "download button" work like "Update selected" or like "Download Object..."?

Do not know the internals, but it should be the same as download members from tag/membership, relation and selection toggle dialogs' context menus

comment:28 Changed 3 weeks ago by GerdP

Yes, sounds reasonable.

Changed 3 weeks ago by GerdP

Attachment: 18566.2.patch added

overpass api behaves a bit strange. Brakets are needed for ways but not for relations.

Changed 3 weeks ago by GerdP

Attachment: 18566.3.patch added

comment:29 Changed 3 weeks ago by GerdP

18566.3.patch improves multifetch with overpass api:

  • generate a single query for all objects
  • detect missing primitives (as overpass doesn't return invisible (deleted) objects and download them with the OSM api if needed

This performs much better than a bunch of rather simple overpass queries. It uses "POST" instead of "GET" and thus avoids to hit some quotas.

comment:30 Changed 3 weeks ago by GerdP

I think there is still one big problem: When you try to upload data and a conflict is found the current code might use Overpass api to "synchronize with the server". I think this is not a good idea, it is likely that the Overpass server is not up to date in this case.

comment:31 Changed 3 weeks ago by GerdP

I need help here: I think it would be best to temporarily disallow usage of overpass API when an upload task is running. What would be the best way to do this?
My idea would be to store the current content of property OverpassDownloadReader.FOR_MULTI_FETCH, make sure it is set to false, execute the upload task and finally reset the OverpassDownloadReader.FOR_MULTI_FETCH in case it was true before.
Is that an acceptable way to do this?

comment:32 Changed 2 weeks ago by GerdP

Resolution: fixed
Status: assignedclosed

In 15811/josm:

fix #18566: Download object: member of relations always downloaded (18566.3.patch)

  • return same result when downloading objects, no matter if overpass api is enabled or not.
  • fix an EDT violation in class ChildRelationBrowser
  • let MultiFetchOverpassObjectReader create a single overpass query, no matter how many objects are requested

(gives much better reaction times).

  • use multi-fetch in ChildRelationBrowser to reduce duplicated code. Small disadvantage: When downloading multiple members of a relation the dialog doesn't update after each member. Should not matter much as the overall time for multiple members is shorter

I tried to keep the code backward compatible. Plugins reverter and undelete should continue to work.

comment:33 Changed 2 weeks ago by GerdP

In 15812/josm:

see #18566: fix findbugs UWF_UNWRITTEN_FIELD introduced with r15811

To be honest, I don't understand the synchronized (this) blocks in the code. Looks like an incomplete approach to implement a "download in background". Hope this change doesn't block anything.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain GerdP.
as The resolution will be set.
The resolution will be deleted.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.