fix(xodr): round-trip virtual-junction linkage and connection internals (#537)

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 `<link>` 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 `<connection>`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.
+  - **`<connection>` `@type` and non-`<laneLink>` 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):
   `<lateralProfile><shape>` and the legacy `<crossfall>` (§10.5.1), road

core/include/roadmaker/road/junction.hpp

@@ -114,6 +114,18 @@ struct JunctionConnection {
 
   /// Pairs of {incoming lane odr_id, connecting lane odr_id}.
   std::vector<std::pair<int, int>> 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 `<connection>` — anything
+  /// besides `<laneLink>` — preserved verbatim (#537). The reader iterated
+  /// `children("laneLink")` only, so a `<predecessor>`/`<successor>` 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<JunctionConnection> preserved_connections;
+
   /// `<connection>` 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

core/include/roadmaker/road/road.hpp

@@ -42,6 +42,19 @@ enum class ContactPoint {
 struct RoadLink {
   std::variant<RoadId, JunctionId> 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<double> 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

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-<laneLink> 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<double> 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;
   }
 

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): `<controller>`

core/tests/fuzz/corpus/virtual_junction_links.xodr

@@ -0,0 +1,75 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!-- VALID seed: virtual-junction linkage and connection internals (#537).
+
+     Three fidelity holes, all in MODELED scopes that #453's preservation sweep
+     deliberately does not cover:
+
+       1. Road-link @elementS/@elementDir. Rule
+          asam.net:xodr:1.7.0:road.linkage.virtjunc_link_attribute_usage
+          REQUIRES both on a <link> pointing at a virtual junction. read_link
+          took elementType/elementId/contactPoint only — no diagnostic — so a
+          spec-conformant file lost its virtual-junction linkage on save.
+       2. A virtual junction's own <connection>s. §12.7 permits them, but the
+          arms-xor-spans policy CLEARED the list: warned, and then destroyed.
+          They are preserved now — held out of `connections` so nothing tries to
+          build geometry from them, and re-emitted unchanged.
+       3. <connection> @type and non-<laneLink> children. The reader iterated
+          children("laneLink") only. @id regeneration is benign (the writer
+          renumbers deterministically); @type is not.
+
+     Road 1 links into virtual junction 100 with @elementS/@elementDir on both
+     ends. Junction 100 declares a connection carrying @type and an unmodeled
+     child, so cases 2 and 3 ride together.
+
+     Apache-2.0. -->
+<OpenDRIVE>
+  <header revMajor="1" revMinor="8" name="virtual_junction_links" vendor="RoadMaker" />
+  <road name="main" length="200" id="1" junction="-1">
+    <link>
+      <predecessor elementType="junction" elementId="100" elementS="0" elementDir="+" />
+      <successor elementType="junction" elementId="100" elementS="200" elementDir="-" />
+    </link>
+    <planView>
+      <geometry s="0" x="0" y="0" hdg="0" length="200">
+        <line />
+      </geometry>
+    </planView>
+    <lanes>
+      <laneSection s="0">
+        <center>
+          <lane id="0" type="none" level="false" />
+        </center>
+        <right>
+          <lane id="-1" type="driving" level="false">
+            <width sOffset="0" a="3.5" b="0" c="0" d="0" />
+          </lane>
+        </right>
+      </laneSection>
+    </lanes>
+  </road>
+  <road name="slip" length="60" id="2" junction="-1">
+    <planView>
+      <geometry s="0" x="0" y="-8" hdg="0" length="60">
+        <line />
+      </geometry>
+    </planView>
+    <lanes>
+      <laneSection s="0">
+        <center>
+          <lane id="0" type="none" level="false" />
+        </center>
+        <right>
+          <lane id="-1" type="driving" level="false">
+            <width sOffset="0" a="3.5" b="0" c="0" d="0" />
+          </lane>
+        </right>
+      </laneSection>
+    </lanes>
+  </road>
+  <junction id="100" name="virtual_stretch" type="virtual" mainRoad="1" orientation="none" sStart="40" sEnd="120">
+    <connection id="0" incomingRoad="1" connectingRoad="2" contactPoint="start" type="virtual">
+      <laneLink from="-1" to="-1" />
+      <userData code="acme:connectionNote" value="kept" />
+    </connection>
+  </junction>
+</OpenDRIVE>

core/tests/test_round_trip.cpp

@@ -416,6 +416,81 @@ TEST(RoundTrip, PreservationDoesNotDuplicateModeledOrDerivedContent) {
   EXPECT_EQ(count("<roadMark"), 1U);
 }
 
+// --- virtual-junction fidelity (#537) ----------------------------------------
+//
+// Built on #534's junction scaffolding. Three holes in MODELED scopes that
+// #453's preservation sweep deliberately does not cover.
+
+TEST(RoundTrip, VirtualJunctionLinkageAndConnectionsSurviveWriteParseWrite) {
+  const std::filesystem::path sample =
+      std::filesystem::path(RM_FUZZ_CORPUS_DIR) / "virtual_junction_links.xodr";
+  const auto loaded = roadmaker::load_xodr(sample);
+  ASSERT_TRUE(loaded.has_value()) << (loaded ? "" : loaded.error().message);
+  const RoadNetwork& network = loaded->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, 2>{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 <connection> carries