Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adding nodes's and ways's version access in lua binding. #3373

Merged
merged 2 commits into from Jan 4, 2017

Conversation

fijemax
Copy link
Contributor

@fijemax fijemax commented Nov 29, 2016

Adding nodes's and ways's version access in lua binding for users who need this information.

Tasklist

Signed-off-by: FILLAU Jean-Maxime jean-maxime.fillau@mapotempo.com

@daniel-j-h
Copy link
Member

Are you sure the version tag is still available since we're using the read_meta::no flag for osmium:

osmium::io::Reader reader(input_file, osmium::io::read_meta::no);

@fijemax
Copy link
Contributor Author

fijemax commented Nov 29, 2016

Hi @daniel-j-h
You are right, can I add an option at extractor to parse meta ?

@daniel-j-h
Copy link
Member

The reason we added this flag is a pbf parsing speed up by 10-20%.

https://github.com/osmcode/libosmium/releases/tag/v2.10.0

@frodrigo
Copy link
Member

We do computation from way geometries in lua to adjust speed. As computation are time consuming, we want to cache the result for next run of osrm-extract. But for caching the computation result we need to have the way version. So in our case reading metata will result in speed up.

If we code parsing meta data of pbf as as option, and add it to this pull request, do you merge it ?

@MoKob MoKob added the Review label Nov 30, 2016
@daniel-j-h
Copy link
Member

Yes one way to achieve this would be to add a new command line option e.g. osrm-extract --with-osm-metadata and keep the metadata reader flag disabled by default unless explicitly specified.

@TheMarex
Copy link
Member

Adding a flag would work for me, but what happens when this function is called from a profile and there is no meta data? Does it crash?

@fijemax
Copy link
Contributor Author

fijemax commented Dec 1, 2016

I tested this use case, if this function is called but there isn't meta data function return 0.

@fijemax
Copy link
Contributor Author

fijemax commented Dec 6, 2016

Hi @daniel-j-h, I added the command line option osrm-extract --with-osm-metadata for enable meta data reading. @TheMarex if the flag is disable and function is called from a profile zero is return.

@TheMarex TheMarex added this to the 5.6.0 milestone Dec 6, 2016
@TheMarex
Copy link
Member

TheMarex commented Dec 6, 2016

@fijemax Looks good. Added this to the 5.6 milestone since we have feature freeze for 5.5.

@TheMarex
Copy link
Member

This needs a rebase, current conflicts in master.

@fijemax
Copy link
Contributor Author

fijemax commented Dec 15, 2016

Hi @TheMarex,
I can test node version since Sol2 integration.
I open an issue #3452

@TheMarex
Copy link
Member

This is blocked by #3452?

@daniel-j-h
Copy link
Member

Yes - theoretically it should work, as it worked with Luabind before.

But we want to be sure #3452 is resolved first.

@fijemax fijemax force-pushed the mt-node-way-version branch 2 times, most recently from ace3f1e to c6c06cd Compare January 4, 2017 10:18
@fijemax
Copy link
Contributor Author

fijemax commented Jan 4, 2017

Hi,
@daniel-j-h @TheMarex
The #3452 is solved.
I rebased and solved conflict of this PR.

@daniel-j-h
Copy link
Member

Perfect, thanks - sorry it took so long.

Could you format your changes to scripting_environment_lua.cpp using the scripts/format.sh script (or clang-format-3.8 manually)? Then this is ready to go.

https://travis-ci.org/Project-OSRM/osrm-backend/jobs/188805267#L409-L410

Signed-off-by: FILLAU Jean-Maxime <jean-maxime.fillau@mapotempo.com>
This keep the metadata reader flag disabled by default unless explicitly specified.

Signed-off-by: FILLAU Jean-Maxime <jean-maxime.fillau@mapotempo.com>
@fijemax
Copy link
Contributor Author

fijemax commented Jan 4, 2017

it's done.

@daniel-j-h daniel-j-h merged commit 300283d into Project-OSRM:master Jan 4, 2017
@daniel-j-h
Copy link
Member

Thank you, this just landed!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants