refactor(xodr,osc): one enum table and one XML helper set instead of two (p-ponytail-a)

CHANGELOG.md

@@ -1465,6 +1465,24 @@ Current version on `main`: **0.0.1**.
     same undo macro, instead of creating a second object.
 
 ### Changed
+- **The persistence layer keeps one table per OpenDRIVE enum instead of two**
+  ([#563](https://github.com/Robomous/RoadMaker/issues/563)). Six enums —
+  `e_laneType`, `e_roadMarkType`, `e_roadMarkColor`, `e_lane_direction`,
+  `e_objectType` and `@orientation` — were spelled out twice: an `if`-chain in
+  `xodr/reader.cpp` and a `switch` in `xodr/writer.cpp`, in different files,
+  with nothing tying the two directions together. That is the shape
+  [#476](https://github.com/Robomous/RoadMaker/issues/476) came in: the writer
+  re-spelled parsed enums into different semantics on save. They now share one
+  `constexpr` table each (`core/src/xodr/enum_names.hpp`), so a read and a write
+  cannot disagree. Five pugixml scalar helpers the two formats each kept a
+  private copy of (`to_double`, `num`, `set_num`, `set_optional_num`,
+  `node_to_string`) collapse into `core/src/xml/xml_common.hpp` the same way.
+  No behaviour change: re-emitting all 143 tracked `.xodr`/`.xosc` fixtures is
+  byte-identical, diagnostics included.
+
+  `append_fragment` was NOT shared despite the identical name and shape — the
+  OpenSCENARIO writer passes `pugi::parse_fragment` and the OpenDRIVE one does
+  not, so that pair is a real divergence and stays per-format.
 - **New roads default to the urban-with-sidewalks template**
   ([#355](https://github.com/Robomous/RoadMaker/issues/355)): the Create Road
   tool, its toolbar dropdown, and the Library fallback now start from
@@ -1876,6 +1894,12 @@ Current version on `main`: **0.0.1**.
   `asam.net:xodr:1.4.0:ids.only_ref_defined_ids` so the drop is never silent.
 
 ### Removed
+- `Environment::procedural_sky` (editor renderer)
+  ([#563](https://github.com/Robomous/RoadMaker/issues/563)). Set by
+  `sober_lighting()` and asserted by two scene-builder tests, but never read by
+  `GLRenderer` — a dead flag with a passing test and a "later render polish"
+  promise attached. The sampled-HDRI path it stood for will need its own field
+  if it is ever built.
 - `edit::junction_stop_lines` and `StopLineParams`, superseded by the derived
   stop-line entity above; the "Add stop lines to all arms" junction context
   action goes with them, since every arm already has one.

core/src/edit/operations.cpp

@@ -60,6 +60,7 @@
 
 #include "../mesh/junction_stoplines_detail.hpp"
 #include "../mesh/object_placement.hpp"
+#include "../road/junction_adjacency.hpp"
 
 namespace roadmaker::edit {
 
@@ -991,18 +992,7 @@ std::unique_ptr<Command> junction_stage(const RoadNetwork& network,
                                         std::span<const JunctionId> carried,
                                         TurnSetPolicy policy);
 
-/// The road end a link names, or nullopt when the link is absent or points at a
-/// junction rather than a road.
-std::optional<RoadEnd> linked_end(const std::optional<RoadLink>& link) {
-  if (!link.has_value()) {
-    return std::nullopt;
-  }
-  const RoadId* road = std::get_if<RoadId>(&link->target);
-  if (road == nullptr) {
-    return std::nullopt;
-  }
-  return RoadEnd{.road = *road, .contact = link->contact};
-}
+using road_detail::linked_end;
 
 /// The link slot a contact owns: predecessor at a Start, successor at an End.
 std::optional<RoadLink>& link_slot(Road& road, ContactPoint contact) {

core/src/mesh/junction_maneuvers.cpp

@@ -29,22 +29,13 @@
 #include <variant>
 #include <vector>
 
+#include "../road/junction_adjacency.hpp"
+
 namespace roadmaker {
 
 namespace {
 
-/// The road end a connecting road's link names, or nullopt when the link is
-/// absent or points at a junction rather than a road.
-std::optional<RoadEnd> linked_end(const std::optional<RoadLink>& link) {
-  if (!link.has_value()) {
-    return std::nullopt;
-  }
-  const RoadId* road = std::get_if<RoadId>(&link->target);
-  if (road == nullptr) {
-    return std::nullopt;
-  }
-  return RoadEnd{.road = *road, .contact = link->contact};
-}
+using road_detail::linked_end;
 
 /// The outgoing lane id a connecting road links to — the successor of its
 /// single right-hand driving lane. Mirrors retarget_junction's TurnKey read so

core/src/osc/reader.cpp

@@ -53,8 +53,7 @@
 #include <fmt/format.h>
 #include <pugixml.hpp>
 
-#include <fast_float/fast_float.h>
-
+#include <algorithm>
 #include <cmath>
 #include <cstddef>
 #include <fstream>
@@ -69,39 +68,18 @@
 #include <variant>
 #include <vector>
 
+#include "../xml/xml_common.hpp"
+
 namespace roadmaker::osc {
 namespace {
 
 // --- scalars ----------------------------------------------------------------
 
-/// Locale-independent `double` parsing; rejects trailing garbage and
-/// non-finite results.
-///
-/// Copied from core/src/xodr/reader.cpp:53-71 rather than shared, for the same
-/// reason `num()` is copied in this format's writer (osc/writer.cpp:59-63): the
-/// two standards' scalar policies are independent and either may need to
-/// diverge. OpenSCENARIO additionally admits `$parameter` expressions in
-/// numeric attributes (§9), which this reader does NOT evaluate — such a value
-/// fails here and takes the preserve-the-spelling path below, which is the
-/// correct outcome for a value whose meaning is only known at runtime.
-std::optional<double> to_double(std::string_view text) {
-  const char* first = text.data();
-  const char* last = text.data() + text.size();
-  double value{};
-  const auto result = fast_float::from_chars(first, last, value);
-  if (result.ec != std::errc{}) {
-    return std::nullopt;
-  }
-  for (const char* p = result.ptr; p != last; ++p) {
-    if (*p != ' ' && *p != '\t' && *p != '\r' && *p != '\n') {
-      return std::nullopt;
-    }
-  }
-  if (!std::isfinite(value)) {
-    return std::nullopt;
-  }
-  return value;
-}
+/// A numeric attribute this rejects is never dropped: it takes the
+/// preserve-the-spelling path below. That is the correct outcome for
+/// OpenSCENARIO's `$parameter` expressions (§9), which this reader does not
+/// evaluate because their meaning is only known at runtime.
+using xml_common::to_double;
 
 /// Strict non-negative integer parsing for `@revMajor` / `@revMinor`.
 std::optional<int> to_revision(std::string_view text) {
@@ -118,25 +96,10 @@ std::optional<int> to_revision(std::string_view text) {
   return value;
 }
 
-/// Serializes a node as a self-contained XML fragment, for the preserved tier.
-///
-/// `pugi::format_raw` drops the indentation the source document happened to
-/// carry, which is why a preserved fragment comes back re-canonicalized rather
-/// than byte-identical — fmt-s2's caveat (#326), stated here because this is
-/// where it originates.
-std::string node_to_string(const pugi::xml_node& node) {
-  std::ostringstream out;
-  node.print(out, "", pugi::format_raw);
-  return out.str();
-}
+using xml_common::node_to_string;
 
 bool is_one_of(std::string_view name, std::initializer_list<std::string_view> known) {
-  for (const std::string_view candidate : known) {
-    if (name == candidate) {
-      return true;
-    }
-  }
-  return false;
+  return std::ranges::find(known, name) != known.end();
 }
 
 std::optional<PhaseSemantics> to_semantics(std::string_view text) {

core/src/osc/writer.cpp

@@ -51,32 +51,16 @@
 #include <variant>
 #include <vector>
 
+#include "../xml/xml_common.hpp"
+
 namespace roadmaker::osc {
 namespace {
 
 // --- formatting -------------------------------------------------------------
 
-/// Shortest-precision round-trippable formatting; locale-independent.
-///
-/// Copied deliberately from core/src/xodr/writer.cpp:52-60 rather than shared:
-/// the two formats' number policies are independent and either may need to
-/// diverge. The "-0" normalization is load-bearing — without it a negative
-/// zero reaches the file and no round trip normalizes it away — and it has its
-/// own test here, because the OpenDRIVE suite does not cover this copy.
-std::string num(double value) {
-  std::string text = fmt::format("{}", value);
-  return text == "-0" ? "0" : text;
-}
-
-void set_num(pugi::xml_node node, const char* name, double value) {
-  node.append_attribute(name).set_value(num(value).c_str());
-}
-
-void set_optional_num(pugi::xml_node node, const char* name, const std::optional<double>& value) {
-  if (value.has_value()) {
-    set_num(node, name, *value);
-  }
-}
+using xml_common::num;
+using xml_common::set_num;
+using xml_common::set_optional_num;
 
 /// Sets an attribute only when the string is non-empty. Optional OpenSCENARIO
 /// attributes are omitted, never written empty: `reference=""` names a
@@ -148,6 +132,10 @@ std::size_t preserved_element_count(const RawXml& preserved, std::string_view el
   return count;
 }
 
+/// Appends a preserved fragment verbatim. NOT shareable with the OpenDRIVE
+/// writer's same-named helper despite the identical body shape: this one adds
+/// `pugi::parse_fragment` and that one takes pugixml's defaults. A REAL
+/// divergence, unlike the scalar helpers those two writers used to copy (#563).
 void append_fragment(pugi::xml_node parent, const std::string& fragment) {
   parent.append_buffer(
       fragment.data(), fragment.size(), pugi::parse_default | pugi::parse_fragment);

core/src/road/junction_adjacency.hpp

@@ -80,4 +80,21 @@ touched_junctions(const RoadNetwork& network, RoadId id, const Road& road) {
   return false;
 }
 
+/// The road end a link names, or nullopt when the link is absent or points at a
+/// junction rather than a road.
+///
+/// One definition, shared by the command layer and the junction mesher (#563):
+/// both need to ask "which road end is on the other side of this link?", and
+/// both used to answer it with their own byte-identical copy.
+[[nodiscard]] inline std::optional<RoadEnd> linked_end(const std::optional<RoadLink>& link) {
+  if (!link.has_value()) {
+    return std::nullopt;
+  }
+  const RoadId* road = std::get_if<RoadId>(&link->target);
+  if (road == nullptr) {
+    return std::nullopt;
+  }
+  return RoadEnd{.road = *road, .contact = link->contact};
+}
+
 } // namespace roadmaker::road_detail

core/src/xml/xml_common.hpp

@@ -0,0 +1,98 @@
+/*
+ * Copyright 2026 Robomous
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#pragma once
+
+// Internal (non-installed) pugixml scalar helpers shared by both persistence
+// layers, xodr/ and osc/. Each existed twice, per format, each copy carrying a
+// comment defending the duplication against a divergence that never happened
+// (#563). Split them again the day a policy actually differs.
+//
+// NOT shared, deliberately: `append_fragment`. The two formats pass different
+// pugixml parse flags (OpenSCENARIO adds `parse_fragment`) — a real divergence.
+
+#include <fmt/format.h>
+#include <pugixml.hpp>
+
+#include <fast_float/fast_float.h>
+
+#include <cmath>
+#include <optional>
+#include <sstream>
+#include <string>
+#include <string_view>
+#include <system_error>
+
+namespace roadmaker::xml_common {
+
+/// Locale-independent double parsing; rejects trailing garbage (whitespace is
+/// tolerated). `std::stod` is locale-dependent — never use it for ASAM IO.
+///
+/// A value this rejects is never dropped: both readers fall through to the
+/// preserve-the-spelling path. That is the correct outcome for OpenSCENARIO's
+/// `$parameter` expressions (§9), whose meaning is only known at runtime.
+[[nodiscard]] inline std::optional<double> to_double(std::string_view text) {
+  const char* first = text.data();
+  const char* last = text.data() + text.size();
+  double value{};
+  const auto result = fast_float::from_chars(first, last, value);
+  if (result.ec != std::errc{}) {
+    return std::nullopt;
+  }
+  for (const char* p = result.ptr; p != last; ++p) {
+    if (*p != ' ' && *p != '\t' && *p != '\r' && *p != '\n') {
+      return std::nullopt;
+    }
+  }
+  if (!std::isfinite(value)) {
+    return std::nullopt;
+  }
+  return value;
+}
+
+/// Shortest-precision round-trippable formatting; locale-independent.
+///
+/// The "-0" normalization is load-bearing: without it a negative zero reaches
+/// the file and no round trip normalizes it away.
+[[nodiscard]] inline std::string num(double value) {
+  std::string text = fmt::format("{}", value);
+  return text == "-0" ? "0" : text;
+}
+
+inline void set_num(pugi::xml_node node, const char* name, double value) {
+  node.append_attribute(name).set_value(num(value).c_str());
+}
+
+inline void
+set_optional_num(pugi::xml_node node, const char* name, const std::optional<double>& value) {
+  if (value.has_value()) {
+    set_num(node, name, *value);
+  }
+}
+
+/// Serializes a node as a self-contained XML fragment, for the verbatim
+/// preservation tier (roadmaker/xodr/raw_xml.hpp).
+///
+/// `pugi::format_raw` drops whatever indentation the source document happened
+/// to carry, which is why a preserved fragment comes back re-canonicalized
+/// rather than byte-identical — fmt-s2's caveat (#326) originates here.
+[[nodiscard]] inline std::string node_to_string(const pugi::xml_node& node) {
+  std::ostringstream out;
+  node.print(out, "", pugi::format_raw);
+  return out.str();
+}
+
+} // namespace roadmaker::xml_common