Modify

Opened 3 years ago

Closed 8 months ago

#13843 closed defect (fixed)

Not properly loading attributes from multipart objects

Reported by: naoliv Owned by: openbrian
Priority: normal Milestone:
Component: Plugin opendata Version:
Keywords: Cc:

Description

The attached example shapefile has a multipart object (two lines) with name=Test
When opening it in JOSM, however, we can see name=Test only in one way, with the other having no tags at all.

Shouldn't all ways have the same properties/tags when importing multipart objects like this?

URL:http://josm.openstreetmap.de/svn/trunk
Repository:UUID: 0c6e7542-c601-0410-84e7-c038aed88b3b
Last:Changed Date: 2016-10-22 12:33:10 +0200 (Sat, 22 Oct 2016)
Build-Date:2016-10-23 01:37:31
Revision:11154
Relative:URL: ^/trunk

Identification: JOSM/1.5 (11154 pt_BR) Linux Debian GNU/Linux unstable (sid)
Memory Usage: 249 MB / 4029 MB (82 MB allocated, but free)
Java version: 1.8.0_111-8u111-b14-2-b14, Oracle Corporation, OpenJDK 64-Bit Server VM
Screen: :0.0 1920x1080
Maximum Screen Size: 1920x1080
Java package: openjdk-8-jre:amd64-8u111-b14-2
Java ATK Wrapper package: libatk-wrapper-java:all-0.33.3-9
VM arguments: [-Dawt.useSystemAAFontSettings=on]
Dataset consistency test: No problems found

Plugins:
+ AddrInterpolation (32699)
+ Create_grid_of_ways (32699)
+ FastDraw (32938)
+ FixAddresses (32796)
+ ImportImagePlugin (32699)
+ OpeningHoursEditor (32699)
+ PicLayer (32796)
+ SimplifyArea (32796)
+ apache-commons (32699)
+ buildings_tools (32944)
+ download_along (32946)
+ editgpx (32699)
+ ejml (32680)
+ geojson (43)
+ geotools (32813)
+ graphview (32796)
+ jogl (1.0.46)
+ jts (32699)
+ kendzi3d (1.0.191)
+ kendzi3d-resources (0.0.1)
+ log4j (32699)
+ measurement (32936)
+ merge-overlap (32699)
+ opendata (32898)
+ pdfimport (32796)
+ photo_geotagging (32699)
+ poly (32699)
+ reverter (32796)
+ tageditor (33021)
+ tagging-preset-tester (32869)
+ todo (30000)
+ turnlanes-tagging (252)
+ turnrestrictions (32796)
+ undelete (32699)
+ utilsplugin2 (32815)

Attachments (4)

test.tar.gz (563 bytes) - added by naoliv 3 years ago.
patch_13843.diff (9.9 KB) - added by openbrian 8 months ago.
patch to copy attribs to all parts of a multipart feature.
patch_13843_svn.diff (8.3 KB) - added by openbrian 8 months ago.
same patch but generated using svn instead of git.
13843_fix_zip_reader_test.diff (2.5 KB) - added by openbrian 8 months ago.
Fix a failing automated test for ZipReader

Download all attachments as: .zip

Change History (23)

Changed 3 years ago by naoliv

Attachment: test.tar.gz added

comment:1 Changed 3 years ago by naoliv

Summary: Not properly opening shapefiles with multipartsNot properly loading attributes from multipart objects

comment:2 Changed 3 years ago by Don-vip

what kind of object is that? If I remember correctly, the plugin assembles all parts into a single OSM way, but in this case the segments are discontinued. Is the geometry valid?

comment:3 Changed 2 years ago by Don-vip

Owner: changed from Don-vip to naoliv
Status: newneedinfo

comment:4 Changed 2 years ago by Don-vip

Resolution: needinfo
Status: needinfoclosed

comment:5 Changed 10 months ago by openbrian

Resolution: needinfo
Status: closedreopened

