added vignette as access restriction #5722

enishettyrahul · 2025-11-18T12:22:34Z

Add vignette as edge attribute.

What issue is this PR targeting? If there is no issue that addresses the problem, please open a corresponding issue and link it here.
This to differentiate between toll and vignette costing.

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
If you made changes to a translation file, update transifex too

Requirements / Relations

Rahul Krishna Enishetty [email protected], Mercedes-Benz Tech Innovation GmbH
Provider Information

Licensed under MIT

enishettyrahul · 2025-11-18T12:46:03Z

This PR is created to restriction the vignette access, if the user prefers to avoid it.

The Information to be passed via isochrone as an Array.

We can do a soft avoid by providing only a single country for avoidance.
For ex: ["CH"]

We can also do Hard avoid by Providing ["ALL"] as a parameter to exclude_country_vignettes as vehicle costing.

chrstnbwnkl · 2025-11-18T12:56:05Z

valhalla/baldr/accessrestriction.h

  uint64_t modes_ : 12;             // Mode(s) this access restriction applies to
  uint64_t except_destination_ : 1; // Whether local traffic is exempted from this restriction
-  uint64_t spare_ : 23;
+  uint64_t countryIsoCode_ : 16;    // Country ISO Code as a numeric value


I'm torn about whether this is actually necessary at all to store this information. When expanding, we should probably have the begin node's admin index, we'd just need to pass it to DynamicCost::Allowed.

If we decide to store it on the restriction though, it should go into value_ because it's specific to this type of restriction. You can also limit it to 16 bits there to reserve more bits on the value to get more specific (regional, or store provider info in countries where highways are privately owned etc.).

We have been interested in the possibility of treating countries in cost profiles for quite some time, but unfortunately the PR at that time got a little out of date 4289. As noted in the issue discussion on this PR, we now have more know how and want to revisit this as soon as we have more capacity available.

We also considered whether to store the country in the value_ or in a new attribute. The reason we decided on a new attribute was that this would allow us to store more complex data in our value_, such as the cost of the vignette or other country-specific information we may want in the future (not only for vignettes), while maintaining good readability in the code. What do you think, should we still move the country to the value_ or keep the countryIsoCode_?

@chrstnbwnkl Did you already have the change to check the PR?

I think I want to hear other maintainers' thoughts on this as well, this is a non-trivial design decision and I would like to make a decision that will not render us deadlocked in the future.

Maybe you could explain in a little more detail how you are planning to use the country information for types other than vignette? That might help in guiding that decision.

Hi @chrstnbwnkl

I have changed the code now to use the Value varible in AccessRestriction instead of new CountryIso Variable.

Could you please review the changes for the feature.?

Some changes like in test_elevation.cc are added after calling ./scripts/clang-tidy-only-diff.sh

chrstnbwnkl · 2025-11-18T12:59:10Z

valhalla/sif/dynamiccost.h

+            break;
+          }
+        }
+        return doNotAvoidVignette;


This shouldn't return early: this will cause timed access restrictions to not be evaluated if there was also a vignette flag but it didn't match the countries provided by the user (crazy edge case but still). Just return false if it matches, otherwise continue with the remaining restrictions.

Also, can you stick to snake_casing here and make it do_not_avoid_vignette?

Done. Now when the country it found out then return is called.

chrstnbwnkl

Looks mostly good! Just a few nitpicks

chrstnbwnkl · 2025-11-18T16:29:47Z

src/baldr/accessrestriction.cc

      break;
+    case AccessType::kVignette:
+      writer("value", countryIsoCode());
+      writer("country_iso", countryIsoCode());


chrstnbwnkl · 2025-11-18T16:34:17Z

src/mjolnir/util.cc

    osm_data = PBFGraphParser::ParseWays(config.get_child("mjolnir"), input_files, ways_bin,
                                         way_nodes_bin, access_bin);
-    osm_data.pbf_checksum_ = get_pbf_checksum(input_files, tile_dir);
+    osm_data.pbf_checksum_ = get_pbf_checksum(input_files);


looks like a merging error

they appeared after the call of ./scripts/clang-tidy-only-diff.sh

chrstnbwnkl · 2025-11-18T16:35:32Z

src/sif/dynamiccost.cc

+  // parse from JSON array and set on costing options protobuf.
+  auto exclude_vignettes_node = rapidjson::get_child_optional(json, "/exclude_country_vignettes");
+  auto country_vignettes = co->mutable_exclude_country_vignettes();
+  country_vignettes->Clear();


nit: is it necessary to clean this?

It is best to clear, so that only new request is considered always.

chrstnbwnkl · 2025-11-18T16:38:59Z

