Fix vertex loss in transformation for all-letter-ending node groups#189
Draft
Fix vertex loss in transformation for all-letter-ending node groups#189
Conversation
Two bugs caused vertices to be silently dropped when most node names end in letters (like burnside_frame where all structural nodes have suffixes r, l, rr, ll). Bug 1: insertTreeInMap in VertexExtraction used OMap1.snoc which replaces the first element when the key matches. For letter-ending nodes, every group gets SupportKey, so only the last group was kept. Fixed: check if key exists and merge vertices with (<>) on NonEmpty. Bug 2: addVertexTreeToForest used M.insert which replaced the supportForest entry when processing SupportTree vertices. Vertices moved to supportForest by moveSupportVertices (typically digit-ending nodes classified as support via high connectivity) were lost when the remaining SupportTree vertices were processed by addVertexTreeToForest. Fixed: use M.insertWith mergeOMap1Trees to merge OMap1 entries.
The threshold controls when Y-coordinate differences (front/back) are significant enough to sort by, falling back to Z (height) within a band. The old name was a holdover from when the code sorted on Z instead of Y. Breaking change: existing .jbeam-edit.yaml configs using z-sorting-threshold must rename the key to y-sorting-threshold. Also renames the Haskell field zSortingThreshold to ySortingThreshold and the local variable zDiff to yDiff in compareAV.
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
1 participant
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.
Summary
insertTreeInMap(VertexExtraction) replaced OMap1 entries viaOMap1.snocwhen the key matched, silently dropping all but the last vertex group for vehicles where most node names end in letters (e.g. burnside_frame where nodes have suffixes r, l, rr, ll, all classified asSupportKey).addVertexTreeToForestusedM.insertwhich replaced thesupportForestentry when processing remaining SupportTree vertices, dropping any vertices that had been moved there bymoveSupportVertices.Both issues are now fixed:
insertTreeInMapchecks if the key exists and merges with(<>)onNonEmptyinstead of replacing.addVertexTreeToForestusesM.insertWith mergeOMap1Treesto append OMap1 entries rather than overwrite.Tested against burnside_frame (all 76 nodes now present, was 21 before) and bolide_chassis (stale beam references resolved as a consequence). All existing tests pass.
Missing test fixtures
@gittarrgy01 The transformation test suite has no coverage for vehicles where most structural nodes end in letters (r, l, rr, ll), these take a different code path through
insertTreeInMapandaddVertexTreeToForest. The bugs fixed here had no test coverage and were only caught by running against real BeamNG files.Could you add a JBeam example file in
examples/jbeam/that exercises this pattern? The test infrastructure will pick it up from there automatically.