From 764c5f5c225ef5c19d18d51bad19becec9c2c5e5 Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Sat, 1 Aug 2026 15:49:33 -0700 Subject: [PATCH] fix(xodr): preserve the five warned-and-dropped spec areas (#539) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit fmt-f2. and the legacy (§10.5.1), road / (§10.6), (§12.16), and root (chapter 15), and an outside
(§7.1). These were LOUD and lossy: one diagnostic, then permanent data loss — worse than a silent drop in one respect, because the file looked like it had been understood. A round trip flattened a non-planar carriageway, dropped a CRG-referenced surface, made the standard's own roundabout grouping unrepresentable, and lost a tram file's rail layer. All five reuse #453's capture_unmodeled() rather than adding five more walks; the change is a set of modeled-member lists plus three writer emission points. That reuse is what fmt-f1 was shaped for. The diagnostic is downgraded too, and that is half the fix. "is not supported yet and was ignored" was TRUE before and is a lie now. note_preserved() says "is preserved verbatim but not modeled; it round-trips unchanged and has no effect in this build" — a user needs the second half without being told the first, since a preserved still will not bend the carriageway. Nothing here is modeled and none of it needs to be for v0.1.0; modeling stays with whoever owns the feature later ( with #495). Fixes #539 --- CHANGELOG.md | 21 +++++ core/include/roadmaker/road/network.hpp | 17 ++++ core/include/roadmaker/road/road.hpp | 11 +++ core/src/xodr/reader.cpp | 67 ++++++++++++--- core/src/xodr/writer.cpp | 23 ++++- core/tests/fuzz/corpus/preserved_sweep.xodr | 93 +++++++++++++++++++++ core/tests/test_round_trip.cpp | 64 ++++++++++++++ 7 files changed, 280 insertions(+), 16 deletions(-) create mode 100644 core/tests/fuzz/corpus/preserved_sweep.xodr diff --git a/CHANGELOG.md b/CHANGELOG.md index a3e7e4b7..b61f8e83 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1345,6 +1345,27 @@ Current version on `main`: **0.0.1**. about silently never matched. ### Fixed +- **Five spec areas that were warned about and then thrown away now round-trip** + ([#539](https://github.com/Robomous/RoadMaker/issues/539), fmt-f2): + `` and the legacy `` (§10.5.1), road + ``/`` (§10.6), `` (§12.16), `` and root + `` (chapter 15), and an `` outside `
` (§7.1). + + These were **loud and lossy** — one diagnostic, then permanent data loss, + which is worse than a silent drop in one respect: the file looked like it had + been understood. A round trip flattened a non-planar carriageway, dropped a + CRG-referenced surface, made the standard's own roundabout grouping + unrepresentable, and lost a tram file's rail layer. + + All five now ride the preserved tier built for + [#453](https://github.com/Robomous/RoadMaker/issues/453) — reusing + `capture_unmodeled()` rather than adding five more walks — and the diagnostic + is downgraded from *"is not supported yet and was ignored"* (true before, a + lie now) to *"is preserved verbatim but not modeled; it round-trips unchanged + and has no effect in this build"*. Nothing here is modeled, and none of it + needs to be for v0.1.0; modeling stays with whoever owns the feature later + (`` with [#495](https://github.com/Robomous/RoadMaker/issues/495), + for instance). - **The last parse scopes that dropped unmodeled input in silence now preserve it** ([#453](https://github.com/Robomous/RoadMaker/issues/453), fmt-f1). The Preserved tier was established for signals, objects and controllers, but six diff --git a/core/include/roadmaker/road/network.hpp b/core/include/roadmaker/road/network.hpp index 7c00eb4f..0e46de89 100644 --- a/core/include/roadmaker/road/network.hpp +++ b/core/include/roadmaker/road/network.hpp @@ -373,6 +373,22 @@ class RoadNetwork { preserved_user_data_ = std::move(fragments); } + /// Root-level `` children RoadMaker does not model — + /// `` (§12.16), `` (chapter 15), and an `` + /// outside `
` (§7.1) — preserved as verbatim XML fragments in + /// document order (fmt-f2, #539). All three were warned about and then + /// DROPPED, so a legal file lost a roundabout grouping or a rail layer on its + /// first save. Sibling of `preserved_user_data_`, separate because the writer + /// emits the two in different places. + [[nodiscard]] const std::vector& preserved_root_children() const { + return preserved_root_children_; + } + + /// Replaces the preserved root children wholesale. Parser-only, as above. + void set_preserved_root_children(std::vector fragments) { + preserved_root_children_ = std::move(fragments); + } + private: Arena roads_; Arena sections_; @@ -386,6 +402,7 @@ class RoadNetwork { GeoReference georeference_; RawXml preserved_header_; std::vector preserved_user_data_; + std::vector preserved_root_children_; }; /// Plan-view bounding box of the whole network as {lo_x, lo_y, hi_x, hi_y}, diff --git a/core/include/roadmaker/road/road.hpp b/core/include/roadmaker/road/road.hpp index 39dd1eff..4f9b8e08 100644 --- a/core/include/roadmaker/road/road.hpp +++ b/core/include/roadmaker/road/road.hpp @@ -104,6 +104,17 @@ struct Road { /// #453). Held on the Road because `` has no struct of its own. RawXml lanes_extras; + /// Unmodeled children of `` — `` (§10.5.1) and the + /// legacy `` — preserved verbatim (fmt-f2, #539). Both were warned + /// about and dropped, so a round trip FLATTENED a non-planar carriageway. + RawXml lateral_profile_extras; + + /// Unmodeled children of `` itself — ``/`` (§10.6) and + /// `` (chapter 15) — preserved verbatim (fmt-f2, #539). A + /// CRG-referencing file used to lose its surface detail, and a tram file its + /// rail layer, on the first save. + RawXml road_extras; + /// Unmodeled children of `` — anything besides /// `` — preserved verbatim (fmt-f1, #453). Same reason: the /// profile container has no struct of its own. diff --git a/core/src/xodr/reader.cpp b/core/src/xodr/reader.cpp index 006568bd..44f958a2 100644 --- a/core/src/xodr/reader.cpp +++ b/core/src/xodr/reader.cpp @@ -206,7 +206,7 @@ class Parser { parse_surfaces(root); parse_terrain_reference(root); parse_root_user_data(root); - warn_unsupported_root_children(root); + preserve_unmodeled_root_children(root); return std::move(result_); } @@ -251,6 +251,22 @@ class Parser { } /// Unsupported-element warnings are emitted once per element name. + /// The "preserved, not modeled" note (fmt-f2, #539) — the downgrade from + /// `warn_unsupported`, which said "ignored" because the element really was + /// dropped. These elements now survive a round trip byte-for-byte; what the + /// user still needs to know is that this build assigns them no meaning, so a + /// `` will not bend the carriageway and a `` will not + /// group anything. Once per element name, like warn_unsupported. + void note_preserved(const std::string& element, const std::string& location) { + if (warned_elements_.insert("preserved:" + element).second) { + diag(Severity::Warning, + location, + fmt::format("element <{}> is preserved verbatim but not modeled; it round-trips " + "unchanged and has no effect in this build", + element)); + } + } + void warn_unsupported(const std::string& element, const std::string& location) { if (warned_elements_.insert(element).second) { diag(Severity::Warning, @@ -542,12 +558,23 @@ class Parser { } pending_refs_.push_back(std::move(pending)); + // / (§10.6) and (chapter 15) used to be warned + // about and DROPPED (fmt-f2, #539). Preserved verbatim now — a + // CRG-referencing file kept its surface detail, a tram file its rail layer. + static constexpr std::string_view kRoadChildren[] = {"planView", + "elevationProfile", + "lateralProfile", + "lanes", + "link", + "type", + "userData", + "objects", + "signals"}; + capture_unmodeled(road_node, {}, kRoadChildren, network().road(road_id)->road_extras); for (const pugi::xml_node child : road_node.children()) { - const std::string name = child.name(); - if (name != "planView" && name != "elevationProfile" && name != "lateralProfile" && - name != "lanes" && name != "link" && name != "type" && name != "userData" && - name != "objects" && name != "signals") { - warn_unsupported(name, location); + const std::string_view name = child.name(); + if (std::ranges::find(kRoadChildren, name) == std::end(kRoadChildren)) { + note_preserved(std::string(name), location); } } current_road_ = {}; @@ -792,9 +819,15 @@ class Parser { superelevation, fmt::format("{}/lateralProfile/superelevation[{}]", location, index))); ++index; } + // (§10.5.1) and the legacy used to be warned about and + // DROPPED, which flattened a non-planar carriageway on the first save + // (fmt-f2, #539). They are preserved verbatim instead; modeling stays with + // whoever owns road shape later. + static constexpr std::string_view kLateralChildren[] = {"superelevation"}; + capture_unmodeled(profile, {}, kLateralChildren, road.lateral_profile_extras); for (const pugi::xml_node child : profile.children()) { if (std::string_view(child.name()) != "superelevation") { - warn_unsupported(child.name(), location + "/lateralProfile"); + note_preserved(child.name(), location + "/lateralProfile"); } } } @@ -3534,18 +3567,26 @@ class Parser { return link; } - void warn_unsupported_root_children(const pugi::xml_node& root) { + /// Root children RoadMaker does not model — `` (§12.16), + /// `` (chapter 15) and an `` outside `
` (§7.1) — + /// are PRESERVED rather than warned-and-dropped (fmt-f2, #539). A legal file + /// used to lose its roundabout grouping or rail layer on the first save. + /// + /// Root `` carries RoadMaker extensions (rm:surface, rm:terrain); + /// they are parsed by parse_surfaces/parse_terrain_reference, and every other + /// code is preserved by parse_root_user_data (fmt-s2, #326) — so it is named + /// modeled here and never lands in this list. + void preserve_unmodeled_root_children(const pugi::xml_node& root) { + std::vector fragments; for (const pugi::xml_node child : root.children()) { const std::string_view name = child.name(); - // Root carries RoadMaker extensions (rm:surface, rm:terrain); - // they are parsed by parse_surfaces/parse_terrain_reference, and every - // other code is preserved verbatim by parse_root_user_data (fmt-s2, - // #326) — none of it is unsupported. if (name != "header" && name != "road" && name != "junction" && name != "controller" && name != "userData") { - warn_unsupported(std::string(name), "OpenDRIVE"); + fragments.push_back(node_to_string(child)); + note_preserved(std::string(name), "OpenDRIVE"); } } + network().set_preserved_root_children(std::move(fragments)); } RoadNetwork& network() { return result_.network; } diff --git a/core/src/xodr/writer.cpp b/core/src/xodr/writer.cpp index a5050860..4a634265 100644 --- a/core/src/xodr/writer.cpp +++ b/core/src/xodr/writer.cpp @@ -1363,9 +1363,13 @@ void write_road(pugi::xml_node root, append_fragment(profile, fragment); } } - if (!road.superelevation.empty()) { - write_poly3_list( - road_node.append_child("lateralProfile"), "superelevation", "s", road.superelevation); + if (!road.superelevation.empty() || !road.lateral_profile_extras.children.empty()) { + pugi::xml_node lateral = road_node.append_child("lateralProfile"); + write_poly3_list(lateral, "superelevation", "s", road.superelevation); + // /, preserved rather than dropped (fmt-f2, #539). + for (const std::string& fragment : road.lateral_profile_extras.children) { + append_fragment(lateral, fragment); + } } pugi::xml_node lanes = road_node.append_child("lanes"); @@ -1416,6 +1420,12 @@ void write_road(pugi::xml_node root, // follows in the road element sequence (1.9.0 §10.1). write_signals(road_node, network, road_id, road, options); + // Road children RoadMaker does not model — / (§10.6), + // (chapter 15) — preserved rather than dropped (fmt-f2, #539). + for (const std::string& fragment : road.road_extras.children) { + append_fragment(road_node, fragment); + } + // Authoring waypoints round-trip through the spec-sanctioned // extension (OpenDRIVE 1.9.0 §7.2: code required, value optional free // text). Emitted last so the normative children keep their order. @@ -3028,6 +3038,13 @@ Expected write_xodr(const RoadNetwork& network, append_fragment(root, fragment); } + // Root children RoadMaker does not model — (§12.16), + // (chapter 15), an outside
(§7.1) — preserved + // rather than dropped (fmt-f2, #539). + for (const std::string& fragment : network.preserved_root_children()) { + append_fragment(root, fragment); + } + std::ostringstream out; doc.save(out, " ", pugi::format_default, pugi::encoding_utf8); return std::move(out).str(); diff --git a/core/tests/fuzz/corpus/preserved_sweep.xodr b/core/tests/fuzz/corpus/preserved_sweep.xodr new file mode 100644 index 00000000..5d75ba75 --- /dev/null +++ b/core/tests/fuzz/corpus/preserved_sweep.xodr @@ -0,0 +1,93 @@ + + + +
+ + + + + + + + + + + + + + +
+ +
+ + + + + + + + + + + + + + + + + + + + + + + + + + +
+ +
+ + + + + + + + + + + + + + + + + + + + + diff --git a/core/tests/test_round_trip.cpp b/core/tests/test_round_trip.cpp index 8c667bb2..e073ae9e 100644 --- a/core/tests/test_round_trip.cpp +++ b/core/tests/test_round_trip.cpp @@ -282,6 +282,70 @@ TEST(RoundTrip, LaneDirectionSurvivesWriteParseWrite) { EXPECT_EQ(*xml, *again); } +// --- the warned-and-dropped spec areas (fmt-f2, #539) ------------------------ +// +// These were LOUD and lossy: one diagnostic, then permanent data loss. Worse +// than a silent drop in one respect — the file looked like it had been +// understood. None of them is modeled by this build; the bar is that they +// round-trip byte-for-byte and that the diagnostic says so. + +TEST(RoundTrip, TheWarnedAndDroppedScopesNowSurviveWriteParseWrite) { + const std::filesystem::path sample = + std::filesystem::path(RM_FUZZ_CORPUS_DIR) / "preserved_sweep.xodr"; + const auto loaded = roadmaker::load_xodr(sample); + ASSERT_TRUE(loaded.has_value()) << (loaded ? "" : loaded.error().message); + + const auto written = roadmaker::write_xodr(loaded->network, "preserved_sweep"); + ASSERT_TRUE(written.has_value()); + + for (const std::string_view marker : { + " (§10.5.1) + " + " (§10.6) + " child + " + }) { + EXPECT_NE(written->find(marker), std::string::npos) << "lost: " << marker; + } + // Content, not just the tag: a writer emitting an empty would pass + // a tag-only check while still having flattened the carriageway. + EXPECT_NE(written->find(R"(file="pavement.crg")"), std::string::npos); + EXPECT_NE(written->find(R"(junction="101")"), std::string::npos); + EXPECT_NE(written->find(R"(file="extra_geometry.xodr")"), std::string::npos); + + // Fixed point. + const auto reparsed = roadmaker::parse_xodr(*written, "preserved_sweep"); + ASSERT_TRUE(reparsed.has_value()); + const auto again = roadmaker::write_xodr(reparsed->network, "preserved_sweep"); + ASSERT_TRUE(again.has_value()); + EXPECT_EQ(*written, *again); + EXPECT_EQ(roadmaker::count_errors(loaded->diagnostics), 0U); +} + +TEST(RoundTrip, TheSweptScopesSayPreservedRatherThanIgnored) { + // The diagnostic is half the fix. "not supported yet and was ignored" was + // TRUE before and is a lie now; what the user still needs to know is that the + // element has no effect, not that it was thrown away. + const auto loaded = + roadmaker::load_xodr(std::filesystem::path(RM_FUZZ_CORPUS_DIR) / "preserved_sweep.xodr"); + ASSERT_TRUE(loaded.has_value()); + + const auto says = [&](std::string_view needle) { + return std::ranges::any_of(loaded->diagnostics, [&](const roadmaker::Diagnostic& d) { + return d.message.find(needle) != std::string::npos; + }); + }; + EXPECT_TRUE(says(" is preserved verbatim but not modeled")); + EXPECT_TRUE(says(" is preserved verbatim but not modeled")); + EXPECT_TRUE(says(" is preserved verbatim but not modeled")); + // And nothing claims to have ignored them any more. + EXPECT_FALSE(says(" is not supported yet and was ignored")); + EXPECT_FALSE(says(" is not supported yet and was ignored")); +} + // --- the remaining non-preserving parse scopes (fmt-f1, #453) ---------------- // // Six scopes that each dropped unmodeled input in silence. They are asserted