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.

Sign up for GitHub

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

Jump to bottom

Move base costing options parsing into a separate function #3125

Merged
merged 8 commits into from
Jun 7, 2021
Merged

Move base costing options parsing into a separate function #3125

merged 8 commits into from
Jun 7, 2021

Conversation

genadz
Copy link
Contributor

@genadz genadz commented Jun 2, 2021

Issue

#3102

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.

Requirements / Relations

Link any requirements here. Other pull requests this PR is based on?

@genadz genadz mentioned this pull request Jun 2, 2021
Closed
genadz
genadz commented Jun 2, 2021
View reviewed changes
src/sif/dynamiccost.cc
@@ -294,8 +355,123 @@ void ParseSharedCostOptions(const rapidjson::Value& value, CostingOptions* pbf_c
pbf_costing_options->set_shortest(rapidjson::get<bool>(value, "/shortest", false));
pbf_costing_options->set_top_speed(
kVehicleSpeedRange(rapidjson::get<uint32_t>(value, "/top_speed", kMaxAssumedSpeed)));
pbf_costing_options->set_closure_factor(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

moved to ParseBaseCostOptions

genadz
genadz commented Jun 2, 2021
View reviewed changes
src/sif/pedestriancost.cc
@@ -471,7 +458,6 @@ class PedestrianCost : public DynamicCost {
c += country_crossing_cost_ * (node->type() == baldr::NodeType::kBorderControl);
c += gate_cost_ * (node->type() == baldr::NodeType::kGate);
c += bike_share_cost_ * (node->type() == baldr::NodeType::kBikeShare);
c += toll_booth_cost_ * (node->type() == baldr::NodeType::kTollBooth);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we don't parse tool_booth_cost option for pedestrians, so it doesn't make sense to use it here

mandeepsandhu
mandeepsandhu reviewed Jun 2, 2021
View reviewed changes
valhalla/sif/dynamiccost.h Outdated
@@ -1037,13 +1026,70 @@ class DynamicCost {
using cost_ptr_t = std::shared_ptr<DynamicCost>;
using mode_costing_t = std::array<cost_ptr_t, static_cast<size_t>(TravelMode::kMaxTravelMode)>;

/*
* Structure that stores default values for costing options that are common for most costing models.
* It mostly contains options used in DynamicCost::gate_base_costs() method.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/gate_base_costs/get_base_costs

@genadz genadz requested a review from mandeepsandhu June 4, 2021 10:38
@kevinkreiser
Copy link
Member

@genadz sorry i havent looked at this yet. i will schedule it for over the weekend.

@genadz
Copy link
Contributor Author

genadz commented Jun 4, 2021

@genadz sorry i havent looked at this yet. i will schedule it for over the weekend.

thank you!

mandeepsandhu
mandeepsandhu approved these changes Jun 4, 2021
View reviewed changes
Copy link
Contributor

@mandeepsandhu mandeepsandhu left a comment

Choose a reason for hiding this comment

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

Thanks for the refactor @genadz! This will make working with costings much easier!

This lgtm. We should wait for @kevinkreiser 's feedback before merging it in.

@kevinkreiser
Copy link
Member

no need to wait for me! judging by the magnitude of the change and the importance id just suggest that we RAD it if we havent already just to be sure. code motion copy pasting type stuff is easy to make a typo imho

kevinkreiser
kevinkreiser approved these changes Jun 6, 2021
View reviewed changes
Copy link
Member

@kevinkreiser kevinkreiser left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looks good just a RAD of each costing would be good. Although maybe we can do without that with a more mechanical test. Like a loop over all costings over all costing args? I know we have the inline costing tests but I forget if they are exhastive

@merkispavel
Copy link
Contributor

merkispavel commented Jun 7, 2021 •
edited
Loading

I RAD tested the changes - no failures 🟢

@genadz
Copy link
Contributor Author

genadz commented Jun 7, 2021

I RAD tested the changes - no failures 🟢

Thank you! Could you merge it?

@merkispavel merkispavel merged commit c9b2cc3 into master Jun 7, 2021
@genadz genadz deleted the kgv_refactor_cost_opts branch June 15, 2021 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevinkreiser kevinkreiser kevinkreiser approved these changes

@mandeepsandhu mandeepsandhu mandeepsandhu approved these changes

@SvetlanaBayarovich SvetlanaBayarovich Awaiting requested review from SvetlanaBayarovich

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@genadz @kevinkreiser @merkispavel @mandeepsandhu