From 1446fec445f0a846a02237baa2bfb28183cdca50 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Thu, 16 Jul 2026 00:51:58 -0700 Subject: [PATCH 1/4] feat(core): junction regeneration learns turn-set changes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit regenerate_junction refused any turn-set change and the editor swallowed the failure into a warning toast, so adding a lane to a road feeding a junction left it stale until a manual delete-and-recreate. Teach it to add, drop, and rewrite connections instead — the case GW-2 step 12's turn lane needs. - DirtySet gains junctions_are_current: the flag that says a command already brought its junctions up to date, split out from topology so a lane appearing can be topology AND need regeneration. The editor's regen loop keys off it instead of topology. - add_lane, remove_lane, set_lane_type declare junctions_touching so the editor knows which junctions to regenerate. - regenerate_junction is restructured around a creator: it partitions the plan into matched/new/dropped turns, keeps the ids of the turns that survive (keyed matching), materializes the ones that appeared, and erases the ones that vanished with everything they own. A TurnSetPolicy parameter keeps the per-frame drag path in-place-only, where creating connecting roads would leak arena slots on every discarded frame. - The regen loop now merges a regeneration's own topology into the dirty set, so a dropped connecting road prunes stale selections. Author: Armando Anaya --- core/include/roadmaker/edit/command.hpp | 15 +- core/include/roadmaker/edit/operations.hpp | 46 ++- core/src/edit/operations.cpp | 330 +++++++++++++++------ core/tests/test_edit_operations.cpp | 198 +++++++++++++ editor/src/document/document.cpp | 35 ++- editor/tests/test_junction_regen.cpp | 30 ++ 6 files changed, 544 insertions(+), 110 deletions(-) diff --git a/core/include/roadmaker/edit/command.hpp b/core/include/roadmaker/edit/command.hpp index cb088784..7e1433ec 100644 --- a/core/include/roadmaker/edit/command.hpp +++ b/core/include/roadmaker/edit/command.hpp @@ -30,8 +30,21 @@ struct DirtySet { /// pass in phase 2 (#69); phase 4 (#71) reuses it for instanced props. std::vector objects; - /// Roads or junctions were added or removed (drives tree-model resets). + /// Roads or junctions were added or removed. Drives the editor's wholesale + /// mesh re-upload (a partial per-road upload cannot add or drop an item) and + /// prunes selections that named a now-erased id. bool topology = false; + + /// This command already brought `junctions` up to date itself; the editor + /// must not regenerate them again. Set by the commands that build or tear + /// down junction structure (create/delete junction, split_road, delete_road) + /// — a second regeneration would double-work or fight them. + /// + /// Default false is the safe direction: a command that forgets the flag gets + /// a redundant regeneration (slow, correct) rather than a stale junction + /// (fast, wrong). Kept separate from `topology` deliberately — a lane + /// appearing is topology AND needs regeneration, which one flag cannot say. + bool junctions_are_current = false; }; /// One undoable kernel mutation (docs/m2/01_editing_framework.md §1.1). diff --git a/core/include/roadmaker/edit/operations.hpp b/core/include/roadmaker/edit/operations.hpp index 1f56a282..bd8ae695 100644 --- a/core/include/roadmaker/edit/operations.hpp +++ b/core/include/roadmaker/edit/operations.hpp @@ -282,17 +282,43 @@ struct TAttachOptions { double s, const TAttachOptions& options = {}); +/// Whether a regeneration may change the junction's turn set. +enum class TurnSetPolicy { + /// Turns may be added and dropped: new connecting roads are created, ones + /// whose turn disappeared are erased, and the connection table is rewritten. + /// The turns that survive keep their connecting-road IDs. + AllowChange, + /// Only geometry and widths may change; a different turn set is an error. + /// For the per-frame preview path ONLY — see regenerate_junction. + InPlaceOnly, +}; + /// Re-runs the generator from a junction's recorded arm list and replaces its -/// connecting-road geometry and lane widths in place — connecting-road IDs -/// and the connection table survive, so held references and the undo stack -/// stay valid (02 §6 "Dependency tracking"). The editor triggers this after -/// any edit to an incoming road (via junctions_touching). M2 restriction: the -/// connection COUNT must be unchanged (a lane added/removed on an incoming -/// road changes the turn set — recreate the junction); an empty arm list -/// (foreign junction) is an error. A no-op regeneration writes byte-identical -/// output. -[[nodiscard]] RM_API std::unique_ptr regenerate_junction( - const RoadNetwork& network, JunctionId junction, const JunctionGenOptions& options = {}); +/// connecting-road geometry and lane widths in place (02 §6 "Dependency +/// tracking"). The editor triggers this after any edit to an incoming road +/// (via junctions_touching). An empty arm list (foreign junction) is an error. +/// A no-op regeneration writes byte-identical output. +/// +/// A turn that survives keeps its connecting-road ID — matching is by the +/// (incoming road+contact+lane, outgoing road+contact+lane) key, not by order +/// — so held references and the undo stack stay valid across a regeneration. +/// +/// Under AllowChange (the default) a lane added to, removed from, or retyped +/// on an incoming road regenerates the junction: turns that appeared get fresh +/// connecting roads, turns that vanished have theirs erased. +/// +/// `policy` exists for ONE caller. A preview session reverts and DESTROYS its +/// command on every frame (Document::update_preview), and revert frees created +/// ids with erase_exact, which reserves the slot rather than recycling it — so +/// a discarded command's created slots can never be reused. A per-frame +/// regeneration that creates connecting roads therefore leaks slots for the +/// rest of the session, which is why move_waypoint_following_junctions asks +/// for InPlaceOnly and takes the stale junction (as it does today) instead. +[[nodiscard]] RM_API std::unique_ptr +regenerate_junction(const RoadNetwork& network, + JunctionId junction, + const JunctionGenOptions& options = {}, + TurnSetPolicy policy = TurnSetPolicy::AllowChange); /// Deletes the junction AND its connecting roads (the §7 closure); incoming /// roads survive with their predecessor/successor links into the junction diff --git a/core/src/edit/operations.cpp b/core/src/edit/operations.cpp index 39c7dc1a..2bf294a0 100644 --- a/core/src/edit/operations.cpp +++ b/core/src/edit/operations.cpp @@ -352,6 +352,10 @@ class CompositeCommand final : public Command { } } dirty.topology = dirty.topology || child_dirty.topology; + // One child that built its own junctions speaks for the composite: the + // assemblies and attach_t_junction end in create_junction, so the + // junctions they name are already generated. + dirty.junctions_are_current = dirty.junctions_are_current || child_dirty.junctions_are_current; } return dirty; } @@ -595,18 +599,22 @@ std::vector deletion_closure(const RoadNetwork& network, std::vector doomed, - std::optional doomed_junction) { +/// Captures `doomed` roads onto `command->erased`, with everything they own: +/// sections, lanes, and the objects and signals anchored to them. +/// +/// The ownership walk has to be explicit because erase_road_exact does NOT +/// cascade the way erase_road does — it frees exactly the slot it is given. +/// Anything owned but not captured here would survive its road holding a +/// RoadId into an emptied slot. erase_values_exact erases leaf-to-root, so the +/// order things are added in does not matter. +void capture_road_erasure(const RoadNetwork& network, + GenericCommand& command, + std::span doomed) { for (const RoadId road_id : doomed) { const Road* road = network.road(road_id); + if (road == nullptr) { + continue; + } command.erased.roads.emplace_back(road_id, *road); for (const LaneSectionId section_id : road->sections) { const LaneSection* section = network.lane_section(section_id); @@ -615,7 +623,26 @@ void capture_deletion(const RoadNetwork& network, command.erased.lanes.emplace_back(lane_id, *network.lane(lane_id)); } } + for (const ObjectId object_id : objects_of(network, road_id)) { + command.erased.objects.emplace_back(object_id, *network.object(object_id)); + } + for (const SignalId signal_id : signals_of(network, road_id)) { + command.erased.signals.emplace_back(signal_id, *network.signal(signal_id)); + } } +} + +/// Captures onto `command` everything the closure deletion touches: the +/// doomed roads with everything they own (plus `doomed_junction` when set) as +/// erasures, and — as before/after value edits — surviving junctions whose +/// connections reference a doomed road, and surviving roads whose links (or +/// junction back-reference) point into the deleted set. Undo restores every +/// removed object and link exactly. +void capture_deletion(const RoadNetwork& network, + GenericCommand& command, + std::span doomed, + std::optional doomed_junction) { + capture_road_erasure(network, command, doomed); if (doomed_junction.has_value()) { command.erased.junctions.emplace_back(*doomed_junction, *network.junction(*doomed_junction)); } @@ -1154,7 +1181,10 @@ std::unique_ptr delete_road(const RoadNetwork& network, RoadId road_id) // Every doomed road is dirty so incremental re-mesh drops (and, on undo, // restores) its mesh entry; every junction touching one re-floors. - DirtySet dirty{.roads = doomed, .topology = true}; + // junctions_are_current: capture_deletion strips the doomed connections AND + // arms itself, so a survivor's arm list is already correct — regenerating it + // here would replan a junction the user just tore an arm off of. + DirtySet dirty{.roads = doomed, .topology = true, .junctions_are_current = true}; for (const RoadId doomed_id : doomed) { for (const JunctionId junction_id : junctions_touching(network, doomed_id)) { if (std::ranges::find(dirty.junctions, junction_id) == dirty.junctions.end()) { @@ -1548,7 +1578,11 @@ std::unique_ptr split_road(const RoadNetwork& network, RoadId road_id, duplicated_lanes.push_back(LaneBlueprint{.value = std::move(copy)}); } - DirtySet split_dirty{.roads = {road_id}, .topology = true}; + // junctions_are_current: the split remaps the junction's arms and + // incoming_road onto the tail itself (below), so the connection table is + // already correct — a regeneration would only re-fit geometry that did not + // move. + DirtySet split_dirty{.roads = {road_id}, .topology = true, .junctions_are_current = true}; if (succ_junction.has_value()) { split_dirty.junctions.push_back(*succ_junction); } @@ -2268,19 +2302,63 @@ Expected preview_junction(const RoadNetwork& network, .dropped_turns = std::move(plan->dropped)}; } +/// Builds ONE connecting road for `cp` into `target`, registering everything it +/// creates in `created`, and returns the connection table entry for it. The +/// single authority for what a connecting road is: shared by +/// materialize_junction (a fresh junction) and regenerate_junction (a turn that +/// appeared on an existing one). +/// +/// Holds no reference across a create_* call — every arena insert may +/// reallocate (arena.hpp "never store pointers across mutations"). +JunctionConnection materialize_connection(RoadNetwork& target, + Values& created, + JunctionId junction_id, + const ConnectingPlan& cp) { + const RoadId road_id = target.create_road("", next_free_road_odr_id(target)); + created.roads.emplace_back(road_id, Road{}); + { + Road& road = *target.road(road_id); + road.plan_view = cp.line; + road.length = road.plan_view.length(); + road.elevation = connecting_elevation(cp, road.length); + road.junction = junction_id; + // Connecting roads run in driving direction: start touches the incoming + // arm, end the outgoing arm (12.4.1 laneLink direction rules). + road.predecessor = RoadLink{.target = cp.from.road, .contact = cp.from.contact}; + road.successor = RoadLink{.target = cp.to.road, .contact = cp.to.contact}; + } + + const LaneSectionId section_id = target.add_lane_section(road_id, 0.0); + created.sections.emplace_back(section_id, LaneSection{}); + const LaneId center = target.add_lane(section_id, 0, LaneType::None); + created.lanes.emplace_back(center, Lane{}); + // Single right-hand driving lane carrying the +s (driving-direction) flow. + const LaneId drive = target.add_lane(section_id, -1, LaneType::Driving); + created.lanes.emplace_back(drive, Lane{}); + const double length = target.road(road_id)->length; + Lane& lane = *target.lane(drive); + lane.widths.push_back(connecting_lane_width(cp, length)); + lane.predecessor = cp.from_lane; + lane.successor = cp.to_lane; + + return JunctionConnection{.incoming_road = cp.from.road, + .connecting_road = road_id, + .contact_point = ContactPoint::Start, + .lane_links = {{cp.from_lane, -1}}}; +} + /// Builds the junction record, its connecting roads and the connection table /// into `target` from `plan`, registering every created object in `created`; /// also links each incoming end to the junction. Shared by create_junction's -/// creator (regeneration edits existing roads in place instead). Assumes the -/// arm link slots are free — validated at factory time. +/// creator (regeneration reuses materialize_connection per turn instead). +/// Assumes the arm link slots are free — validated at factory time. Expected materialize_junction(RoadNetwork& target, Values& created, std::span ends, const JunctionPlan& plan) { const JunctionId junction_id = target.create_junction(next_free_junction_odr_id(target), ""); created.junctions.emplace_back(junction_id, Junction{}); - Junction& junction = *target.junction(junction_id); - junction.arms.assign(ends.begin(), ends.end()); + target.junction(junction_id)->arms.assign(ends.begin(), ends.end()); for (const RoadEnd& end : ends) { Road& road = *target.road(end.road); @@ -2292,36 +2370,14 @@ Expected materialize_junction(RoadNetwork& target, } } + std::vector connections; + connections.reserve(plan.roads.size()); for (const ConnectingPlan& cp : plan.roads) { - const RoadId road_id = target.create_road("", next_free_road_odr_id(target)); - created.roads.emplace_back(road_id, Road{}); - Road& road = *target.road(road_id); - road.plan_view = cp.line; - road.length = road.plan_view.length(); - road.elevation = connecting_elevation(cp, road.length); - road.junction = junction_id; - // Connecting roads run in driving direction: start touches the incoming - // arm, end the outgoing arm (12.4.1 laneLink direction rules). - road.predecessor = RoadLink{.target = cp.from.road, .contact = cp.from.contact}; - road.successor = RoadLink{.target = cp.to.road, .contact = cp.to.contact}; - - const LaneSectionId section_id = target.add_lane_section(road_id, 0.0); - created.sections.emplace_back(section_id, LaneSection{}); - const LaneId center = target.add_lane(section_id, 0, LaneType::None); - created.lanes.emplace_back(center, Lane{}); - // Single right-hand driving lane carrying the +s (driving-direction) flow. - const LaneId drive = target.add_lane(section_id, -1, LaneType::Driving); - created.lanes.emplace_back(drive, Lane{}); - Lane& lane = *target.lane(drive); - lane.widths.push_back(connecting_lane_width(cp, road.length)); - lane.predecessor = cp.from_lane; - lane.successor = cp.to_lane; - - junction.connections.push_back(JunctionConnection{.incoming_road = cp.from.road, - .connecting_road = road_id, - .contact_point = ContactPoint::Start, - .lane_links = {{cp.from_lane, -1}}}); + connections.push_back(materialize_connection(target, created, junction_id, cp)); } + // Re-fetch: every create_road above may have reallocated the road arena, and + // create_junction the junction arena. + target.junction(junction_id)->connections = std::move(connections); return {}; } @@ -2355,7 +2411,11 @@ std::unique_ptr create_junction(const RoadNetwork& network, return invalid_command(std::string(kName), plan.error()); } - auto command = std::make_unique(std::string(kName), DirtySet{.topology = true}); + // junctions_are_current: materialize_junction below IS the generator — it + // builds the connection table and the connecting roads. Regenerating on top + // of a fresh build would re-plan what was just planned. + auto command = std::make_unique( + std::string(kName), DirtySet{.topology = true, .junctions_are_current = true}); for (const RoadEnd& end : ends) { const bool already_touched = std::ranges::any_of( command->before.roads, [&](const auto& entry) { return entry.first == end.road; }); @@ -2506,7 +2566,8 @@ std::unique_ptr create_linked_road(const RoadNetwork& network, std::unique_ptr regenerate_junction(const RoadNetwork& network, JunctionId junction_id, - const JunctionGenOptions& options) { + const JunctionGenOptions& options, + TurnSetPolicy policy) { static constexpr std::string_view kName = "Regenerate Junction"; const auto fail = [&](std::string message) { return invalid_command( @@ -2524,20 +2585,11 @@ std::unique_ptr regenerate_junction(const RoadNetwork& network, if (!plan.has_value()) { return invalid_command(std::string(kName), plan.error()); } - // M2 restriction: only geometry/width may change; a different turn set - // (a lane added or removed on an incoming road) needs a full recreate so - // ids can be freed. Regeneration edits the existing connecting roads in - // place, preserving their ids and the connection table. - if (plan->roads.size() != junction->connections.size()) { + if (policy == TurnSetPolicy::InPlaceOnly && + plan->roads.size() != junction->connections.size()) { return fail("regeneration changed the connection count; delete and recreate the junction"); } - DirtySet dirty{.junctions = {junction_id}}; - for (const JunctionConnection& connection : junction->connections) { - dirty.roads.push_back(connection.connecting_road); - } - auto command = std::make_unique(std::string(kName), std::move(dirty)); - // Match each freshly planned turn to its existing connecting road by KEY — // the (incoming road+contact+lane, outgoing road+contact+lane) it links — not // by generation order. A node drag can re-order the plan (e.g. a turn crossing @@ -2580,7 +2632,16 @@ std::unique_ptr regenerate_junction(const RoadNetwork& network, .to_lane = to_lane}; }; - std::vector matched(junction->connections.size(), false); + // Partition the plan against the existing table. `matched` pairs a planned + // turn with the connecting road that already serves it (id reused); what is + // left over on each side is a turn that appeared or one that vanished. + struct Matched { + ConnectingPlan cp; + RoadId road; + }; + std::vector matched_turns; + std::vector new_turns; + std::vector claimed(junction->connections.size(), false); for (const ConnectingPlan& cp : plan->roads) { const TurnKey want{.from_road = cp.from.road, .from_contact = cp.from.contact, @@ -2590,7 +2651,7 @@ std::unique_ptr regenerate_junction(const RoadNetwork& network, .to_lane = cp.to_lane}; std::size_t found = junction->connections.size(); for (std::size_t i = 0; i < junction->connections.size(); ++i) { - if (matched[i]) { + if (claimed[i]) { continue; } if (const auto key = connection_key(junction->connections[i]); @@ -2600,32 +2661,91 @@ std::unique_ptr regenerate_junction(const RoadNetwork& network, } } if (found == junction->connections.size()) { - // Same count but a different turn set (e.g. a lane retyped so a turn moved - // to a different lane): the ids can't be reused in place. - return fail("regeneration changed the turn set; delete and recreate the junction"); + if (policy == TurnSetPolicy::InPlaceOnly) { + // Same count but a different turn set (e.g. a lane retyped so a turn + // moved to a different lane): the ids can't be reused in place. + return fail("regeneration changed the turn set; delete and recreate the junction"); + } + new_turns.push_back(cp); + continue; } - matched[found] = true; - const RoadId road_id = junction->connections[found].connecting_road; - const Road* road = network.road(road_id); - Road after = *road; - after.plan_view = cp.line; - after.length = after.plan_view.length(); - after.elevation = connecting_elevation(cp, after.length); - command->before.roads.emplace_back(road_id, *road); - command->after.roads.emplace_back(road_id, std::move(after)); - - const LaneSection& section = *network.lane_section(road->sections.front()); - for (const LaneId lane_id : section.lanes) { + claimed[found] = true; + matched_turns.push_back( + Matched{.cp = cp, .road = junction->connections[found].connecting_road}); + } + + // Unclaimed connections serve a turn the plan no longer contains. NOTE: a + // connection whose key could not be read at all (a malformed connecting road + // — no sections, missing links, empty lane_links) lands here too and is + // rebuilt from the plan rather than reported. That repairs it, but silently. + std::vector dropped; + for (std::size_t i = 0; i < junction->connections.size(); ++i) { + if (!claimed[i]) { + dropped.push_back(junction->connections[i].connecting_road); + } + } + + // Every connecting road is dirty so incremental re-mesh re-tessellates the + // survivors and drops the mesh entries of the erased ones. + DirtySet dirty{.junctions = {junction_id}, + .topology = !new_turns.empty() || !dropped.empty(), + .junctions_are_current = true}; + for (const JunctionConnection& connection : junction->connections) { + dirty.roads.push_back(connection.connecting_road); + } + auto command = std::make_unique(std::string(kName), std::move(dirty)); + + // The junction record itself changes (the connection table is rewritten), so + // it belongs in `before` — the creator mutates it in the network and + // GenericCommand re-reads `after` from there once the creator has run. + command->before.junctions.emplace_back(junction_id, *junction); + for (const Matched& match : matched_turns) { + const Road* road = network.road(match.road); + command->before.roads.emplace_back(match.road, *road); + for (const LaneId lane_id : network.lane_section(road->sections.front())->lanes) { const Lane* lane = network.lane(lane_id); - if (lane->odr_id != -1) { - continue; + if (lane->odr_id == -1) { + command->before.lanes.emplace_back(lane_id, *lane); } - Lane lane_after = *lane; - lane_after.widths = {connecting_lane_width(cp, after.length)}; - command->before.lanes.emplace_back(lane_id, *lane); - command->after.lanes.emplace_back(lane_id, std::move(lane_after)); } } + capture_road_erasure(network, *command, dropped); + + command->creator = [junction_id, + matched_turns = std::move(matched_turns), + new_turns = std::move(new_turns), + dropped](RoadNetwork& target, Values& created) -> Expected { + // Pass 1: rewrite the turns that survive. No creation happens here, so + // references stay valid within an iteration. + std::vector table; + table.reserve(matched_turns.size() + new_turns.size()); + for (const Matched& match : matched_turns) { + Road& road = *target.road(match.road); + road.plan_view = match.cp.line; + road.length = road.plan_view.length(); + road.elevation = connecting_elevation(match.cp, road.length); + const double length = road.length; + for (const LaneId lane_id : target.lane_section(road.sections.front())->lanes) { + Lane& lane = *target.lane(lane_id); + if (lane.odr_id == -1) { + lane.widths = {connecting_lane_width(match.cp, length)}; + } + } + table.push_back(JunctionConnection{.incoming_road = match.cp.from.road, + .connecting_road = match.road, + .contact_point = ContactPoint::Start, + .lane_links = {{match.cp.from_lane, -1}}}); + } + // Pass 2: build the turns that appeared. Every create_* here may realloc + // an arena, which is why no reference from pass 1 survives into it. + for (const ConnectingPlan& cp : new_turns) { + table.push_back(materialize_connection(target, created, junction_id, cp)); + } + // The dropped roads are erased by `erased` after this returns; dropping + // them from the table first means they are never referenced when they go. + target.junction(junction_id)->connections = std::move(table); + return {}; + }; return command; } @@ -2663,8 +2783,17 @@ std::unique_ptr move_waypoint_following_junctions(const RoadNetwork& ne for (const JunctionId junction_id : followed) { // Built lazily, so each regeneration plans against the network with the // move already applied — the whole point of following mid-drag. - builders.push_back( - [junction_id](RoadNetwork& net) { return regenerate_junction(net, junction_id); }); + // + // InPlaceOnly: this command is a preview factory, rebuilt and discarded on + // every drag frame. A regeneration that created connecting roads would have + // them erase_exact'd by the frame's revert and then lose the only handle + // that could restore those slots when the command is destroyed — reserving + // arena slots, per frame, for the rest of the session. A drag that changes + // the turn set therefore leaves the junction stale (and toasts) exactly as + // it does today; lane edits, which are not previewed, regenerate normally. + builders.push_back([junction_id](RoadNetwork& net) { + return regenerate_junction(net, junction_id, {}, TurnSetPolicy::InPlaceOnly); + }); } // Same undo-menu text as the plain move: whether the drag happened to touch a // junction is not something the user should read in the Edit menu. @@ -2690,7 +2819,13 @@ std::unique_ptr delete_junction(const RoadNetwork& network, JunctionId }); const std::vector doomed = deletion_closure(network, std::move(seeds)); - DirtySet dirty{.roads = doomed, .junctions = {junction_id}, .topology = true}; + // junctions_are_current: the junction named here is the one being deleted, + // and capture_deletion strips the doomed connections from every survivor — + // there is nothing left to regenerate against. + DirtySet dirty{.roads = doomed, + .junctions = {junction_id}, + .topology = true, + .junctions_are_current = true}; for (const RoadId doomed_id : doomed) { for (const JunctionId touched : junctions_touching(network, doomed_id)) { if (std::ranges::find(dirty.junctions, touched) == dirty.junctions.end()) { @@ -3017,8 +3152,16 @@ add_lane(const RoadNetwork& network, LaneSectionId section_id, int side, LaneTyp ? outermost_lane->widths : std::vector{Poly3{.a = 3.5}}; + // A lane on an end section changes what driving_lanes_at reports, and so the + // junction's turn set: name the junctions so the editor regenerates them. + // A lane on an interior section leaves the turn set alone and regeneration + // is a byte-identical no-op — cheaper than deciding here which sections are + // ends. auto command = std::make_unique( - std::string(kName), DirtySet{.roads = {section->road}, .topology = true}); + std::string(kName), + DirtySet{.roads = {section->road}, + .junctions = junctions_touching(network, section->road), + .topology = true}); command->before.sections.emplace_back(section_id, *section); command->creator = [section_id, new_odr_id, type, widths = std::move(widths)]( RoadNetwork& target, Values& created) -> Expected { @@ -3076,10 +3219,12 @@ std::unique_ptr remove_lane(const RoadNetwork& network, LaneId lane_id) } }); - DirtySet dirty{.roads = {context->road_id}, .topology = true}; - for (const auto& [junction_id, value] : junctions_before) { - dirty.junctions.push_back(junction_id); - } + // Every junction the road touches, not just those whose lane_links this + // command pruned: losing a lane drops a turn from the plan even where no + // lane_link named it, and regeneration is what rebuilds the turn set. + DirtySet dirty{.roads = {context->road_id}, + .junctions = junctions_touching(network, context->road_id), + .topology = true}; auto command = std::make_unique(std::string(kName), std::move(dirty)); command->erased.lanes.emplace_back(lane_id, context->lane); command->before.junctions = std::move(junctions_before); @@ -3124,8 +3269,13 @@ std::unique_ptr set_lane_type(const RoadNetwork& network, LaneId lane_i } Lane after = context->lane; after.type = type; - auto command = - std::make_unique(std::string(kName), DirtySet{.roads = {context->road_id}}); + // Retyping to or from Driving changes what driving_lanes_at reports, so a + // turn appears, disappears, or moves to a different lane — name the + // junctions so regeneration rebuilds the turn set. + auto command = std::make_unique( + std::string(kName), + DirtySet{.roads = {context->road_id}, + .junctions = junctions_touching(network, context->road_id)}); command->before.lanes.emplace_back(lane_id, context->lane); command->after.lanes.emplace_back(lane_id, std::move(after)); return command; diff --git a/core/tests/test_edit_operations.cpp b/core/tests/test_edit_operations.cpp index c820fb17..34d2f1af 100644 --- a/core/tests/test_edit_operations.cpp +++ b/core/tests/test_edit_operations.cpp @@ -1,3 +1,4 @@ +#include "roadmaker/edit/connection.hpp" #include "roadmaker/edit/edit_stack.hpp" #include "roadmaker/edit/operations.hpp" #include "roadmaker/road/authoring.hpp" @@ -1108,6 +1109,203 @@ TEST(EditOperations, RegenerateJunctionIsByteEqualWhenNothingChanged) { auto regen = roadmaker::edit::regenerate_junction(network, junction); ASSERT_TRUE(regen->apply(network).has_value()); EXPECT_EQ(before, snapshot_xodr(network)); // deterministic re-run reproduces the document + + // And it must undo cleanly. expect_command_round_trip cannot be used here — + // it asserts a command changes something — but reverting a no-op is exactly + // where the P2 creator restructure could have corrupted the junction while + // still reproducing the document on apply. + ASSERT_TRUE(regen->revert(network).has_value()); + EXPECT_EQ(before, snapshot_xodr(network)); +} + +// --- regeneration: turn-set changes (P2 #263) ------------------------------- + +LaneId lane_with_odr_id(const RoadNetwork& network, RoadId road, int odr_id) { + for (const LaneId lane_id : network.lane_section(network.road(road)->sections.front())->lanes) { + if (network.lane(lane_id)->odr_id == odr_id) { + return lane_id; + } + } + return LaneId{}; +} + +void retype_lane(RoadNetwork& network, RoadId road, int odr_id, roadmaker::LaneType type) { + const LaneId lane = lane_with_odr_id(network, road, odr_id); + ASSERT_TRUE(lane.is_valid()); + ASSERT_TRUE(roadmaker::edit::set_lane_type(network, lane, type)->apply(network).has_value()); +} + +void add_outermost_lane(RoadNetwork& network, RoadId road, int side) { + ASSERT_TRUE(roadmaker::edit::add_lane(network, + network.road(road)->sections.front(), + side, + roadmaker::LaneType::Driving) + ->apply(network) + .has_value()); +} + +/// Adds one turn to the west→east movement, and only that one. +/// +/// plan_junction pairs `min(incoming, outgoing)` lanes per ordered arm pair, so +/// widening one arm alone changes nothing — the other side of the movement +/// still bottlenecks it at one lane. On an End-contact arm the incoming lanes +/// are the negative ones and the outgoing lanes the positive ones, so west +/// gains a driving lane on the right and east one on the left. +void widen_west_to_east(RoadNetwork& network, const TJunction& t) { + add_outermost_lane(network, t.west, -1); + add_outermost_lane(network, t.east, 1); +} + +TEST(EditOperations, RegenerateJunctionGrowsTheTurnSetWhenAnArmGainsADrivingLane) { + RoadNetwork network; + const TJunction t = make_t_junction(network); + ASSERT_TRUE(roadmaker::edit::create_junction(network, t.ends)->apply(network).has_value()); + const JunctionId junction = network.find_junction("1"); + const std::size_t before = network.junction(junction)->connections.size(); + ASSERT_EQ(before, 6U); // 3 two-way single-lane arms: every ordered pair + + widen_west_to_east(network, t); + + // The case that used to be refused outright ("delete and recreate the + // junction"), which is what GW-2 step 12's turn lane needs. + auto regen = roadmaker::edit::regenerate_junction(network, junction); + expect_command_round_trip(network, *regen); + ASSERT_TRUE(regen->apply(network).has_value()); + + // Exactly one turn appears: west→east gains a second lane pairing, while + // west→south still bottlenecks on south's single outgoing lane. + EXPECT_EQ(network.junction(junction)->connections.size(), before + 1); + EXPECT_EQ(connections_with_incoming(network, junction, t.west), 3); + + // The junction is not merely bigger — it is welded and exportable. + const auto welds = roadmaker::edit::verify_junction_welds(network, junction); + ASSERT_TRUE(welds.has_value()); + EXPECT_FALSE(welds->breaches); + const auto written = roadmaker::write_xodr(network, "grown"); + ASSERT_TRUE(written.has_value()); + const auto reloaded = roadmaker::parse_xodr(*written, "grown"); + ASSERT_TRUE(reloaded.has_value()); + EXPECT_EQ(roadmaker::count_errors(reloaded->diagnostics), 0U); +} + +TEST(EditOperations, RegenerateJunctionShrinksTheTurnSetAndUndoResurrectsTheExactIds) { + RoadNetwork network; + const TJunction t = make_t_junction(network); + ASSERT_TRUE(roadmaker::edit::create_junction(network, t.ends)->apply(network).has_value()); + const JunctionId junction = network.find_junction("1"); + widen_west_to_east(network, t); + ASSERT_TRUE(roadmaker::edit::regenerate_junction(network, junction)->apply(network).has_value()); + + const std::size_t grown = network.junction(junction)->connections.size(); + std::vector ids_before; + for (const JunctionConnection& connection : network.junction(junction)->connections) { + ids_before.push_back(connection.connecting_road); + } + + // Take the west lane back out of service: the turn it carried has to go, and + // its connecting road with it. + retype_lane(network, t.west, -3, roadmaker::LaneType::Shoulder); + + auto regen = roadmaker::edit::regenerate_junction(network, junction); + const std::string before_regen = snapshot_xodr(network); + ASSERT_TRUE(regen->apply(network).has_value()); + EXPECT_LT(network.junction(junction)->connections.size(), grown); + + // A connecting road actually went away — not merely dropped from the table + // but erased from the arena. Without this a regen that unlinks the road but + // leaks it still passes the count and undo checks. + const auto still_alive = + std::ranges::count_if(ids_before, [&](RoadId id) { return network.road(id) != nullptr; }); + EXPECT_LT(static_cast(still_alive), ids_before.size()) + << "a dropped connecting road must be erased, not orphaned"; + + // Undo restores the table AND resurrects the erased connecting roads under + // their original ids — erase_exact reserves the slot precisely so this holds. + ASSERT_TRUE(regen->revert(network).has_value()); + EXPECT_EQ(snapshot_xodr(network), before_regen); + ASSERT_EQ(network.junction(junction)->connections.size(), grown); + for (std::size_t i = 0; i < ids_before.size(); ++i) { + EXPECT_EQ(network.junction(junction)->connections[i].connecting_road, ids_before[i]); + EXPECT_NE(network.road(ids_before[i]), nullptr) << "original connecting id resurrected"; + } +} + +/// The connecting road serving the `from`→`to` movement, or an invalid id. +/// The outgoing road is not stored on the connection — it lives only on the +/// connecting road's successor link. +RoadId connecting_road_for(const RoadNetwork& network, JunctionId junction, RoadId from, RoadId to) { + for (const JunctionConnection& connection : network.junction(junction)->connections) { + if (connection.incoming_road != from) { + continue; + } + const roadmaker::Road* road = network.road(connection.connecting_road); + if (road == nullptr || !road->successor.has_value()) { + continue; + } + if (const RoadId* target = std::get_if(&road->successor->target); + target != nullptr && *target == to) { + return connection.connecting_road; + } + } + return RoadId{}; +} + +TEST(EditOperations, RegenerateJunctionKeepsTheIdsOfTurnsAGrowthDoesNotTouch) { + RoadNetwork network; + const TJunction t = make_t_junction(network); + ASSERT_TRUE(roadmaker::edit::create_junction(network, t.ends)->apply(network).has_value()); + const JunctionId junction = network.find_junction("1"); + + // east→south uses east's INCOMING lanes and south's OUTGOING lanes; south→west + // uses south's incoming and west's outgoing. widen_west_to_east touches + // neither set, so keyed matching must reuse both connecting roads rather than + // churn every id whenever the turn set changes at all. (Movements arriving at + // east DO legitimately re-target: a new outgoing lane changes which lane the + // discipline rule picks.) + const RoadId east_south = connecting_road_for(network, junction, t.east, t.south); + const RoadId south_west = connecting_road_for(network, junction, t.south, t.west); + ASSERT_TRUE(east_south.is_valid()); + ASSERT_TRUE(south_west.is_valid()); + + widen_west_to_east(network, t); + ASSERT_TRUE(roadmaker::edit::regenerate_junction(network, junction)->apply(network).has_value()); + + EXPECT_EQ(connecting_road_for(network, junction, t.east, t.south), east_south); + EXPECT_EQ(connecting_road_for(network, junction, t.south, t.west), south_west); + EXPECT_NE(network.road(east_south), nullptr); + EXPECT_NE(network.road(south_west), nullptr); +} + +TEST(EditOperations, RegenerateJunctionInPlaceOnlyStillRefusesAChangedTurnSet) { + RoadNetwork network; + const TJunction t = make_t_junction(network); + ASSERT_TRUE(roadmaker::edit::create_junction(network, t.ends)->apply(network).has_value()); + const JunctionId junction = network.find_junction("1"); + widen_west_to_east(network, t); + + // The per-frame preview path (move_waypoint_following_junctions) asks for + // this: creating connecting roads there would reserve arena slots on every + // discarded drag frame. + expect_command_rejected( + network, + roadmaker::edit::regenerate_junction( + network, junction, {}, roadmaker::edit::TurnSetPolicy::InPlaceOnly)); +} + +TEST(EditOperations, AddLaneNamesTheJunctionsItsRoadFeeds) { + RoadNetwork network; + const TJunction t = make_t_junction(network); + ASSERT_TRUE(roadmaker::edit::create_junction(network, t.ends)->apply(network).has_value()); + const JunctionId junction = network.find_junction("1"); + + // Without this the editor's regeneration loop iterates an empty list, so the + // junction stays stale no matter what the loop's skip condition says. + auto add = roadmaker::edit::add_lane( + network, network.road(t.west)->sections.front(), -1, roadmaker::LaneType::Driving); + const roadmaker::edit::DirtySet dirty = add->dirty(); + ASSERT_EQ(dirty.junctions.size(), 1U); + EXPECT_EQ(dirty.junctions[0], junction); + EXPECT_FALSE(dirty.junctions_are_current) << "add_lane does not regenerate; the editor does"; } TEST(EditOperations, RegenerateJunctionTracksMovedIncomingEnd) { diff --git a/editor/src/document/document.cpp b/editor/src/document/document.cpp index d11f6fe2..a30a142f 100644 --- a/editor/src/document/document.cpp +++ b/editor/src/document/document.cpp @@ -34,7 +34,13 @@ std::string describe_dirty(const edit::DirtySet& dirty) { for (std::size_t i = 0; i < dirty.junctions.size(); ++i) { text += (i == 0 ? "" : ",") + std::to_string(dirty.junctions[i].index); } - text += dirty.topology ? "] topology" : "]"; + text += "]"; + if (dirty.topology) { + text += " topology"; + } + if (dirty.junctions_are_current) { + text += " junctions_are_current"; + } return text; } @@ -167,17 +173,22 @@ void Document::push_applied_with_regeneration(std::unique_ptr com last_dirty_ = dirty; // the primary edit's dirty set (before regenerations) spdlog::info("command: {} {}", command->name(), describe_dirty(dirty)); - // Editing an incoming road (geometry, elevation) regenerates every junction - // it touches (02 §6): re-run the generator from each junction's recorded - // arms, replacing the connecting-road geometry in place. Junctions loaded - // from foreign files have no recorded arms and are left untouched. - // Topology commands (create/delete junction, split, delete road) are - // skipped: they list their own junction as dirty but must not self- or - // double-regenerate — the create already built the connecting roads. + // Editing an incoming road (geometry, elevation, or its lanes) regenerates + // every junction it touches (02 §6): re-run the generator from each + // junction's recorded arms, replacing the connecting-road geometry — and, + // since P2, the turn set — in place. Junctions loaded from foreign files + // have no recorded arms and are left untouched. + // + // Commands that built their own junction structure (create/delete junction, + // split, delete road) say so with junctions_are_current and are skipped: + // they list their junction as dirty for re-meshing, but regenerating it + // would fight the structure they just wrote. That used to key off + // `topology`, which cannot express "a lane appeared AND the junction needs + // regenerating" — the case Lane Add and Lane Carve are made of. std::vector> regenerations; bool announced = false; for (const JunctionId junction_id : dirty.junctions) { - if (dirty.topology || already_regenerated) { + if (dirty.junctions_are_current || already_regenerated) { break; } const Junction* junction = network_.junction(junction_id); @@ -218,6 +229,12 @@ void Document::push_applied_with_regeneration(std::unique_ptr com dirty.roads.push_back(road); } } + // A regeneration that changed the turn set created or erased connecting + // roads, so it is topology in its own right — and the primary edit (a lane + // added, say) never said so. Without this the mesh takes the partial + // per-road path, which cannot add or drop an item, and prune_stale never + // runs, leaving a selection pointing at an erased road. + dirty.topology = dirty.topology || regen_dirty.topology; regenerations.push_back(std::move(regen)); } diff --git a/editor/tests/test_junction_regen.cpp b/editor/tests/test_junction_regen.cpp index ac742941..5ba0e499 100644 --- a/editor/tests/test_junction_regen.cpp +++ b/editor/tests/test_junction_regen.cpp @@ -194,3 +194,33 @@ TEST(JunctionRegen, DirectCommitPathMatchesTheDragResult) { ASSERT_TRUE(welds.has_value()); EXPECT_FALSE(welds->breaches); } + +// The P2 case: a lane added to a junction arm used to make the regeneration +// refuse ("changed the connection count") and the editor swallow it into a +// warning toast, freezing the junction. It must now regenerate silently, and +// the junction must genuinely grow. +TEST(JunctionRegen, AddingADrivingLaneToAnArmRegeneratesWithoutAToast) { + JunctionScene scene; + const std::size_t before = + scene.document.network().junction(scene.junction)->connections.size(); + QSignalSpy skipped(&scene.document, &Document::regeneration_skipped); + + // One extra outgoing lane on east and one extra incoming lane on west open a + // second west->east movement; the min(incoming, outgoing) pairing needs both. + const auto add = [&](RoadId road, int side) { + return scene.document.push_command( + roadmaker::edit::add_lane(scene.document.network(), + scene.document.network().road(road)->sections.front(), + side, + roadmaker::LaneType::Driving)); + }; + ASSERT_TRUE(add(scene.west, -1).has_value()); + ASSERT_TRUE(add(scene.east, 1).has_value()); + + EXPECT_EQ(skipped.count(), 0) << "no 'junction not updated' toast"; + EXPECT_GT(scene.document.network().junction(scene.junction)->connections.size(), before); + const auto welds = + roadmaker::edit::verify_junction_welds(scene.document.network(), scene.junction); + ASSERT_TRUE(welds.has_value()); + EXPECT_FALSE(welds->breaches); +} From 84563904c6388be3366bfc83b74baf503cf69821 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Thu, 16 Jul 2026 01:23:42 -0700 Subject: [PATCH 2/4] feat(core): interior insert_lane with renumbering and link remap Lane Form and Lane Carve need a lane that starts partway across the carriageway, which add_lane (outermost-only) cannot author. insert_lane places a lane at a given odr id and renumbers every lane already at or outside it one step further out. Because odr id IS the lateral position, an interior insert necessarily renumbers, and everything that named the shifted lanes by id is remapped rather than cleared: adjacent-section predecessor/successor links (the writer refuses a dangling one in either direction) and junction lane_links. The inserted lane is a new lane, so it does not link back to the neighbouring sections (asam new_lane_appear). All of it runs inside the command creator, so apply/revert stays byte-identical. Also exposes it through the Python bindings and the soak driver, adds the TurnSetPolicy enum and junctions_are_current to the bindings, and amends the frozen M2 editing-tools doc with the turn-set-change decision. Author: Armando Anaya --- core/include/roadmaker/edit/operations.hpp | 15 ++ core/src/edit/operations.cpp | 159 ++++++++++++++++++++- core/tests/test_edit_operations.cpp | 49 +++++-- core/tests/test_lane_sections.cpp | 65 +++++++++ docs/design/m2/02_editing_tools.md | 14 ++ editor/tests/soak/soak_driver.cpp | 31 ++++ editor/tests/soak/soak_driver.hpp | 1 + editor/tests/test_junction_regen.cpp | 3 +- python/src/bindings.cpp | 29 +++- python/tests/test_lane_sections.py | 51 +++++++ 10 files changed, 394 insertions(+), 23 deletions(-) diff --git a/core/include/roadmaker/edit/operations.hpp b/core/include/roadmaker/edit/operations.hpp index bd8ae695..d9f2f3a1 100644 --- a/core/include/roadmaker/edit/operations.hpp +++ b/core/include/roadmaker/edit/operations.hpp @@ -341,6 +341,21 @@ add_lane(const RoadNetwork& network, LaneSectionId section, int side, LaneType t /// referencing the lane are cleared (and restored exactly on undo). [[nodiscard]] RM_API std::unique_ptr remove_lane(const RoadNetwork& network, LaneId lane); +/// Inserts a lane at `at_odr_id`, renumbering every lane already at or outside +/// that position one step further out — where add_lane only ever appends the +/// outermost. `at_odr_id` must name a lane that exists (numbering stays +/// contiguous) and share its sign with the side it lands on; the center lane +/// (0) cannot be displaced. +/// +/// The inserted lane does NOT link to the neighbouring sections: a lane that +/// appears mid-road is a new lane, not a continuation +/// (asam.net:xodr:1.4.0:road.lane.link.new_lane_appear). The lanes it pushes +/// outward keep their own links, and everything that named them by id — +/// adjacent-section predecessor/successor and junction lane_links — is +/// remapped to the new numbering (restored exactly on undo). +[[nodiscard]] RM_API std::unique_ptr +insert_lane(const RoadNetwork& network, LaneSectionId section, int at_odr_id, LaneType type); + [[nodiscard]] RM_API std::unique_ptr set_lane_type(const RoadNetwork& network, LaneId lane, LaneType type); diff --git a/core/src/edit/operations.cpp b/core/src/edit/operations.cpp index 2bf294a0..073ea805 100644 --- a/core/src/edit/operations.cpp +++ b/core/src/edit/operations.cpp @@ -355,7 +355,8 @@ class CompositeCommand final : public Command { // One child that built its own junctions speaks for the composite: the // assemblies and attach_t_junction end in create_junction, so the // junctions they name are already generated. - dirty.junctions_are_current = dirty.junctions_are_current || child_dirty.junctions_are_current; + dirty.junctions_are_current = + dirty.junctions_are_current || child_dirty.junctions_are_current; } return dirty; } @@ -2585,8 +2586,7 @@ std::unique_ptr regenerate_junction(const RoadNetwork& network, if (!plan.has_value()) { return invalid_command(std::string(kName), plan.error()); } - if (policy == TurnSetPolicy::InPlaceOnly && - plan->roads.size() != junction->connections.size()) { + if (policy == TurnSetPolicy::InPlaceOnly && plan->roads.size() != junction->connections.size()) { return fail("regeneration changed the connection count; delete and recreate the junction"); } @@ -2639,6 +2639,7 @@ std::unique_ptr regenerate_junction(const RoadNetwork& network, ConnectingPlan cp; RoadId road; }; + std::vector matched_turns; std::vector new_turns; std::vector claimed(junction->connections.size(), false); @@ -2822,10 +2823,8 @@ std::unique_ptr delete_junction(const RoadNetwork& network, JunctionId // junctions_are_current: the junction named here is the one being deleted, // and capture_deletion strips the doomed connections from every survivor — // there is nothing left to regenerate against. - DirtySet dirty{.roads = doomed, - .junctions = {junction_id}, - .topology = true, - .junctions_are_current = true}; + DirtySet dirty{ + .roads = doomed, .junctions = {junction_id}, .topology = true, .junctions_are_current = true}; for (const RoadId doomed_id : doomed) { for (const JunctionId touched : junctions_touching(network, doomed_id)) { if (std::ranges::find(dirty.junctions, touched) == dirty.junctions.end()) { @@ -3261,6 +3260,152 @@ std::unique_ptr remove_lane(const RoadNetwork& network, LaneId lane_id) return command; } +std::unique_ptr +insert_lane(const RoadNetwork& network, LaneSectionId section_id, int at_odr_id, LaneType type) { + static constexpr std::string_view kName = "Insert Lane"; + const auto fail = [&](std::string message) { + return invalid_command( + std::string(kName), + Error{.code = ErrorCode::InvalidArgument, .message = std::move(message)}); + }; + const LaneSection* section = network.lane_section(section_id); + if (section == nullptr) { + return fail("stale lane-section id"); + } + if (at_odr_id == 0) { + return fail("cannot insert at the center lane"); + } + const int side = at_odr_id > 0 ? 1 : -1; + bool occupied = false; + for (const LaneId lane_id : section->lanes) { + if (network.lane(lane_id)->odr_id == at_odr_id) { + occupied = true; + break; + } + } + if (!occupied) { + // Numbering stays contiguous; appending past the outermost is add_lane. + return fail("no lane at that position to insert before; use add_lane to append"); + } + + // Everything at or outside the insert point on this side steps one further + // out. `new = old + side` for both sides (more negative on the right, more + // positive on the left), and since it is a contiguous outer block the shift + // preserves the section's descending lane order. + const auto shifted = [&](int odr) { + return odr != 0 && (side > 0 ? odr >= at_odr_id : odr <= at_odr_id); + }; + + const RoadId road_id = section->road; + DirtySet dirty{ + .roads = {road_id}, .junctions = junctions_touching(network, road_id), .topology = true}; + auto command = std::make_unique(std::string(kName), std::move(dirty)); + + // `before` snapshots everything the creator mutates so the engine can revert + // it and re-read `after` from the network. The section's lane list gains the + // new lane; the shifted lanes change odr id. + command->before.sections.emplace_back(section_id, *section); + for (const LaneId lane_id : section->lanes) { + if (shifted(network.lane(lane_id)->odr_id)) { + command->before.lanes.emplace_back(lane_id, *network.lane(lane_id)); + } + } + + // Adjacent-section links that named a shifted lane by id are remapped, not + // cleared — the lanes still continue, just under new numbers. The writer + // refuses a dangling intra-road link in either direction. + const Road& road = *network.road(road_id); + const auto here = std::ranges::find(road.sections, section_id); + const auto capture_neighbor = [&](LaneSectionId neighbor_id, bool forward) { + for (const LaneId neighbor_lane_id : network.lane_section(neighbor_id)->lanes) { + const Lane& lane = *network.lane(neighbor_lane_id); + const std::optional& link = forward ? lane.successor : lane.predecessor; + if (link.has_value() && shifted(*link)) { + command->before.lanes.emplace_back(neighbor_lane_id, lane); + } + } + }; + if (here != road.sections.begin()) { + capture_neighbor(*std::prev(here), /*forward=*/true); + } + if (here != road.sections.end() && std::next(here) != road.sections.end()) { + capture_neighbor(*std::next(here), /*forward=*/false); + } + + // Junction lane_links that named a shifted lane by id are remapped too. + std::vector touched_junctions; + network.for_each_junction([&](JunctionId junction_id, const Junction& junction) { + const bool touched = std::ranges::any_of(junction.connections, [&](const auto& connection) { + return std::ranges::any_of(connection.lane_links, [&](const std::pair& link) { + return (connection.incoming_road == road_id && shifted(link.first)) || + (connection.connecting_road == road_id && shifted(link.second)); + }); + }); + if (touched) { + command->before.junctions.emplace_back(junction_id, junction); + touched_junctions.push_back(junction_id); + } + }); + + command->creator = [section_id, + at_odr_id, + side, + type, + road_id, + touched_junctions = std::move(touched_junctions)]( + RoadNetwork& target, Values& created) -> Expected { + const auto shift = [&](int odr) { + return odr != 0 && (side > 0 ? odr >= at_odr_id : odr <= at_odr_id) ? odr + side : odr; + }; + // 1. Renumber the outer block. Mutating odr id in place keeps the section's + // descending order because the block is contiguous. + for (const LaneId lane_id : target.lane_section(section_id)->lanes) { + Lane& lane = *target.lane(lane_id); + lane.odr_id = shift(lane.odr_id); + } + // 2. Add the new lane at the now-free position. It appears mid-road, so it + // is not linked back to either neighbouring section. + const LaneId lane_id = target.add_lane(section_id, at_odr_id, type); + if (!lane_id.is_valid()) { + return make_error(ErrorCode::InvalidArgument, "insert position is still occupied"); + } + target.lane(lane_id)->widths = {Poly3{.a = 3.5}}; + created.lanes.emplace_back(lane_id, Lane{}); + // 3. Remap every link that named a shifted lane by id. + const Road& road = *target.road(road_id); + const auto here = std::ranges::find(road.sections, section_id); + const auto remap_neighbor = [&](LaneSectionId neighbor_id, bool forward) { + for (const LaneId neighbor_lane_id : target.lane_section(neighbor_id)->lanes) { + Lane& lane = *target.lane(neighbor_lane_id); + std::optional& link = forward ? lane.successor : lane.predecessor; + if (link.has_value()) { + *link = shift(*link); + } + } + }; + if (here != road.sections.begin()) { + remap_neighbor(*std::prev(here), /*forward=*/true); + } + if (here != road.sections.end() && std::next(here) != road.sections.end()) { + remap_neighbor(*std::next(here), /*forward=*/false); + } + for (const JunctionId junction_id : touched_junctions) { + for (JunctionConnection& connection : target.junction(junction_id)->connections) { + for (std::pair& link : connection.lane_links) { + if (connection.incoming_road == road_id) { + link.first = shift(link.first); + } + if (connection.connecting_road == road_id) { + link.second = shift(link.second); + } + } + } + } + return {}; + }; + return command; +} + std::unique_ptr set_lane_type(const RoadNetwork& network, LaneId lane_id, LaneType type) { static constexpr std::string_view kName = "Set Lane Type"; auto context = lane_context(network, lane_id); diff --git a/core/tests/test_edit_operations.cpp b/core/tests/test_edit_operations.cpp index 34d2f1af..6215baaa 100644 --- a/core/tests/test_edit_operations.cpp +++ b/core/tests/test_edit_operations.cpp @@ -1136,10 +1136,8 @@ void retype_lane(RoadNetwork& network, RoadId road, int odr_id, roadmaker::LaneT } void add_outermost_lane(RoadNetwork& network, RoadId road, int side) { - ASSERT_TRUE(roadmaker::edit::add_lane(network, - network.road(road)->sections.front(), - side, - roadmaker::LaneType::Driving) + ASSERT_TRUE(roadmaker::edit::add_lane( + network, network.road(road)->sections.front(), side, roadmaker::LaneType::Driving) ->apply(network) .has_value()); } @@ -1233,7 +1231,8 @@ TEST(EditOperations, RegenerateJunctionShrinksTheTurnSetAndUndoResurrectsTheExac /// The connecting road serving the `from`→`to` movement, or an invalid id. /// The outgoing road is not stored on the connection — it lives only on the /// connecting road's successor link. -RoadId connecting_road_for(const RoadNetwork& network, JunctionId junction, RoadId from, RoadId to) { +RoadId +connecting_road_for(const RoadNetwork& network, JunctionId junction, RoadId from, RoadId to) { for (const JunctionConnection& connection : network.junction(junction)->connections) { if (connection.incoming_road != from) { continue; @@ -1286,10 +1285,9 @@ TEST(EditOperations, RegenerateJunctionInPlaceOnlyStillRefusesAChangedTurnSet) { // The per-frame preview path (move_waypoint_following_junctions) asks for // this: creating connecting roads there would reserve arena slots on every // discarded drag frame. - expect_command_rejected( - network, - roadmaker::edit::regenerate_junction( - network, junction, {}, roadmaker::edit::TurnSetPolicy::InPlaceOnly)); + expect_command_rejected(network, + roadmaker::edit::regenerate_junction( + network, junction, {}, roadmaker::edit::TurnSetPolicy::InPlaceOnly)); } TEST(EditOperations, AddLaneNamesTheJunctionsItsRoadFeeds) { @@ -1308,6 +1306,39 @@ TEST(EditOperations, AddLaneNamesTheJunctionsItsRoadFeeds) { EXPECT_FALSE(dirty.junctions_are_current) << "add_lane does not regenerate; the editor does"; } +TEST(EditOperations, InsertLaneRemapsTheJunctionLaneLinksThatNamedTheShiftedLane) { + RoadNetwork network; + const TJunction t = make_t_junction(network); + ASSERT_TRUE(roadmaker::edit::create_junction(network, t.ends)->apply(network).has_value()); + const JunctionId junction = network.find_junction("1"); + + // Every generated connection off an End-contact arm links its incoming lane + // -1. Inserting a lane at -1 pushes that lane to -2, so the lane_links naming + // it must follow — otherwise, even before the editor regenerates, the + // junction references a lane that has moved. + const auto west_links_name = [&](int odr) { + for (const JunctionConnection& connection : network.junction(junction)->connections) { + if (connection.incoming_road == t.west) { + for (const auto& [from, to] : connection.lane_links) { + if (from == odr) { + return true; + } + } + } + } + return false; + }; + ASSERT_TRUE(west_links_name(-1)); + + auto insert = roadmaker::edit::insert_lane( + network, network.road(t.west)->sections.front(), -1, roadmaker::LaneType::Driving); + expect_command_round_trip(network, *insert); + ASSERT_TRUE(insert->apply(network).has_value()); + + EXPECT_TRUE(west_links_name(-2)) << "the lane_link followed the renumbering"; + EXPECT_FALSE(west_links_name(-1)) << "nothing still names the old id (now the new lane)"; +} + TEST(EditOperations, RegenerateJunctionTracksMovedIncomingEnd) { RoadNetwork network; const TJunction t = make_t_junction(network); diff --git a/core/tests/test_lane_sections.cpp b/core/tests/test_lane_sections.cpp index 30f2af26..d6f8df1e 100644 --- a/core/tests/test_lane_sections.cpp +++ b/core/tests/test_lane_sections.cpp @@ -586,3 +586,68 @@ TEST(LaneSections, SetLaneWidthRefusesToFlattenAWidthThatVariesAlongS) { expect_rejected(network, roadmaker::edit::set_lane_width(network, driving_id, 4.0)); EXPECT_EQ(network.lane(driving_id)->widths.size(), 2U) << "the second record must survive"; } + +// --- insert_lane: interior insert with renumbering (P2 #263) ---------------- + +/// odr ids present on a section, in the section's stored (descending) order. +std::vector odr_ids(const RoadNetwork& network, LaneSectionId section_id) { + std::vector ids; + for (const LaneId lane_id : network.lane_section(section_id)->lanes) { + ids.push_back(network.lane(lane_id)->odr_id); + } + return ids; +} + +TEST(LaneSections, InsertLaneRenumbersTheOuterBlockAndRoundTrips) { + RoadNetwork network; + const RoadId road_id = author_straight(network, "1"); + const LaneSectionId first = network.road(road_id)->sections[0]; + ASSERT_EQ(odr_ids(network, first), (std::vector{1, 0, -1, -2})); + + // Insert at -1: the old -1 and -2 step out to -2 and -3, and a fresh lane + // takes -1. The left side and center are untouched. + auto command = roadmaker::edit::insert_lane(network, first, -1, LaneType::Driving); + expect_command_round_trip(network, *command); + ASSERT_TRUE(command->apply(network).has_value()); + EXPECT_EQ(odr_ids(network, first), (std::vector{1, 0, -1, -2, -3})); +} + +TEST(LaneSections, InsertLaneLeavesTheNewLaneUnlinkedAndRemapsNeighbourLinks) { + RoadNetwork network; + const RoadId road_id = author_straight(network, "1"); + ASSERT_TRUE( + roadmaker::edit::split_lane_section(network, road_id, 60.0)->apply(network).has_value()); + const LaneSectionId head = network.road(road_id)->sections[0]; + const LaneSectionId tail = network.road(road_id)->sections[1]; + + // Before: the head's -1 continues into the tail's -1, both ways. + ASSERT_EQ(lane_by_odr(network, head, -1)->successor, -1); + ASSERT_EQ(lane_by_odr(network, tail, -1)->predecessor, -1); + + auto command = roadmaker::edit::insert_lane(network, head, -1, LaneType::Driving); + expect_command_round_trip(network, *command); + ASSERT_TRUE(command->apply(network).has_value()); + + // The inserted lane appears mid-road: no continuation into either neighbour. + const Lane* inserted = lane_by_odr(network, head, -1); + EXPECT_FALSE(inserted->predecessor.has_value()); + EXPECT_FALSE(inserted->successor.has_value()); + + // The lane that used to be -1 is now -2 and still continues into the tail's + // -1 (the tail was not renumbered); the tail's predecessor link followed the + // renumbering from -1 to -2. + EXPECT_EQ(lane_by_odr(network, head, -2)->successor, -1); + EXPECT_EQ(lane_by_odr(network, tail, -1)->predecessor, -2); +} + +TEST(LaneSections, InsertLaneRejectsBadPositions) { + RoadNetwork network; + const RoadId road_id = author_straight(network, "1"); + const LaneSectionId first = network.road(road_id)->sections[0]; + + expect_rejected(network, roadmaker::edit::insert_lane(network, first, 0, LaneType::Driving)); + // No lane at -5: appending past the outermost is add_lane's job. + expect_rejected(network, roadmaker::edit::insert_lane(network, first, -5, LaneType::Driving)); + expect_rejected(network, + roadmaker::edit::insert_lane(network, LaneSectionId{}, -1, LaneType::Driving)); +} diff --git a/docs/design/m2/02_editing_tools.md b/docs/design/m2/02_editing_tools.md index ab34d61d..75180e1f 100644 --- a/docs/design/m2/02_editing_tools.md +++ b/docs/design/m2/02_editing_tools.md @@ -253,6 +253,20 @@ restore-in-place so undo stays valid). > the count (a lane added/removed on an arm) returns an error asking for a > recreate rather than reallocating IDs. Kernel surface: `preview_junction` > (non-mutating count + dropped-turn report) and `regenerate_junction`. +> +> **Amended (P2, #263 — maintainer decision 2026-07-15).** The count-must-be- +> unchanged restriction is lifted. `regenerate_junction` now takes a +> `TurnSetPolicy`: under `AllowChange` (default) a lane added to, removed from, +> or retyped on an arm grows or shrinks the turn set — new connecting roads are +> created, vanished ones erased, and the turns that survive keep their IDs +> (keyed matching). The old refusal remains under `InPlaceOnly`, which the +> per-frame node-drag preview uses because creating connecting roads on a +> command that is reverted and discarded every frame would leak arena slots. +> The dirty-set contract gained `junctions_are_current` to distinguish "this +> command already built its junctions" (create/delete/split) from "this +> command is topology **and** needs the junction regenerated" (a lane +> appearing) — a distinction `topology` alone could not express. Detail: the +> [P2 discovery report](../../../roadmap/pillars/p2_discovery.md) §4. **Edge cases.** Ends too far apart (> configurable 50 m): factory error. Arms nearly parallel: connecting-road fit may loop — generator drops turn pairs whose diff --git a/editor/tests/soak/soak_driver.cpp b/editor/tests/soak/soak_driver.cpp index 3144d573..18fc873b 100644 --- a/editor/tests/soak/soak_driver.cpp +++ b/editor/tests/soak/soak_driver.cpp @@ -68,6 +68,7 @@ void SoakDriver::step(int index) { {1, &SoakDriver::op_attach_t, "attach_t"}, {1, &SoakDriver::op_assembly_drop_on_road, "assembly_drop_on_road"}, {1, &SoakDriver::op_remove_lane, "remove_lane"}, + {2, &SoakDriver::op_insert_lane, "insert_lane"}, {1, &SoakDriver::op_overpass, "overpass"}, {1, &SoakDriver::op_delete_crossing_road, "delete_crossing_road"}, {1, &SoakDriver::op_delete_junction, "delete_junction"}, @@ -699,6 +700,36 @@ void SoakDriver::op_remove_lane() { push(edit::remove_lane(document_.network(), outermost)); } +void SoakDriver::op_insert_lane() { + // Interior insert with renumbering — the path Lane Form/Carve build on. Picks + // a random existing non-center lane as the insert point so the renumbering, + // adjacent-section link remap, and junction lane_link remap all run; the + // kernel refuses the center and any empty position (recorded, not fatal). + const std::vector roads = live_roads(/*editable_only=*/true); + if (roads.empty()) { + return; + } + const RoadId road_id = roads[static_cast(rand_int(0, int(roads.size()) - 1))]; + const Road* road = document_.network().road(road_id); + if (road == nullptr || road->sections.empty()) { + return; + } + const LaneSectionId section_id = + road->sections[static_cast(rand_int(0, int(road->sections.size()) - 1))]; + const LaneSection* section = document_.network().lane_section(section_id); + if (section == nullptr || section->lanes.empty()) { + return; + } + const LaneId at = + section->lanes[static_cast(rand_int(0, int(section->lanes.size()) - 1))]; + const Lane* lane = document_.network().lane(at); + if (lane == nullptr) { + return; + } + const LaneType type = chance(0.5) ? LaneType::Driving : LaneType::Shoulder; + push(edit::insert_lane(document_.network(), section_id, lane->odr_id, type)); +} + void SoakDriver::op_overpass() { // Apply the overpass elevation profile where a road crosses another (gate // finding 4): the SAME headless path ProfilePanel::apply_overpass drives. It diff --git a/editor/tests/soak/soak_driver.hpp b/editor/tests/soak/soak_driver.hpp index 37bd0d1d..857af4f0 100644 --- a/editor/tests/soak/soak_driver.hpp +++ b/editor/tests/soak/soak_driver.hpp @@ -95,6 +95,7 @@ class SoakDriver { void op_attach_t(); void op_assembly_drop_on_road(); void op_remove_lane(); + void op_insert_lane(); void op_overpass(); void op_delete_crossing_road(); void op_delete_junction(); diff --git a/editor/tests/test_junction_regen.cpp b/editor/tests/test_junction_regen.cpp index 5ba0e499..f953892a 100644 --- a/editor/tests/test_junction_regen.cpp +++ b/editor/tests/test_junction_regen.cpp @@ -201,8 +201,7 @@ TEST(JunctionRegen, DirectCommitPathMatchesTheDragResult) { // the junction must genuinely grow. TEST(JunctionRegen, AddingADrivingLaneToAnArmRegeneratesWithoutAToast) { JunctionScene scene; - const std::size_t before = - scene.document.network().junction(scene.junction)->connections.size(); + const std::size_t before = scene.document.network().junction(scene.junction)->connections.size(); QSignalSpy skipped(&scene.document, &Document::regeneration_skipped); // One extra outgoing lane on east and one extra incoming lane on west open a diff --git a/python/src/bindings.cpp b/python/src/bindings.cpp index 668f8a6a..d269af2f 100644 --- a/python/src/bindings.cpp +++ b/python/src/bindings.cpp @@ -836,7 +836,8 @@ NB_MODULE(_roadmaker, m) { nb::class_(edit, "DirtySet") .def_ro("roads", &roadmaker::edit::DirtySet::roads) .def_ro("junctions", &roadmaker::edit::DirtySet::junctions) - .def_ro("topology", &roadmaker::edit::DirtySet::topology); + .def_ro("topology", &roadmaker::edit::DirtySet::topology) + .def_ro("junctions_are_current", &roadmaker::edit::DirtySet::junctions_are_current); nb::class_(edit, "Command") .def_prop_ro( @@ -1249,18 +1250,25 @@ NB_MODULE(_roadmaker, m) { "T-junction workflow: splits the target around s, deletes the middle " "stub, and generates a junction from the three ends (all legal turns). " "One undoable command."); + nb::enum_(edit, "TurnSetPolicy") + .value("ALLOW_CHANGE", roadmaker::edit::TurnSetPolicy::AllowChange) + .value("IN_PLACE_ONLY", roadmaker::edit::TurnSetPolicy::InPlaceOnly); edit.def( "regenerate_junction", [](const roadmaker::RoadNetwork& network, roadmaker::JunctionId junction, - const roadmaker::edit::JunctionGenOptions& options) { - return roadmaker::edit::regenerate_junction(network, junction, options); + const roadmaker::edit::JunctionGenOptions& options, + roadmaker::edit::TurnSetPolicy policy) { + return roadmaker::edit::regenerate_junction(network, junction, options, policy); }, "network"_a, "junction"_a, "options"_a = roadmaker::edit::JunctionGenOptions{}, - "Re-runs the generator from the junction's recorded arms, replacing " - "connecting-road geometry in place (ids preserved)."); + "policy"_a = roadmaker::edit::TurnSetPolicy::AllowChange, + "Re-runs the generator from the junction's recorded arms. Under " + "ALLOW_CHANGE (default) a lane added, removed, or retyped on an incoming " + "road grows or shrinks the turn set; the turns that survive keep their " + "connecting-road ids. IN_PLACE_ONLY refuses any turn-set change."); edit.def("delete_junction", &roadmaker::edit::delete_junction, "network"_a, "junction"_a); // --- parametric intersection assemblies (rm.edit.assembly) --- @@ -1351,6 +1359,17 @@ NB_MODULE(_roadmaker, m) { "lane"_a, "Outermost lane of its side only; adjacent-section links and junction " "lane_links referencing the lane are cleared (undo restores them)."); + edit.def("insert_lane", + &roadmaker::edit::insert_lane, + "network"_a, + "section"_a, + "at_odr_id"_a, + "type"_a, + "Inserts a lane at at_odr_id, renumbering every lane already at or " + "outside it one step further out (add_lane only appends the " + "outermost). The new lane does not link to the neighbouring sections; " + "adjacent-section links and junction lane_links that named a shifted " + "lane are remapped to the new numbering."); edit.def("set_lane_type", &roadmaker::edit::set_lane_type, "network"_a, "lane"_a, "type"_a); edit.def("set_lane_width", &roadmaker::edit::set_lane_width, diff --git a/python/tests/test_lane_sections.py b/python/tests/test_lane_sections.py index 37c71046..feb49125 100644 --- a/python/tests/test_lane_sections.py +++ b/python/tests/test_lane_sections.py @@ -180,3 +180,54 @@ def test_split_survives_an_xodr_round_trip(network): assert not [d for d in diagnostics if d.severity == rm.Severity.ERROR] assert len(back.road(back.find_road("1")).sections) == 2 assert rm.write_xodr(back) == text + + +def odr_ids(net, section): + return sorted(net.lane(lane_id).odr_id for lane_id in net.lane_section(section).lanes) + + +def test_insert_lane_renumbers_the_outer_block(network): + road = network.find_road("1") + section = network.road(road).sections[0] + assert odr_ids(network, section) == [-2, -1, 0, 1] + before = rm.write_xodr(network) + stack = rm.edit.EditStack() + + # Insert at -1: the old -1 and -2 step out, a fresh lane takes -1. + stack.push(network, rm.edit.insert_lane(network, section, -1, rm.LaneType.DRIVING)) + assert odr_ids(network, section) == [-3, -2, -1, 0, 1] + + # apply -> revert is byte-identical by contract. + stack.undo(network) + assert rm.write_xodr(network) == before + + +def test_insert_lane_leaves_the_new_lane_unlinked(network): + road = network.find_road("1") + stack = rm.edit.EditStack() + stack.push(network, rm.edit.split_lane_section(network, road, 60.0)) + head = network.road(road).sections[0] + tail = network.road(road).sections[1] + + stack.push(network, rm.edit.insert_lane(network, head, -1, rm.LaneType.DRIVING)) + + inserted = network.lane(lane_by_odr_id(network, head, -1)) + assert inserted.predecessor is None + assert inserted.successor is None + # The tail's predecessor followed the renumbering from -1 to -2. + assert network.lane(lane_by_odr_id(network, tail, -1)).predecessor == -2 + + +@pytest.mark.parametrize( + "at_odr_id", + [ + 0, # the center lane cannot be displaced + -5, # no lane there — appending past the outermost is add_lane + ], +) +def test_insert_lane_rejects_bad_positions(network, at_odr_id): + road = network.find_road("1") + section = network.road(road).sections[0] + stack = rm.edit.EditStack() + with pytest.raises(ValueError): + stack.push(network, rm.edit.insert_lane(network, section, at_odr_id, rm.LaneType.DRIVING)) From 61e3375cb0f50be858fbd9f054628a3d1f08ec4d Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Thu, 16 Jul 2026 01:26:40 -0700 Subject: [PATCH 3/4] docs(m2): fix the relative path to the P2 discovery report --- docs/design/m2/02_editing_tools.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/design/m2/02_editing_tools.md b/docs/design/m2/02_editing_tools.md index 75180e1f..5c5c9c3e 100644 --- a/docs/design/m2/02_editing_tools.md +++ b/docs/design/m2/02_editing_tools.md @@ -266,7 +266,7 @@ restore-in-place so undo stays valid). > command already built its junctions" (create/delete/split) from "this > command is topology **and** needs the junction regenerated" (a lane > appearing) — a distinction `topology` alone could not express. Detail: the -> [P2 discovery report](../../../roadmap/pillars/p2_discovery.md) §4. +> [P2 discovery report](../../roadmap/pillars/p2_discovery.md) §4. **Edge cases.** Ends too far apart (> configurable 50 m): factory error. Arms nearly parallel: connecting-road fit may loop — generator drops turn pairs whose From d0bfc8a2954551fddd04f9b4a1c0286b3178b5ba Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Thu, 16 Jul 2026 01:28:58 -0700 Subject: [PATCH 4/4] fix(core): rename insert_lane creator locals that shadowed the factory scope GCC -Werror=shadow (Linux only; the macOS clang preset does not flag it) rejected the creator's road/here locals shadowing the factory-scope ones. --- core/src/edit/operations.cpp | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/core/src/edit/operations.cpp b/core/src/edit/operations.cpp index 073ea805..1e364a0c 100644 --- a/core/src/edit/operations.cpp +++ b/core/src/edit/operations.cpp @@ -3372,22 +3372,22 @@ insert_lane(const RoadNetwork& network, LaneSectionId section_id, int at_odr_id, target.lane(lane_id)->widths = {Poly3{.a = 3.5}}; created.lanes.emplace_back(lane_id, Lane{}); // 3. Remap every link that named a shifted lane by id. - const Road& road = *target.road(road_id); - const auto here = std::ranges::find(road.sections, section_id); + const Road& owner = *target.road(road_id); + const auto pos = std::ranges::find(owner.sections, section_id); const auto remap_neighbor = [&](LaneSectionId neighbor_id, bool forward) { for (const LaneId neighbor_lane_id : target.lane_section(neighbor_id)->lanes) { - Lane& lane = *target.lane(neighbor_lane_id); - std::optional& link = forward ? lane.successor : lane.predecessor; + Lane& neighbor = *target.lane(neighbor_lane_id); + std::optional& link = forward ? neighbor.successor : neighbor.predecessor; if (link.has_value()) { *link = shift(*link); } } }; - if (here != road.sections.begin()) { - remap_neighbor(*std::prev(here), /*forward=*/true); + if (pos != owner.sections.begin()) { + remap_neighbor(*std::prev(pos), /*forward=*/true); } - if (here != road.sections.end() && std::next(here) != road.sections.end()) { - remap_neighbor(*std::next(here), /*forward=*/false); + if (pos != owner.sections.end() && std::next(pos) != owner.sections.end()) { + remap_neighbor(*std::next(pos), /*forward=*/false); } for (const JunctionId junction_id : touched_junctions) { for (JunctionConnection& connection : target.junction(junction_id)->connections) {