I ran into this issue too. Using QGIS, I can see that a feature is a geometry collection of linestrings. When I use the OpenData plugin to open this data in JOSM, I see the non-geometry attributes are lost from some OSM ways.

I did a scan of the code (https://github.com/openstreetmap/josm-plugins/blob/master/opendata/src/org/openstreetmap/josm/plugins/opendata/core/io/geographic/ShpReader.java#L149) and I think I see why.

"w" is assigned one part of the geometry. The next item (line 132) will clobber "w". The primitive is only assigned the last geometry item (line 157). And the attributes are only assigned to this feature (line 172).

I can take a stab at fixing this, but I need instructions on how to compile the plugin (standalone) and install it into my locally installed JOSM. The READMEs (opendata, and josm-plugins) are missing this.

If you have opinions, please let me know how you would like this solved. Looks like polygons have this issue too. Not sure I'll solve that issue. If possible, I'd like to start by creating an automated test for this in ShpReaderTest.java.

Also, has what's the official repo for the plugin: svn or github? Github repo says it's a mirror. svn.osm.org says "NOTICE: Please note that this OpenStreetMap svn repo is deprecated and many items have now moved to repositories under the github.com/OpenStreetMap project."

comment:6 Changed 10 months ago by Don-vip

Hi Brian,
Thanks for your preliminary analysis!
The official repository for plugins is the OSM SVN. We plan to switch from it to eventually one git repository per plugin, but we're not there yet.
To compile and install the plugin you can do as follows (see DevelopersGuide/DevelopingPlugins#Settinguptheenvironment for details):

  • svn co https://svn.openstreetmap.org/applications/editors/josm
  • cd plugins/opendata
  • ant clean install

I would be happy to apply your future patch, let me know if you have other questions :)

A test case would be nice too, to ensure non-regression.

Other interesting targets:

  • ant test
  • ant checkstyle
  • ant spotbugs
Last edited 10 months ago by Don-vip (previous) (diff)

comment:7 Changed 10 months ago by openbrian

I made a github repo for josm-opendata and created a branch to fix this issue. Please review the PR (branch against master) here.

https://github.com/openbrian/josm-opendata/pull/1

I did this in several commits so you can see how the code changed.

(Notice on master, there's one other small change. I just renamed a variable.)

comment:8 Changed 10 months ago by Don-vip

It looks nice :) Can you please add a test? Then I'll apply the patch and make a new release.

Changed 8 months ago by openbrian

Attachment: patch_13843.diff added

patch to copy attribs to all parts of a multipart feature.

comment:10 Changed 8 months ago by Don-vip

The patch doesn't apply nicely. Are you up to date with the current SVN code base?

comment:11 Changed 8 months ago by openbrian

Sorry about that. I overlayed a git repo on top of the opendata folder. I did my work in a branch and generated the patch using "git diff master". Here's one using "svn diff".

Changed 8 months ago by openbrian

Attachment: patch_13843_svn.diff added

same patch but generated using svn instead of git.

comment:12 Changed 8 months ago by Don-vip

Resolution: fixed
Status: reopenedclosed

Fixed in [o34898:34900]. Thanks!

comment:13 Changed 8 months ago by Don-vip

The changes cause a regression, see Jenkins.

comment:14 Changed 8 months ago by Don-vip

Resolution: fixed
Status: closedreopened

@openbrian I have fixed the NPE in [o34907] but it doesn't fix the test. Can you please take a look?

comment:15 Changed 8 months ago by Don-vip

Owner: changed from naoliv to openbrian
Status: reopenednew

comment:16 Changed 8 months ago by Don-vip

Possible regression: #17397

Changed 8 months ago by openbrian

Fix a failing automated test for ZipReader

comment:17 Changed 8 months ago by openbrian

I added a patch to fix the failing automated test.

comment:18 Changed 8 months ago by Don-vip

Applied in [o34911].

comment:19 Changed 8 months ago by Don-vip

Resolution: fixed
Status: newclosed

Released in [o34912].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain openbrian.
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.