p2-s2: junction regeneration learns turn-set changes

[JArmandoAnaya]

Part of the Road to Parity roadmap. Epic #251. Ground truth: p2_discovery.md §4.

Highest-risk sprint in the pillar. Gates p2-s6 (Lane Carve).

The problem

regenerate_junction refuses any turn-set change, twice:

• "regeneration changed the connection count; delete and recreate the junction"
• "regeneration changed the turn set; delete and recreate the junction"

and the editor swallows the failure into a warning toast rather than failing the user's edit (document.cpp). So today: add a lane to a road feeding a junction → the junction does not update and stays stale until the user deletes and recreates it. GW-2 step 12 carves a turn lane "approaching the junction" — exactly the refused case.

The restriction is not incidental: regen's ability to preserve connecting-road ids (and therefore keep undo exact) depends on a fixed connection count.

Decision (maintainer, 2026-07-15): teach regen turn-set changes. A turn lane that cannot reach a junction is a hollow feature.

Scope

• Regen allocates ids for new turns, erase_exacts removed ones, and rewrites the connection table, keeping undo exact. Turn matching is already by key rather than by order — that half is done.

• GenericCommand's creator/erased/before/after sets already support this shape; no new command machinery.

• Interior lane insert with renumbering + link remap (moved here from p2-s1: kernel lane-section foundations (split, width profiles, interior lanes) #262, maintainer-approved 2026-07-15). add_lane is outermost-only today; Lane Form and Lane Carve need an interior insert, which means renumbering lanes outward and remapping every link that named them — the neighbouring sections' predecessor/successor links and JunctionConnection::lane_links.

  It moved here because that remap is the same problem this sprint already owns: junction lane_links reference only a road's end sections, and inserting a lane in an end section changes the turn set — the case regen refuses today. Implementing it in p2-s1: kernel lane-section foundations (split, width profiles, interior lanes) #262 meant writing an unexercisable end-section path against a contract this sprint redefines. remove_lane already clears those links rather than remapping; generalize that here, once.

  The new lane must NOT be linked back to the neighbouring sections: a lane that appears is not connected to the original (asam.net:xodr:1.4.0:road.lane.link.new_lane_appear).

Refactorings

• regenerate_junction becomes a topology-changing command. This collides with the editor's dirty-set contract: document.cpp deliberately breaks out of the regen loop when dirty.topology is set, so create/delete/split don't self-regenerate. The contract needs a third state (topology-changed-and-regenerate).
• Resolve that contract on paper in this issue before writing code.

Acceptance

• Add a lane to a junction arm → the junction regenerates; no warning toast. This case has no test today because the answer was "refuse".
• No-op regen stays byte-identical — RegenerateJunctionIsByteEqualWhenNothingChanged must stay green.
• verify_junction_welds reports no breach after a turn-set change.
• Undo restores the exact connection table and connecting-road ids.
• Soak driver exercises lane edits on junction arms; ASan clean.

Out of scope

Junction interior geometry changes; corner radius and per-corner materials (P4).

Note

If this sprint overruns, stop and report. p2-s6 depends on it, and the fallback (carve refuses near junction arms) is a GW-2 amendment requiring maintainer approval — not a silent deviation.

Supersedes

(none)

[JArmandoAnaya]

Design note: resolving the dirty-set contract (required before code)

Read against the merged p2-s1 baseline (main 728a9f6). Three findings change the shape of this sprint.

Finding 1 — DirtySet::topology conflates two unrelated meanings

document.cpp:177-181:

for (const JunctionId junction_id : dirty.junctions) {
  if (dirty.topology || already_regenerated) { break; }

topology is documented as "Roads or junctions were added or removed (drives tree-model resets)" — a view concern. But the regen loop reuses it as "this command already handled its own junctions, don't regen them" — a correctness concern. Those coincided only because, until now, every topology command was also a junction-building command.

Lane Add / Lane Carve break the coincidence: they are genuinely topology (a lane appears → the scene tree must reset) and they need regen. Under the current contract that combination is unexpressible.

Proposal: leave topology as the view flag it is documented to be, and add an explicit correctness flag:

/// This command already brought DirtySet::junctions up to date; the editor
/// must not regenerate them again. Set by commands that build junction
/// structure themselves (create/delete junction, split_road, attach_t, the
/// assemblies) — a second regeneration would double-work or fight them.
bool junctions_are_current = false;

The loop becomes if (dirty.junctions_are_current || already_regenerated) break;. Default false is the safe direction: a new command that forgets the flag gets a redundant regen (slow, correct) rather than a stale junction (fast, wrong).

Sites to set it true — the current topology = true users that build junctions: create_junction (:2358), delete_junction (:2693), split_road (:1551), merge_roads (:1917), attach_t_junction (:2253), close_gap (:2392), delete_road (:1157), and the composites (:2504, :2814, :2891, :2985). Each needs a one-line justification in review — this is the sprint's real review surface, not the regen algorithm.

Finding 2 — add_lane never declares a dirty junction, so regen could never have run

operations.cpp:3021:

DirtySet{.roads = {section->road}, .topology = true}   // <- no .junctions

The loop iterates dirty.junctions. It is empty. So even deleting the topology skip entirely would change nothing for add_lane — the warning toast users see today comes from other commands' regen attempts, not from adding a lane. The lane ops must populate dirty.junctions via junctions_touching(network, road), which remove_lane (:3079) already does (for the lane_links it clears) and add_lane does not.

So this sprint is two independent fixes, and the flag alone is not enough:

1. the lane ops declare the junctions they touch, and
2. the editor stops skipping regen for them.

Finding 3 — lifting the restriction means restructuring regen around a creator

regenerate_junction builds command->after declaratively (:2607-2627). But GenericCommand overwrites after whenever creator is set — after = read_values(network, before) runs after the creator (:~200). Creating connecting roads for new turns therefore forces every mutation into the creator; a half-declarative/half-creator command silently drops the declarative half.

That is a real rewrite of the most invariant-critical function in the file, but it does not need new machinery:

• create_junction (:2358) already delegates to materialize_junction(target, created, ends, plan) — the new-turn path reuses it rather than reinventing connecting-road creation.
• Turn matching is already by key, not order (:2583-2601) — the hard half is done. Unmatched planned turns → create; unmatched existing connections → erased + drop from the table.
• erased is computed at factory time and applied alongside a creator, so freeing removed connecting roads needs no new hook.

Sequencing

1. junctions_are_current + audit every topology = true site (mechanical, reviewable, no behaviour change on its own).
2. Lane ops declare junctions_touching.
3. Restructure regenerate_junction around a creator; lift both restrictions (:2513, :2604).
4. Interior insert_lane + renumber + link remap (moved here from p2-s1: kernel lane-section foundations (split, width profiles, interior lanes) #262) — lands last, on top of a regen that can absorb its turn-set change. Junction lane_links remap is the same code path this sprint already opens.

Test plan

• RegenerateJunctionIsByteEqualWhenNothingChanged (test_edit_operations.cpp:1101) must stay green — it is the guard that the restructure is behaviour-preserving for the no-op case.
• New: add a lane to an arm → junction regenerates, connection count grows, no toast. No test exists today because the answer was "refuse".
• New: remove a lane from an arm → connection count shrinks, freed ids erased.
• verify_junction_welds no breach after a turn-set change; undo restores the exact connection table and connecting-road ids.
• Soak: lane edits on junction arms (p2-s1 already added split_lane_section + lane_width_profile ops; this adds the arm case).

Open question for review: whether junctions_are_current should instead be inferred (e.g. "the command created or erased a junction"). I favour the explicit flag — inference would re-couple the view and correctness concerns that Finding 1 is untangling.

[JArmandoAnaya]

Implemented — PR #270

The design note's three findings held up; the implementation resolved the open question and turned up one blocker.

junctions_are_current, the explicit flag (open question in the design note): went with the explicit flag over inferring it. Default false fails in the safe direction — a command that forgets it gets a redundant regeneration (slow, correct), never a stale junction (fast, wrong). Set on create_junction / delete_junction / split_road / delete_road; the editor's regen loop keys off it instead of topology.

The restructure followed the note: partition matched / new / dropped by the existing TurnKey, keep the ids of survivors, materialize_connection (factored out of materialize_junction) for new turns, erase dropped ones with everything they own. The creator does all matched writes before any creation so no arena reference is held across a reallocation.

Blocker found and scoped out — the preview slot leak (#271). A preview command is reverted and destroyed every frame, and Arena::erase_exact reserves a slot rather than recycling it, so a per-frame regeneration that created connecting roads would leak arena slots for the rest of the session. This sprint lifts the refusal on the push/commit path only (which is all its acceptance needs — add_lane goes through push_command, not a preview) and keeps the per-frame drag path in-place-only via a new TurnSetPolicy parameter. The general fix is #271, which blocks #218.

Interior insert_lane (moved here from #262) landed as designed: renumber the outer block, remap adjacent-section links and junction lane_links, leave the new lane unlinked.

One item handed to #216 (noted there): set_lane_width / set_lane_width_profile still declare no dirty junctions, so a width edit on an arm goes stale. Geometry-only, no turn-set machinery needed — belongs with the Lane Width tool.

Verification: 864 ctest + 99 pytest green, ASan+UBSan clean, soak PASS on 3 seeds; the grow/shrink tests were mutation-checked to confirm they bite.