test/elevation_builder.cc


 struct TestableSample : public valhalla::skadi::sample {
-  static uint16_t get_tile_index(const PointLL& coord) {
+  static uint16_t get_tile_index(const valhalla::midgard::PointLL& coord) {


can we replace all of this with a using namespace ...?

they appeared after the call of ./scripts/clang-tidy-only-diff.sh

#5488 (comment)
test/elevation_builder.cc does not compile because it gets left out during refactorings because it is not enabled.
Probably caused by #5568, makes sense to just add the using statement in this file.

chrstnbwnkl · 2025-11-18T16:40:13Z

valhalla/sif/dynamiccost.h

+  std::string iso;
+  iso.push_back(static_cast<char>((countryIsoCode >> 8) & 0xFF));
+  iso.push_back(static_cast<char>(countryIsoCode & 0xFF));
+  LOG_INFO("Country ISO: " + iso + " Value in Int: " + std::to_string(countryIsoCode));


remove log statement

chrstnbwnkl · 2025-11-18T16:42:39Z

valhalla/sif/dynamiccost.h

+      if (restriction.type() == baldr::AccessType::kVignette) {
+        auto countryIso = ConvertIntToISO(restriction.countryIsoCode());
+        for (auto v : exclude_country_vignettes_) {
+          if ((v == countryIso) || (v == "ALL")) {


capitalized looks a bit weird here, maybe call it "all" and make it constexpr at the top of the file?

Fixed with use of small or capital letters now.

Thanks for mentioning the check for Capitalization.

chrstnbwnkl

A few things:

code style: please don't use camel or pascal case, use snake case, except for compile time constants, which should be kMyConstanst
looks like you reverted the update to the date third party lib

chrstnbwnkl · 2025-12-11T16:17:48Z

src/mjolnir/graphbuilder.cc

-                                           r->second.value(), r->second.except_destination());
+      auto value = r->second.value();
+      if (r->second.type() == AccessType::kVignette) {
+        value = CountryISOCodeToValue(boost::to_upper_copy(countryIso));


to_upper_copy is not necessary here because we get that string from the tile's admin data

chrstnbwnkl · 2025-12-11T16:18:47Z

src/mjolnir/graphbuilder.cc

+              const Admin& admin = graphtile.admins_builder(admin_index);
+              countryIso = admin.country_iso();
+            } catch (...) {
+              LOG_ERROR("admin_index size is greater than admin count in Graphbuilder");


this try catch seems superfluous

chrstnbwnkl · 2025-12-11T16:20:38Z

valhalla/sif/dynamiccost.h

+      // Special check for vignette restrictions
+      if (restriction.type() == baldr::AccessType::kVignette) {
+        auto countryIso = ConvertIntToISO(restriction.value());
+        std::transform(countryIso.begin(), countryIso.end(), countryIso.begin(), ::toupper);


same here, transforming to upper should not be necessary, right?

chrstnbwnkl · 2025-12-11T16:22:07Z

valhalla/sif/dynamiccost.h

  bool is_hgv_{false};

+  // List of country vignettes to exclude
+  std::vector<std::string> exclude_country_vignettes_;


this should probably be a std::unordered_set so look ups are fast

chrstnbwnkl · 2025-12-11T16:24:17Z

valhalla/sif/dynamiccost.h

+        auto countryIso = ConvertIntToISO(restriction.value());
+        std::transform(countryIso.begin(), countryIso.end(), countryIso.begin(), ::toupper);
+        for (auto v : exclude_country_vignettes_) {
+          if ((v == countryIso) || (v == HardAvoidVignette)) {


(v == HardAvoidVignette) should be checked before pulling out the iso string

Signed-off-by: Rahul Krishna Enishetty <[email protected]>

enishettyrahul · 2025-12-19T14:33:46Z

Hallo @chrstnbwnkl ,

Could you please review this ?
Please set all the comments to resolved that you feel are completed.

enishettyrahul mentioned this pull request Nov 18, 2025

Add vignette as attribute #5596

Closed

6 tasks

chrstnbwnkl reviewed Nov 18, 2025

View reviewed changes

chrstnbwnkl requested changes Nov 18, 2025

View reviewed changes

enishettyrahul force-pushed the vignette_as_access_restriction branch 2 times, most recently from 5eedb5a to 5f5e9ec Compare December 9, 2025 15:39

chrstnbwnkl requested changes Dec 11, 2025

View reviewed changes

enishettyrahul force-pushed the vignette_as_access_restriction branch from ea619ca to 4c9a00b Compare December 12, 2025 12:15

added vignette as access restriction

950a37f

Signed-off-by: Rahul Krishna Enishetty <[email protected]>

enishettyrahul force-pushed the vignette_as_access_restriction branch from 4c9a00b to 950a37f Compare December 12, 2025 12:29

enishettyrahul added 2 commits December 15, 2025 15:27

Merge branch 'master' into vignette_as_access_restriction

bf136b7

Added Vignette as access restriction

255baa4

Signed-off-by: Rahul Krishna Enishetty <[email protected]>

added vignette as access restriction #5722

Are you sure you want to change the base?

added vignette as access restriction #5722

Conversation

enishettyrahul commented Nov 18, 2025

Tasklist

Requirements / Relations

Uh oh!

enishettyrahul commented Nov 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enishettyrahul Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrstnbwnkl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chrstnbwnkl left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

enishettyrahul Dec 9, 2025 •

edited

Loading