valhalla icon indicating copy to clipboard operation
valhalla copied to clipboard

Add helpers for DirectedEdgeExt and save them to file in GraphTileBuilder

Open MehdiSaffar opened this issue 3 years ago • 5 comments

Issue

This PR adds helpers in GraphTile and GraphTileBuilder to manipulate DirectedEdgeExt lists as easily as with regular DirectedEdge.

It also writes the DirectedEdgeExt data to the tile if header->has_ext_directededge_ is true.

NOTE: I didn't make changes to GraphTileBuilder::Update function as I was not sure whether to create two signatures, one that takes only a list of directed edges and assumes there are no new extensions and another that takes both edges and edge extensions and writes them together OR have an optional argument of the list of directed edge extensions...

I am not very familiar with the codebase and I haven't touched C++ in a long while so please be indulgent :)

Tasklist

  • [ ] Add tests
  • [ ] Add #fixes with the issue number that this PR addresses
  • [ ] Update the docs with any new request parameters or changes to behavior described
  • [ ] Update the changelog
  • [ ] If you made changes to the lua files, update the taginfo too.

MehdiSaffar avatar Mar 08 '22 10:03 MehdiSaffar

@MehdiSaffar everything i see here looks good! Regarding the GraphTileBuilder interface.. I was thinking maybe we would just add a new method called AddDirectedEdgeExt (or whatever naming matches the other naming conventions). This way someone coudl write their own valhalla_add_directed_edge_ext binary and add the data with that method. If you want we can tackle this in a subsequent pr or I'm fine adding it to this PR if you are game for that. Let me know and I can describe what needs to be done. Thanks again!

kevinkreiser avatar Mar 08 '22 16:03 kevinkreiser

@kevinkreiser Thanks a lot for your feedback! If you believe the logic for AddDirectedEdgeExt is easy and straightforward, we can add it to this PR, otherwise, we can merge this PR and create another one for it. Your call :)

EDIT 1: In directededge.h I noticed that some attributes were uint32_t while others were uint64_t. Is there a specific reason for it? Or does it make no difference?

image

MehdiSaffar avatar Mar 09 '22 09:03 MehdiSaffar

@MehdiSaffar yep there is a specific reason, basically we are using a feature of C called bitfields where you spread values across a single integral types, claiming sections of its bits for separate fields. this is a very important concept in how our internal format and many other formats like (flatbuffer and capnproto) achieve a parseless datastructure that can be directly memory mapped. i would suggest just reading up a bit on bitfields and how they work its a wonderful technique for packign as much info into some bytes as possible :smile:

kevinkreiser avatar Mar 09 '22 14:03 kevinkreiser

@MehdiSaffar i think we shoudl merge what you have. but it seems to me your fork is configured such that i cannot make edits (which is fine). something seems to be wrong with CI though. can you please push a commit with a changelog message. it should go on the enhancement section at the bottom of the current unreleased changes.

kevinkreiser avatar Mar 09 '22 14:03 kevinkreiser

@kevinkreiser My apologies for the delay, I have been quite busy lately. I added an entry to CHANGELOG.md as requested.

MehdiSaffar avatar Mar 21 '22 08:03 MehdiSaffar

superseded by #3791

nilsnolde avatar Oct 12 '22 14:10 nilsnolde