Skip to content

Bucket surface=laterite and surface=clay with mud/dirt/earth/ground#6204

Merged
nilsnolde merged 4 commits into
valhalla:masterfrom
julcnx:fix/surface-laterite-clay-classification
Jul 13, 2026
Merged

Bucket surface=laterite and surface=clay with mud/dirt/earth/ground#6204
nilsnolde merged 4 commits into
valhalla:masterfrom
julcnx:fix/surface-laterite-clay-classification

Conversation

@julcnx

@julcnx julcnx commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Follows up on the classification gap I flagged in #6171: surface=laterite and surface=clay don't match any branch in the surface tag handler's substring-match chain, so they fall through to has_surface_ = false and get a road-class-based default guess instead of any real surface classification.

This PR is the minimal fix: both values now match the same branch as mud/dirt/earth/groundSurface::kDirt.

A finer-grained bucket for the wet-condition-risk surfaces (mud/clay/laterite split off from the merely-firm dirt/earth/ground) was also discussed in the issue thread, but that's a separate, bigger design question (touches costing weights, not just classification) and isn't part of this PR. Not waiting on that discussion to land this smaller, uncontroversial fix.

Test plan

  • Added ParseWays.SurfaceLateriteAndClay in test/gurka/test_parse_osm.cc, asserting laterite, clay, and mud all classify to Surface::kDirt
  • Verified the exact substring-matching logic in isolation (no other existing surface values shift buckets)
  • Full project build wasn't run locally (missing spatialite-tools in this environment); relying on CI to run the gurka suite

Neither value matched any branch in the surface tag handler's
substring-match chain, so they fell through to has_surface_ = false
and got a road-class-based default guess instead of any surface
classification. Both now match the same kDirt branch as mud, which
is the minimal fix; a finer-grained wet-surface-risk bucket (mud/
clay/laterite split from dirt/earth/ground) is a separate, bigger
design question raised in the issue thread and left for later.

Addresses the classification gap discussed in valhalla#6171; the broader
wet-surface routing risk/warning discussion in that issue remains
open.
@julcnx julcnx mentioned this pull request Jul 13, 2026

@chrstnbwnkl chrstnbwnkl left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is better than assuming a paved surface of course! Thanks. Can you also update the taginfo.json?

Comment thread CHANGELOG.md Outdated
## UNRELEASED
* **Removed**
* **Bug Fix**
* FIXED: `surface=laterite` and `surface=clay` fell through the graph parser's surface substring-match chain unclassified (`has_surface_ = false`) instead of being bucketed with `dirt`/`earth`/`ground`/`mud` [#6171](https://github.com/valhalla/valhalla/issues/6171)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

let's keep it short here; also I'd move it to the enhancements section.

Suggested change
* FIXED: `surface=laterite` and `surface=clay` fell through the graph parser's surface substring-match chain unclassified (`has_surface_ = false`) instead of being bucketed with `dirt`/`earth`/`ground`/`mud` [#6171](https://github.com/valhalla/valhalla/issues/6171)
* ADDED: classify `surface=laterite` and `surface=clay` as `Surface::kDirt [#6171](https://github.com/valhalla/valhalla/issues/6171)

@julcnx

julcnx commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, updated the CHANGELOG.md wording/section as suggested.

On taginfo.json: the surface key currently has a single generic entry (no per-value entries for any of the other substrings the parser already handles, e.g. paved, gravel, dirt, mud). Did you want me to add dedicated {key: surface, value: laterite} / {key: surface, value: clay} entries (like the per-value style used for barrier), or something else? Want to match whatever convention you had in mind before I add something inconsistent with the rest of the file.

@nilsnolde

Copy link
Copy Markdown
Member

I'd like to see all values we support in the surface case, i.e. make optimal use of the taginfo.json. we've not been doing that so far, but we should be doing that going forward. it's still on our list to have copilot check those things (or at least try to).

@nilsnolde

Copy link
Copy Markdown
Member

also, you'll need to run scripts/format.sh or pre-commit install && pre-commit run --all-files.

Reviewer requested running scripts/format.sh / pre-commit before further review.
@julcnx

julcnx commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

also, you'll need to run scripts/format.sh or pre-commit install && pre-commit run --all-files.

Done. Committed.

I'd like to see all values we support in the surface case, i.e. make optimal use of the taginfo.json.

That'd be valuable, would it be ok to do it as a separate follow-up PR though? taginfo.json currently has no value-level entries for surface at all, not just for laterite/clay, so covering it properly means mapping all ~24 substrings across the 7 Surface buckets in pbfgraphparser.cc, which felt bigger than this PR's scope. Happy to open that once this one merges. Is this ok with you?

@nilsnolde

Copy link
Copy Markdown
Member

That'd be valuable, would it be ok to do it as a separate follow-up PR though?

yep, I agree! we can have claude do a scan of 10-20 taginfo.json at each PR. dogfood it until it's done in a few months. no urgency. but it'd be nice if we kept that maximally useful.

@chrstnbwnkl

Copy link
Copy Markdown
Member

re: taginfo.json, that's fine with me. I hadn't checked how we kept surface in there, so if it's already not on a per value basis, it shouldn't be this PR's scope to change that. One last thing: the changelog entries go at the bottom of the section, not at the top 😅 then it's good to merge!

@julcnx

julcnx commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

Fixed, moved the CHANGELOG entry to the bottom of the Enhancement section. Thanks for the review!

@nilsnolde
nilsnolde enabled auto-merge (squash) July 13, 2026 11:25
@nilsnolde
nilsnolde merged commit 4b85f20 into valhalla:master Jul 13, 2026
17 of 18 checks passed
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.

4 participants