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

make hgv-only edges also driveable to include in the reclassification logic #3860

Merged
merged 23 commits into from
Dec 15, 2022

Conversation

nilsnolde
Copy link
Member

fixes #3731

I'm not entirely sure if this is fine for all downstream logic.. it seems not too much depends on this directly or indirectly. If someone has a concern, please voice it.

Also, again, IMO including truck here is enough, no need to respect more modes like motorbike.

gknisely
gknisely previously approved these changes Dec 7, 2022
Copy link
Member

@kevinkreiser kevinkreiser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe I'm wrong but I think this is too broad. Let's discuss

@nilsnolde
Copy link
Member Author

Might very well be.. Like I said, didn't look into it that much. Wanted to do that tmrw with you guys. Just wanted to get a quick check if the tests at least pass. Not that it means too much, but at least that:)

@nilsnolde
Copy link
Member Author

So, @kevinkreiser is right, this is too naive. We'll have to do the second pass he mentions, when we find that the first pass found a path that's excluding certain modes. So effectively we'll find a second path to up-class (or even more depending on how many modes are missing).

@nilsnolde nilsnolde marked this pull request as draft December 12, 2022 08:57
@nilsnolde
Copy link
Member Author

This is finished. I'll comment more in the PR.

Comment on lines +193 to +196
if (node == (dt == Options::arrive_by ? "targets" : "sources")) {
for (auto& loc : locations) {
loc.set_date_time(dt == Options::current ? "current" : options.date_time());
}
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just removing a few lines

Comment on lines +6 to +7
class FerryTest : public ::testing::TestWithParam<std::string> {
protected:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this a parameterized test class with all vehicular modes as parameters

@nilsnolde nilsnolde marked this pull request as ready for review December 12, 2022 17:21
@nilsnolde
Copy link
Member Author

I first committed the test with 79f6ea0, so it's easy to just run it with current master.

@nilsnolde
Copy link
Member Author

Hm.. Wanted to run it on the test data of #3731 but on a 200 MB PBF took > 5 mins reclassifying ferry edges before I canceled it.. The whole planet takes that long with master.. Must somehow get stuck in a loop, can't see where though.. We can look at it together tmrw.

Comment on lines +73 to +74
uint16_t fwd_access;
uint16_t rev_access;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I removed the driveableforward & driveablereverse booleans and replaced all logic depending on them with the access masks which are only set if there's no Use::EmergencyAccess and has access to a vehicular mode.

I guess these would fit still into the attributes bit field, now that there's 25 bits available, though then only 1 would be left (access has 12 bits in directed edges) and we'd need to re-write to this if we needed more attributes. So I'd leave it this way.

auto edge_itr = edges[node.start_of];
auto edge = *edge_itr;
auto edge = *edges[node.start_of];
uint16_t auto_fwd_access = edge.fwd_access & baldr::kAutoAccess;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to bool

kevinkreiser
kevinkreiser previously approved these changes Dec 15, 2022
@nilsnolde
Copy link
Member Author

OSX failed on the mapmatch test for some reason.. can't reproduce locally, so I kicked CI again

@nilsnolde nilsnolde merged commit e8ca92b into master Dec 15, 2022
@nilsnolde nilsnolde deleted the nn-hgv-only-reclassification branch December 15, 2022 16:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Truck - weird ferry issue
3 participants