From 4a68ae2395a6ed8fec5e3581f9157d50faa01acd Mon Sep 17 00:00:00 2001 From: Jesus Armando Anaya Date: Sat, 1 Aug 2026 18:14:46 -0700 Subject: [PATCH] fix(xodr): round-trip virtual-junction linkage and connection internals (#537) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three holes, all in MODELED scopes that #453's preservation sweep deliberately does not cover. @elementS/@elementDir were read nowhere. asam.net:xodr:1.7.0:road.linkage.virtjunc_link_attribute_usage REQUIRES both on a pointing at a virtual junction; read_link took only elementType/elementId/contactPoint, with no diagnostic, so a conformant file lost its virtual-junction linkage on the first save. They ride RoadLink now and are written back only when the source carried them — an ordinary road link, where the standard forbids them, stays byte-identical. A virtual junction's own s were DELETED. §12.7 permits them, but arms-xor-spans cleared the list: warned, then destroyed. They are preserved instead — held out of `connections` so nothing tries to build geometry from them (arms-xor-spans still holds for GENERATION, which is what it is actually about) and re-emitted unchanged. @type and non- children were dropped. @id regeneration is benign because the writer renumbers deterministically; @type is not. Built on #534's junction test scaffolding, as the issue intended. Fixes #537 --- CHANGELOG.md | 19 +++++ core/include/roadmaker/road/junction.hpp | 22 ++++++ core/include/roadmaker/road/road.hpp | 13 ++++ core/src/xodr/reader.cpp | 41 +++++++++- core/src/xodr/writer.cpp | 48 ++++++++++++ .../fuzz/corpus/virtual_junction_links.xodr | 75 +++++++++++++++++++ core/tests/test_round_trip.cpp | 75 +++++++++++++++++++ 7 files changed, 290 insertions(+), 3 deletions(-) create mode 100644 core/tests/fuzz/corpus/virtual_junction_links.xodr diff --git a/CHANGELOG.md b/CHANGELOG.md index 49d7203..ae1a810 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1370,6 +1370,25 @@ Current version on `main`: **0.0.1**. about silently never matched. ### Fixed +- **Virtual-junction linkage and connection internals round-trip** + ([#537](https://github.com/Robomous/RoadMaker/issues/537)). Three holes, all + in modeled scopes that #453's preservation sweep deliberately does not cover: + + - **`@elementS`/`@elementDir` were read nowhere.** + `asam.net:xodr:1.7.0:road.linkage.virtjunc_link_attribute_usage` *requires* + both on a `` pointing at a virtual junction, and `read_link` took only + `elementType`/`elementId`/`contactPoint` — with no diagnostic. A conformant + file lost its virtual-junction linkage on the first save. They are carried + on `RoadLink` now and written back only when the source had them, so an + ordinary road link stays byte-identical. + - **A virtual junction's own ``s were deleted.** §12.7 permits + them, but the arms-xor-spans policy cleared the list — warned, then + destroyed. They are preserved instead: held out of `connections` so nothing + tries to build geometry from them (arms-xor-spans still holds for + *generation*), and re-emitted unchanged. + - **`` `@type` and non-`` children were dropped.** + `@id` regeneration is benign because the writer renumbers deterministically; + `@type` carries meaning the file owns. - **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 diff --git a/core/include/roadmaker/road/junction.hpp b/core/include/roadmaker/road/junction.hpp index d91f9dd..c4bdd05 100644 --- a/core/include/roadmaker/road/junction.hpp +++ b/core/include/roadmaker/road/junction.hpp @@ -114,6 +114,18 @@ struct JunctionConnection { /// Pairs of {incoming lane odr_id, connecting lane odr_id}. std::vector> lane_links; + + /// `@type` — `"default"` or `"virtual"` on a connection (§12.3, Table 61), + /// kept verbatim. Empty ⇒ absent. Dropped before #537: `@id` regeneration is + /// benign because the writer renumbers deterministically, but `@type` carries + /// meaning the file owns. + std::string type_str; + + /// Unknown attributes and unmodeled children of `` — anything + /// besides `` — preserved verbatim (#537). The reader iterated + /// `children("laneLink")` only, so a ``/`` on a + /// virtual-junction connection, or any later addition, vanished silently. + RawXml preserved; }; /// An authored override for the pavement fillet at one junction corner — the @@ -492,6 +504,16 @@ struct Junction { /// merely defaulted. std::string type_str; + /// Connections a VIRTUAL junction declared (§12.7), held aside rather than + /// deleted (#537). + /// + /// arms-xor-spans still holds for GENERATION: a span junction never cuts its + /// main road, so there is nothing to derive and these must stay out of + /// `connections` or the mesher would try. But a virtual junction may legally + /// declare them, and clearing the list destroyed a conformant file's data. + /// The writer re-emits these verbatim. + std::vector preserved_connections; + /// `` elements of a **direct** junction (§12.4). Non-empty only /// when `type == JunctionType::Direct`; a direct junction's connections have /// no connecting road, so they cannot live in `connections` and the derived diff --git a/core/include/roadmaker/road/road.hpp b/core/include/roadmaker/road/road.hpp index 4f9b8e0..d9626cc 100644 --- a/core/include/roadmaker/road/road.hpp +++ b/core/include/roadmaker/road/road.hpp @@ -42,6 +42,19 @@ enum class ContactPoint { struct RoadLink { std::variant target; ContactPoint contact = ContactPoint::Start; + + /// `@elementS` — where on the linked element the connection sits [m]. + /// Required alongside `@elementDir` when the link points at a **virtual** + /// junction (`asam.net:xodr:1.7.0:road.linkage.virtjunc_link_attribute_usage`, + /// §10 road linkage) and absent otherwise. Unset ⇒ the attribute was absent, + /// so it round-trips absent (#537). + std::optional element_s; + + /// `@elementDir` — `"+"` or `"-"`, the direction along the linked element. + /// Kept verbatim: it is a two-value enumeration whose spelling is the whole + /// content, and an unknown value must survive rather than be normalised. + /// Empty ⇒ absent. + std::string element_dir; }; /// Plan-view waypoint [m] for clothoid path fitting (authoring API and the diff --git a/core/src/xodr/reader.cpp b/core/src/xodr/reader.cpp index 44f958a..58bfc76 100644 --- a/core/src/xodr/reader.cpp +++ b/core/src/xodr/reader.cpp @@ -2375,8 +2375,19 @@ class Parser { if (!junction.arms.empty() || !junction.connections.empty()) { diag(Severity::Warning, location, - "virtual junction also declares connections or rm:arms; they were dropped", + "virtual junction also declares connections or rm:arms; RoadMaker " + "generates no geometry for them, but they are preserved and " + "re-emitted unchanged", rules::kJunctionVirtualAttributes); + // arms-xor-spans still holds for GENERATION — a span junction never + // cuts its main road, so there is nothing to derive. But a virtual + // junction may legally declare connections (§12.7), and deleting them + // destroyed a conformant file's data (#537). They are moved to the + // preserved tier instead: absent from `connections` so nothing tries to + // build geometry from them, and re-emitted verbatim on write. + for (const JunctionConnection& connection : junction.connections) { + junction.preserved_connections.push_back(connection); + } junction.arms.clear(); junction.connections.clear(); } @@ -2459,6 +2470,14 @@ class Parser { result.lane_links.emplace_back(lane_link.attribute("from").as_int(), lane_link.attribute("to").as_int()); } + // @type carries meaning the file owns (§12.3 Table 61); @id does not, + // because the writer renumbers connections deterministically. Unknown + // attributes and non- children ride the preserved tier (#537). + result.type_str = connection.attribute("type").value(); + static constexpr std::string_view kConnectionAttrs[] = { + "id", "incomingRoad", "connectingRoad", "contactPoint", "type"}; + static constexpr std::string_view kConnectionChildren[] = {"laneLink"}; + capture_unmodeled(connection, kConnectionAttrs, kConnectionChildren, result.preserved); junction.connections.push_back(std::move(result)); } } @@ -3480,7 +3499,9 @@ class Parser { struct PendingLink { std::string element_type; // "road" | "junction" std::string element_id; - std::string contact_point; // "start" | "end" (roads only) + std::string contact_point; // "start" | "end" (roads only) + std::optional element_s; // virtual junctions only (§10) + std::string element_dir; // "+" | "-", virtual junctions only bool present = false; }; @@ -3495,12 +3516,21 @@ class Parser { if (!node) { return {}; } - return PendingLink{ + PendingLink link{ .element_type = node.attribute("elementType").value(), .element_id = node.attribute("elementId").value(), .contact_point = node.attribute("contactPoint").value(), + .element_dir = node.attribute("elementDir").value(), .present = true, }; + // @elementS/@elementDir are REQUIRED when the link points at a virtual + // junction (asam.net:xodr:1.7.0:road.linkage.virtjunc_link_attribute_usage) + // and were read nowhere before #537, so a conformant file lost its + // virtual-junction linkage on save with no diagnostic. + if (const pugi::xml_attribute element_s = node.attribute("elementS")) { + link.element_s = element_s.as_double(); + } + return link; } void resolve_references() { @@ -3564,6 +3594,11 @@ class Parser { rules::kRoadLinkAttributeUsage); return std::nullopt; } + // Carried for every link kind, though only a virtual junction may legally + // use them: preserving what the file wrote beats silently normalising it, + // and the writer emits them iff they were present (#537). + link.element_s = pending.element_s; + link.element_dir = pending.element_dir; return link; } diff --git a/core/src/xodr/writer.cpp b/core/src/xodr/writer.cpp index 4a63426..8445c1f 100644 --- a/core/src/xodr/writer.cpp +++ b/core/src/xodr/writer.cpp @@ -390,6 +390,15 @@ void write_link_element(pugi::xml_node link, node.append_attribute("elementType").set_value("junction"); node.append_attribute("elementId").set_value(element_id.c_str()); } + // @elementS/@elementDir: required for a link into a VIRTUAL junction + // (asam.net:xodr:1.7.0:road.linkage.virtjunc_link_attribute_usage) and absent + // otherwise, so they are written iff the source carried them (#537). + if (road_link.element_s.has_value()) { + set_num(node, "elementS", *road_link.element_s); + } + if (!road_link.element_dir.empty()) { + node.append_attribute("elementDir").set_value(road_link.element_dir.c_str()); + } } void write_lane(pugi::xml_node side, const Lane& lane) { @@ -1658,11 +1667,50 @@ void write_junction(pugi::xml_node root, node.append_attribute("connectingRoad").set_value(connecting->odr_id.c_str()); node.append_attribute("contactPoint") .set_value(connection.contact_point == ContactPoint::End ? "end" : "start"); + if (!connection.type_str.empty()) { + node.append_attribute("type").set_value(connection.type_str.c_str()); + } + for (const auto& [name, value] : connection.preserved.attributes) { + node.append_attribute(name.c_str()).set_value(value.c_str()); + } for (const auto& [from, to] : connection.lane_links) { pugi::xml_node link = node.append_child("laneLink"); link.append_attribute("from").set_value(from); link.append_attribute("to").set_value(to); } + for (const std::string& fragment : connection.preserved.children) { + append_fragment(node, fragment); + } + } + + // A virtual junction's own connections (§12.7): held aside by the reader so + // nothing tries to build geometry from them, re-emitted here unchanged (#537). + for (const JunctionConnection& connection : junction.preserved_connections) { + const Road* incoming = network.road(connection.incoming_road); + const Road* connecting = network.road(connection.connecting_road); + if (incoming == nullptr || connecting == nullptr) { + continue; + } + pugi::xml_node node = junction_node.append_child("connection"); + node.append_attribute("id").set_value(connection_id++); + node.append_attribute("incomingRoad").set_value(incoming->odr_id.c_str()); + node.append_attribute("connectingRoad").set_value(connecting->odr_id.c_str()); + node.append_attribute("contactPoint") + .set_value(connection.contact_point == ContactPoint::End ? "end" : "start"); + if (!connection.type_str.empty()) { + node.append_attribute("type").set_value(connection.type_str.c_str()); + } + for (const auto& [name, value] : connection.preserved.attributes) { + node.append_attribute(name.c_str()).set_value(value.c_str()); + } + for (const auto& [from, to] : connection.lane_links) { + pugi::xml_node link = node.append_child("laneLink"); + link.append_attribute("from").set_value(from); + link.append_attribute("to").set_value(to); + } + for (const std::string& fragment : connection.preserved.children) { + append_fragment(node, fragment); + } } // The signal synchronization group (§12.14, Table 84): `` diff --git a/core/tests/fuzz/corpus/virtual_junction_links.xodr b/core/tests/fuzz/corpus/virtual_junction_links.xodr new file mode 100644 index 0000000..e8012ff --- /dev/null +++ b/core/tests/fuzz/corpus/virtual_junction_links.xodr @@ -0,0 +1,75 @@ + + + +
+ + + + + + + + + + + + +
+ +
+ + + + + + + + + + + + + + + + +
+ +
+ + + + + + + + + + + + + + + diff --git a/core/tests/test_round_trip.cpp b/core/tests/test_round_trip.cpp index e073ae9..9cff93f 100644 --- a/core/tests/test_round_trip.cpp +++ b/core/tests/test_round_trip.cpp @@ -416,6 +416,81 @@ TEST(RoundTrip, PreservationDoesNotDuplicateModeledOrDerivedContent) { EXPECT_EQ(count("network; + + // 1. @elementS/@elementDir reached the model. Required for a virtual-junction + // link (road.linkage.virtjunc_link_attribute_usage) and read nowhere before. + const roadmaker::Road& main = *network.road(network.find_road("1")); + ASSERT_TRUE(main.predecessor.has_value()); + ASSERT_TRUE(main.predecessor->element_s.has_value()); + EXPECT_DOUBLE_EQ(*main.predecessor->element_s, 0.0); + EXPECT_EQ(main.predecessor->element_dir, "+"); + ASSERT_TRUE(main.successor.has_value()); + ASSERT_TRUE(main.successor->element_s.has_value()); + EXPECT_DOUBLE_EQ(*main.successor->element_s, 200.0); + EXPECT_EQ(main.successor->element_dir, "-"); + + // 2. The virtual junction's connection was PRESERVED, not deleted — and is + // held out of `connections` so nothing tries to build geometry from it. + const roadmaker::Junction& junction = *network.junction(network.find_junction("100")); + EXPECT_TRUE(junction.connections.empty()) << "a span junction generates nothing"; + ASSERT_EQ(junction.preserved_connections.size(), 1U) << "the connection was destroyed again"; + + // 3. @type and the unmodeled child rode along. + EXPECT_EQ(junction.preserved_connections[0].type_str, "virtual"); + EXPECT_FALSE(junction.preserved_connections[0].preserved.children.empty()); + + // The bytes. A model-only assertion would pass on a writer that emits none + // of it. + const auto written = roadmaker::write_xodr(loaded->network, "virtual_junction_links"); + ASSERT_TRUE(written.has_value()); + EXPECT_NE(written->find(R"(elementS="0")"), std::string::npos); + EXPECT_NE(written->find(R"(elementS="200")"), std::string::npos); + EXPECT_NE(written->find(R"(elementDir="+")"), std::string::npos); + EXPECT_NE(written->find(R"(elementDir="-")"), std::string::npos); + EXPECT_NE(written->find(R"(type="virtual")"), std::string::npos); + EXPECT_NE(written->find("acme:connectionNote"), std::string::npos); + + // Fixed point, and the linkage is still there after the second trip. + const auto reparsed = roadmaker::parse_xodr(*written, "virtual_junction_links"); + ASSERT_TRUE(reparsed.has_value()); + const roadmaker::Road& round = *reparsed->network.road(reparsed->network.find_road("1")); + ASSERT_TRUE(round.predecessor.has_value()); + EXPECT_EQ(round.predecessor->element_dir, "+"); + const auto again = roadmaker::write_xodr(reparsed->network, "virtual_junction_links"); + ASSERT_TRUE(again.has_value()); + EXPECT_EQ(*written, *again); + EXPECT_EQ(roadmaker::count_errors(loaded->diagnostics), 0U); +} + +TEST(RoundTrip, AnOrdinaryRoadLinkWritesNoElementSOrDir) { + // @elementS/@elementDir are virtual-junction-only, so an ordinary link must + // not start emitting them — that would churn every fixture in the suite and + // assert something the standard forbids there. + RoadNetwork network; + const auto road = roadmaker::author_clothoid_road( + network, + std::array{Waypoint{.x = 0.0, .y = 0.0}, Waypoint{.x = 100.0, .y = 0.0}}, + LaneProfile::two_lane_default(), + "Plain", + "1"); + ASSERT_TRUE(road.has_value()); + const auto written = roadmaker::write_xodr(network, "plain"); + ASSERT_TRUE(written.has_value()); + EXPECT_EQ(written->find("elementS="), std::string::npos); + EXPECT_EQ(written->find("elementDir="), std::string::npos); +} + // --- direct and crossing junctions (#534) ------------------------------------ // // A direct junction (§12.4) has NO connecting road: each carries