Hi Jon, Thanks for sending through the patch. I've just taken a look at it. Some comments:
*WayNode* The WayNode class now has the new optional latitude and longitude fields which makes sense. Can you update the store method and (StoreReader, StoreClassRegister) constructor to persist and load those parameters again? They're needed in case the pipeline does any functionality that requires creating temp files such as sorting. The class is now mutable which may cause problems in a multi-threaded pipeline if task implementations are tempted to modify state on the fly. Some of the Osmosis entity classes are mutable (an historical decision which I regrettably allowed through), but they're protected from concurrent modification through a somewhat elaborate read/write protection mechanism (see CommonEntityData for details ...). In this case, can we keep the class immutable by adding those additional fields to an overloaded constructor? *osmformat.proto* Are these changes mastered somewhere else? In other words, are these new fields the same ones used by Osmium in its implementation? I'm wondering if we need to re-sync from the main OSM-binary project. The osmosis version is the same as that in https://github.com/scrosby/OSM-binary. The repo https://github.com/brettch/OSM-binary is a fork of that, and the osmosis branch there *is* the same code as the osmosis-osm-binary directory in the osmosis repo, just with some re-arranging to fit inside the Osmosis structure and fit inside the Osmosis java package namespace. *Jochen* (if you're reading), where does Osmium source its osmformat.proto file from? Long story short, rather than make changes directly to the file in Osmosis and create a fork, should we apply them to upstream first and then re-sync Osmosis with that? Otherwise, the changes look relatively straightforward. I don't have many strong opinions on how to test it. Osmosis doesn't have an amazing test suite, it started out as a hacked together tool and grew into something bigger than I planned. I mostly rely on some basic end to end testing for each task that creates files and reads then back again. That's not possible if you only have read support for the new file format. We may need to check in a small test file with a couple of ways created by Osmium. Cheers, Brett On Tue, 6 Mar 2018 at 04:18 Locke, Jonathan <jonath...@telenav.com> wrote: > Hi Brett, > > > Attached is a patch that adds *WayNode* location (latitude/longitude) > support to *OsmosisReader*. It seems to work fine. You can check it out > by uncommenting the *@Test* annotation on the test I added and supplying > the path to your own PBF file (I would have added a full unit test there, > but I didn't know how you wanted to handle data for test cases in this > project, so I just left it like this for now). You'll want to create your > PBF file with a command similar to this: > > > *osmium add-locations-to-ways --keep-untagged-nodes -o > new-mexico-latest-with-way-nodes.osm.pbf new-mexico-latest.osm.pbf* > > > Only one technical question for you: is there any way to detect from the > header of the PBF file whether the file contains way node locations? It > would be nice not to have to read nodes for 45 minutes before discovering > that the PBF file doesn't have way node locations. Once there's an osmosis > release with the way node location feature in it (and ideally a data drop > with way node locations), we hope to switch to consuming only PBF files > with way node locations and we'd remove support from our apps for PBF files > without this information (thus the need to detect if the file has way > nodes). > > > Best, > > > Jon > ------------------------------ > *From:* Brett Henderson <br...@bretth.com> > *Sent:* Sunday, March 4, 2018 3:40:02 PM > *To:* Locke, Jonathan > *Cc:* osmosis-dev@openstreetmap.org > > *Subject:* Re: [osmosis-dev] Osmosis and Osmium-enhanced PBF files with > way node locations > It's always nice to hear that your software is useful :-) Thanks! Yell > out if you run into any problems and I'll do my best to point you in the > right direction. > > Cheers, > Brett > > On Fri, 2 Mar 2018 at 11:32 Locke, Jonathan <jonath...@telenav.com> wrote: > > Hi Brett, > > From our perspective, it's definitely worth adding this feature because we > use OsmosisReader in a host of custom Java applications (dozens of them). I > think at this point, Osmosis code is running on our servers 24/7/365 doing > various kinds of back-end processing for different groups around the world. > > I totally understand the part about not having time. I am the author of > Apache Wicket and I've stepped away from that project for what are probably > similar reasons (OSS really does soak up time like mad!). So, I will spend > some time developing a patch for OsmosisReader that supports this new > location-enhanced format and I'll get in touch when my patch is ready for > your review. With luck, I shouldn't have too many questions and the patch > will be close to what you'd like. I figure I will just need to look at the > proto files and maybe the osmium code and make the appropriate changes. > Anyway, thanks for writing a great little library. I've had few if any > problems with it and like I said, it powers a lot of what we do with OSM. > > Best, > > Jon > > ------ > > Hi Jon, > > It sounds like a great initiative. Linking ways to locations efficiently > is perhaps the greatest challenge of working with OSM data, and the one > I've spent more time on than most. Including that information in the raw > data sets would be a huge boon for downstream consumers. > > As you may have noticed Osmosis development is fairly quiet these days. > I'm not able to spend much time on it, and it doesn't see many other > contributions. Unfortunately this means you'll probably be on your own. > I'll do my best to answer any questions, but am unlikely to be able to help > directly. > > I'm curious about whether it's worth adding to Osmosis. Are there many use > cases that other tools like Osmium don't cover? If there are that's great, > I'm a bit out of touch. > > Cheers, > Brett > > _______________________________________________ > osmosis-dev mailing list > osmosis-dev@openstreetmap.org > https://lists.openstreetmap.org/listinfo/osmosis-dev > >
_______________________________________________ osmosis-dev mailing list osmosis-dev@openstreetmap.org https://lists.openstreetmap.org/listinfo/osmosis-dev
