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

add access restrictions to /locate response #4431

Merged
merged 9 commits into from
Dec 14, 2023

Conversation

nilsnolde
Copy link
Member

While debugging some issue I was missing the access restrictions in the /locate response. I added most, except the time-specific restrictions, there it's a "value": null in the response. Adding time domains would add a bit more code. I'd leave it out for now and add it in a separate PR if we want that too.

@nilsnolde
Copy link
Member Author

nilsnolde commented Dec 3, 2023

Looks like this in the response:

		"edges": [
			{
				"access_restrictions": [
					{
						"truck": true,
						"pedestrian": false,
						"wheelchair": false,
						"taxi": false,
						"HOV": false,
						"emergency": false,
						"value": 3.50,
						"motorcycle": false,
						"car": false,
						"moped": false,
						"bus": false,
						"edge_index": 28162,
						"type": "max_weight"
					}
				],
...

It's quite verbose, but I find it pretty useful to debug truck restrictions.

Comment on lines 82 to 83
// TODO(nils): turn the time domain into a map
map->emplace("value", nullptr);
Copy link
Member

Choose a reason for hiding this comment

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

i would wait on this. it would be better to either just do this now or let this out of the pr to avoid any api breakage questioning

Copy link
Member Author

@nilsnolde nilsnolde Dec 12, 2023

Choose a reason for hiding this comment

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

this is more work than anticipated.. basically we'd have to reverse the parsing process of OSM tags and output smth similar to it to make it (intuitively) usable

    // Mo-Fr 06:00-11:00,17:00-19:00; Sa 03:30-19:00
    // Apr-Sep: Mo-Fr 09:00-13:00,14:00-18:00; Apr-Sep: Sa 10:00-13:00
    // Mo,We,Th,Fr 12:00-18:00; Sa-Su 12:00-17:00
    // Feb#16-Oct#15 09:00-18:30; Oct#16-Nov#15: 09:00-17:30; Nov#16-Feb#15: 09:00-16:30

as you can imagine that'd be ugly code. and a lot of it. making it saner in the code would make it much less usable or very big as response.

but I'd honestly still like to know if there's a time-based restriction, even the value is not provided. if anyone would ever tackle this, it'll need to be a (nested) map for the value for sure. so IMHO providing an empty map right now would still be fine for the future. we could also leave out the value property, then those time-based ones would show up but not have that field at all (but all other restrictions would). I'd prefer the empty map, but not too strongly.

EDIT: well, I argued against myself there, ideally it'd be a string as in OSM, not a map, but that would reverse this function:

std::vector<uint64_t> get_time_range(const std::string& str) {
. Urgh.. Then just leave out the value field in the response?

wdyt @kevinkreiser ?

Copy link
Member

Choose a reason for hiding this comment

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

who wrote this....oh wait nm ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

haha this time shit is just so disgusting to work with..

Copy link
Member

Choose a reason for hiding this comment

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

yes it was not fun at all

Copy link
Member

Choose a reason for hiding this comment

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

could be the tiniest bit more fancy and actually look at the type bit and serialize accordingly

Copy link
Member

Choose a reason for hiding this comment

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

actually dont we have to turn this into a date_time object when we check it in dynamic cost? can we do the same thing that that does, turn begin and end into date times for the YMD case and just show the human strings for that.

for the day of the week case i guess we'd just want the day name for start and end and the time? surely there are clues in costing on how its interpreted but also we could just print out each field and call it done

Copy link
Member

@kevinkreiser kevinkreiser Dec 12, 2023

Choose a reason for hiding this comment

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

a compromise would be to have a map with just the raw value in it:

{
  "time_domain": 23049872312
}

and then down the road we can add "begin" and "end" date time strings when we have the time (pun intended)

Copy link
Member

Choose a reason for hiding this comment

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

Also, we should grab from ComplexRestriction because we do toss malformed input from the user. We may not toss everything as parts could be correct. We do also toss unsupported types like PH for public holidays.

Copy link
Member Author

Choose a reason for hiding this comment

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

10 mins is vastly exaggerated for any sort of half-usable output, but yeah prob not more than 2-3 hours with some fanciness added. I'm also fine with the compromise of returning the raw value. we can keep it forever as well, even when we add the actual map. let's do that then for now. the next weeks should be pretty calm, maybe I find the motivation to flesh it out a bit more.

kevinkreiser
kevinkreiser previously approved these changes Dec 14, 2023
Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>
kevinkreiser
kevinkreiser previously approved these changes Dec 14, 2023
Co-authored-by: Kevin Kreiser <kevinkreiser@gmail.com>
@nilsnolde nilsnolde merged commit ac2c5d9 into master Dec 14, 2023
7 of 9 checks passed
@nilsnolde nilsnolde deleted the nn-accessrestrictions-locate branch December 14, 2023 23:32
chrstnbwnkl pushed a commit to chrstnbwnkl/valhalla that referenced this pull request Dec 15, 2023
chrstnbwnkl pushed a commit to chrstnbwnkl/valhalla that referenced this pull request Dec 15, 2023
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.

3 participants