Make support-vertex renaming a fixed point and fix the #189 output regressions - #218
Draft
webdevred wants to merge 7 commits into
Draft
Make support-vertex renaming a fixed point and fix the #189 output regressions#218webdevred wants to merge 7 commits into
webdevred wants to merge 7 commits into
Conversation
Support vertices can be classified two ways: moveSupportVertices pulls
out highly-connected nodes via the threshold, and addVertexTreeToForest
separately classifies letter-ending groups as SupportTree directly.
Both used to key their OMap1 entry differently (SupportKey vs a
prefix-derived key), so they never collided and ended up as two
separate "Support nodes" sections with duplicated/misplaced metadata
instead of one merged section.
Also fixes two spots where a collision in an OMap1 merge (in
mergeOMap1Trees and the near-identical insertTreeInMap) discarded the
incoming side's comments unconditionally, even when the existing side
had none to fall back on.
Regenerated examples/transformed_jbeam/fender-cfg-{default,example}.jbeam
via jbeam-edit-dump-ast; frame and suspension are unaffected.
--transform was not a fixed point. updateSupportVertexName appended a side letter to the name before assignNames ran, and assignNames was written to recognise its own output one character further along. Running the tool twice therefore grew the name: nlsl1 became nlslsl. Drop the pre-mutation and let assignNames derive the whole name from the source name and the x coordinate. A support prefix is now the name with its trailing digits, side letter and s removed, followed by s and the side letter for the group the coordinate falls in. That is a fixed point, so a second pass lands on the same name. Frame's rear support node changes from rl_rsm to rl_sm. The side letter comes from the coordinate rather than from the name, and _r is a side letter as far as the transformation can tell, so it is dropped. The ordinary nodes in that group already lost it: rl_r35 and rl_r38 have been coming out as rl_m0 and rl_m1 all along. The two now agree. The test transforms a fixture with three support hubs sharing one prefix group twice and asserts the second pass renames nothing further. The fixture lives in examples/regression_jbeam/ so it stays out of jbeam-edit-dump-ast's scan and out of the curated example set.
They were renamed to prove that letter-ending names survive a transform, but only the two node rows were changed. Every beam and triangle kept referring to bfl0 and bfr0, so the example has been carrying two dangling references ever since. The missing digit suffix also broke prefix-key lookup, which is what silently dropped the Left side and Right side comments from the output. The property they were there to prove now has its own fixture in examples/regression_jbeam/, with a test that asserts every vertex coordinate survives a transform. That states it directly instead of leaving it implicit in a node count, and it keeps scaffolding out of an example the jbeam maintainer edits. Fender's output goes back to the pre-regression structure: side comment, side metadata, side coordinates, then one support section.
compareAV ordered support vertices by their current name prefix, and the current name is what the transformation is about to replace. The first pass sorted on the source prefixes (rl, rl_f, rl_r), the second on the names the first pass produced, so the section came out in a different order each time. Sort on the prefix the vertex is about to be given instead. It is a pure function of the name and the x coordinate, so it can be computed before the rename, and support prefixes are a fixed point of it, so both passes agree. Frame's support section settles after one pass. Suspension still moves, for an unrelated reason: all three of its support nodes collapse to the same prefix, so the tie falls through to the metadata, and metadata is not carried across vertex tree boundaries.
breakVertices builds its accumulator back to front and reverses it before handing the finished tree back. The nodes it hands to the next tree, the metadata and comments sitting directly in front of the vertex that ended the current one, were never reversed, so they arrived backwards. With two comments in front of a group that means they swap places on every run, which is why frame's // Middle side and // prefix group rl_m kept trading positions and never settled. The first tree in a section is unaffected, it gets its comments from newVertexTree rather than from this hand-off, which is why only the second and later groups moved.
The existing idempotency spec runs one small regression fixture and compares names and Y positions only, so comments, metadata and beam references were never checked. That is how the comment reordering and the metadata shift stayed invisible. Parse each committed file in examples/transformed_jbeam/, transform it again and compare the whole formatted text. The committed file is already the output of one transform, so it is the input the second pass would see. Suspension is pending, because metadata is not carried across vertex tree boundaries. That is tracked separately.
webdevred
force-pushed
the
fix-support-vertex-rename-idempotency
branch
from
August 9, 2026 16:23
fbf9621 to
2e5ea11
Compare
A Windows checkout hands the parser CRLF, and commentStripSpace keeps the newline after a block comment opener by matching on "\n", so the comment collapsed and the texts differed. The spec is not asserting anything about line endings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
--transformwas not a fixed point.updateSupportVertexNameappended a side letter to a support name beforeassignNamesran, andassignNameswas written to recognise its own output one character further along, so running the tool twice grew the name andnlsl1becamenlslsl. The pre-mutation is gone andassignNamesnow derives the whole name from the source name and the x coordinate, which makes a support prefix a fixed point of its own rule.Two more things stopped it from settling once the names did.
compareAVordered support vertices by the name they currently have, which is the thing being replaced, so the first and second pass sorted on different keys. It now sorts on the prefix the vertex is about to be given. AndbreakVerticeshanded the comments and metadata in front of the next group back in reverse, because it reverses the finished tree but not the leftovers, which is why frame's// Middle sideand// prefix group rl_mkept swapping places on every run.Separately, #189 introduced three regressions in the example output that nobody caught because the fixtures were regenerated against the new behaviour. Support vertices reached the forest by two routes and keyed their entries differently, so they never collided and came out as two "Support nodes" sections with metadata stranded between them. Two OMap1 merges also dropped the incoming side's comments unconditionally, even when the existing side had none.
Frame's rear support node changes from
rl_rsmtorl_sm. The side letter comes from the coordinate rather than from the name, and_ris a side letter as far as the transformation can tell. The ordinary nodes in that group already lost it,rl_r35andrl_r38have been coming out asrl_m0andrl_m1all along, so the two now agree rather than contradicting each other.Outside the obvious scope:
bfl_fandbfr_finexamples/jbeam/fender.jbeamare renamed back tobfl0andbfr0. They were renamed to prove that letter-ending names survive a transform, but only the two node rows were changed, so every beam and triangle has been pointing at names that no longer existed. The property they were there to prove now has its own fixture underexamples/regression_jbeam/.Deliberately left alone: support classification still falls back to the node name, so a name ending in a letter becomes a support vertex regardless of how many beams reach it. That is wrong in principle, but it is not damage this branch caused, and it cannot be replaced until the connectivity threshold works. Measured across 3073 vehicle files,
round(0.96 * n)exceeds the maximum possible degreen - 1for every group under 25 nodes, which covers 97% of files, so dropping the name shortcut today would swap a wrong classification for almost no classification at all. Both halves have their own issue. #219 #220Still draft, for two reasons. The example output wants a read from someone who knows jbeam, particularly the fender, which regains its side comments and its right-side group metadata. And suspension is not a fixed point yet, which is pending in the new spec: its three support nodes collapse to the same prefix, so the tie falls through to the metadata, and metadata is not carried across vertex tree boundaries. That is an older bug with its own issue and is not something this branch caused.