graphhopper / map-matching

The map matching functionality is now located in the main repository https://github.com/graphhopper/graphhopper#map-matching
https://www.graphhopper.com/open-source/
Apache License 2.0
785 stars 274 forks source link

Add Extended Json output format #41

Closed chucre closed 8 years ago

chucre commented 8 years ago

Add a new type: GPXEntryExtension that extend GPXEntry, only add name field. Add a Json format that is compatible with https://mapmatching.3scale.net/tutorial result Add a new type for server (extended_json) that use new Json format

karussell commented 8 years ago

Thanks! Interesting!

something is wrong here:

new GPXFile(mr, il).doExport(outFile);
if (format.equals("gpx")) {
   new GPXFile(mr, il).doExport(outFile);    

And why do we need the GPXExtensionEntry?

Two important things:

chucre commented 8 years ago

I will fix that. And if want, i can write the other json format test too, I I will need more info. On Dec 11, 2015 6:40 AM, "Peter" notifications@github.com wrote:

Thanks! Interesting!

Two important things:

  • I would avoid the json dependency in the parent or in the core map matching project, also currently the versions clash
  • We need a test for this - see MatchServletTest (okay we have also no test for the JSON -> another TODO but for me ;))

— Reply to this email directly or view it on GitHub https://github.com/graphhopper/map-matching/pull/41#issuecomment-163875746 .

chucre commented 8 years ago

@karussell today I'm writing a algorithm to calculate the time travel in each edge. To do so I need the time of the each wpt track. So in extended json format I add the original track list in each matched edge. With the corrected lat and lng and id (name attribute). But thinking now, I can write the original timestamp in my output and it will a little my algorithm, since if I have a timestamp in json format, I not need get the timestamp from a map.

I will remove GPXExtensionEntry now, but is important know GPXEntry have more then x,y,z and timestamp;.

chucre commented 8 years ago

@karussell these change looks good?

karussell commented 8 years ago

Thanks!

Why did you need the public methods in GPXExtension? Accessing the attributes directly should be possible within the same package e.g. for testing.

Also would you squash the commits into one commit?

chucre commented 8 years ago

I need public methods because I move com/graphhopper/matching/JsonFile from core to com/graphhopper/matching/http/MatchResultToJson in web module. I could move it to same package, and when PR is ready to be merged, I will squash it.

karussell commented 8 years ago

I see. Then this is reasonable to keep it in the web module. Because if you move JsonFile to core you'll need also the json dependency there which is not desirable (at least for now)

Also would you add a minor description to MatchResultToJson and extended_json in order to understand why another JSON structure is necessary? Also point to this PR.

chucre commented 8 years ago

MatchResultToJson create a structure compatible with: https://mapmatching.3scale.net/tutorial. This service result with routes [ { links: [ { geometry: String, wpts } ] } ]. Where link is a EdgeMatch and wpts are original GPXEntry with position corrected. I don't want change current JSON format because retro-compatibility.

karussell commented 8 years ago

No need to change anything, sorry if this wasn't clearly expressed :)

Just add a comment to the new JSON format or class and squashing the commits would be good to go!

karussell commented 8 years ago

Thanks - merged! (Latest graphhopper master is necessary to use the now protected method errorsToXML for better error reporting if input gpx is empty and not throwing an error)