From 123d6c784647bcad567d8990a2cb42067dc0ac36 Mon Sep 17 00:00:00 2001 From: webdevred <148627186+webdevred@users.noreply.github.com> Date: Sat, 8 Aug 2026 18:44:50 +0200 Subject: [PATCH 1/3] Add failing test for y-sorting-threshold order inversion Reproduces the non-transitive comparator bug with real node spacing from a gen4-style body file: at y-sorting-threshold 0.1 the frontmost node in a group sorts to the back instead of the front. Asserts the invariant that should hold (no node ends up behind another node more than the threshold further forward), currently violated. Fixture lives in examples/regression_jbeam/, not examples/jbeam/, so it stays out of jbeam-edit-dump-ast's scan (which only reads examples/jbeam/ and would exitFailure on any file that fails to transform) and out of the curated example set the jbeam maintainer keeps. --- examples/regression_jbeam/README.md | 9 +++ .../regression_jbeam/y-sorting-repro.jbeam | 37 +++++++++++ test-extra/transformation/Spec.hs | 65 ++++++++++++++++++- 3 files changed, 110 insertions(+), 1 deletion(-) create mode 100644 examples/regression_jbeam/README.md create mode 100644 examples/regression_jbeam/y-sorting-repro.jbeam diff --git a/examples/regression_jbeam/README.md b/examples/regression_jbeam/README.md new file mode 100644 index 00000000..ed93e21f --- /dev/null +++ b/examples/regression_jbeam/README.md @@ -0,0 +1,9 @@ +# Regression jbeam fixtures + +Small `.jbeam` files that exist purely to reproduce a specific bug for a +regression test. Unlike `examples/jbeam/`, these are **not** written or +vetted by the jbeam maintainer, not curated demo material, and +not picked up by `jbeam-edit-dump-ast` (which only scans `examples/jbeam/`). +Don't treat them as examples of good jbeam, and don't add to this +directory unless a test genuinely needs a fixture that can't be built +from what's already in the project. diff --git a/examples/regression_jbeam/y-sorting-repro.jbeam b/examples/regression_jbeam/y-sorting-repro.jbeam new file mode 100644 index 00000000..2e3f2f13 --- /dev/null +++ b/examples/regression_jbeam/y-sorting-repro.jbeam @@ -0,0 +1,37 @@ +{ +"testpart":{ + "nodes":[ + ["id", "posX", "posY", "posZ"], + // Synthetic regression-test fixture for issue #214, not vetted by + // the jbeam maintainer and not intended as a demo/example. + // Real node positions from a gen4-style body file, reduced to just + // the left side, kept because this spacing reproduces a real bug. + ["nl0", 0.953, -1.967, 0.122], + ["nl2", 0.92, -1.953, 0.439], + ["nl4", 0.78, -1.815, 0.719], + ["nl6", 1.036, -1.807, 0.125], + ["nl8", 0.998, -1.791, 0.473], + ["nl10", 0.944, -1.644, 0.72], + ["nl12", 0.823, -1.362, 0.841], + ["nl14", 0.952, -1.354, 0.772], + ["nl18", 0.971, -1.107, 0.628], + ["nl20", 0.841, -1.079, 0.885], + ["nl22", 0.964, -1.058, 0.106], + ["nl24", 0.975, -1.054, 0.424], + ["nl26", 0.038, -0.997, 0.943], + ["nl27", 0.616, -0.958, 0.933], + ["nl29", 0.808, -0.898, 0.923], + ["nl31", 0.038, -0.838, 0.949], + ["nl32", 0.593, -0.78, 0.937], + ["nl34", 0.835, -0.688, 0.91], + ["nl36", 0.975, -0.645, 0.631], + ["nl38", 0.972, -0.644, 0.106], + ], + "beams":[ + ["id1:", "id2:"], + ], + "triangles":[ + ["id1:", "id2:", "id3:"], + ], +}, +} diff --git a/test-extra/transformation/Spec.hs b/test-extra/transformation/Spec.hs index c3463f55..1964de99 100644 --- a/test-extra/transformation/Spec.hs +++ b/test-extra/transformation/Spec.hs @@ -2,11 +2,16 @@ module Spec ( main, ) where +import Data.Char (isDigit) import Data.List (isPrefixOf, isSuffixOf) import Data.Map qualified as M import Data.Set qualified as S +import Data.Text (Text) import Data.Text qualified as T -import JbeamEdit.Core.Node (Node) +import Data.Vector qualified as V +import GHC.IsList (fromList) +import JbeamEdit.Core.Node (Node (..), NumberValue (..), expectArray) +import JbeamEdit.Core.NodePath qualified as NP import JbeamEdit.Formatting import JbeamEdit.IOUtils (tryReadFile) import JbeamEdit.Parsing.Jbeam (parseNodes) @@ -85,6 +90,63 @@ beamValidationSpec = do it "has no duplicate beams" $ findDuplicateBeams internalBeams `shouldBe` [] +nodesQuery :: NP.NodePath +nodesQuery = fromList [NP.ObjectIndex 0, NP.ObjectKey "nodes"] + +{- | (name, Y position) for every vertex in a transformed top node's +"nodes" section, in file order (i.e. the order `transform` actually +wrote them out in). Read straight back out of the output rather than +correlated against the input names, since `transform` renames vertices. +-} +vertexPositionsInOrder :: Node -> [(Text, Double)] +vertexPositionsInOrder topNode = + case NP.queryNodes nodesQuery topNode >>= NP.expectArray nodesQuery of + Left _ -> [] + Right rows -> + [ (name, realToFrac (nvValue yNum)) + | row <- V.toList rows + , Just inner <- [expectArray row] + , Just (String name) <- [inner V.!? 0] + , name /= "id" + , Just (Number yNum) <- [inner V.!? 2] + ] + +{- | Real left-side structural node positions from a NASCAR gen4-style body +file (see issue #214). This specific spacing reproduces a real transform +run: with y-sorting-threshold 0.1, the frontmost node (nl0, Y=-1.967) ends +up sorted to the back of its group instead of the front. +-} +ySortingReproFixture :: FilePath +ySortingReproFixture = "examples/regression_jbeam/y-sorting-repro.jbeam" + +ySortingBandingSpec :: Spec +ySortingBandingSpec = + describe "y-sorting-threshold" + . it + "never places a node behind another node that is more than the threshold further forward" + $ do + let thr = 0.1 :: Double + cfg = newTransformationConfig {ySortingThreshold = 0.1} + topNode <- parseJbeamFile ySortingReproFixture + case transform M.empty cfg topNode of + Left err -> expectationFailure ("transform failed: " ++ T.unpack err) + Right (_, _, _, resultNode) -> do + let positions = zip (vertexPositionsInOrder resultNode) [0 :: Int ..] + groupPrefix = T.dropWhileEnd isDigit + -- Only compare nodes within the same output group (e.g. + -- "nll", "nlm"): a Left-tree node and a Middle-tree node + -- are unrelated blocks in the file, not a single ordered + -- sequence, so their relative position isn't meaningful. + outOfOrder = + [ (n1, n2) + | ((n1, y1), i1) <- positions + , ((n2, y2), i2) <- positions + , i1 < i2 + , groupPrefix n1 == groupPrefix n2 + , y1 - y2 > thr + ] + outOfOrder `shouldBe` [] + main :: IO () main = hspec $ do let exampleConfigPath = unsafeEncodeUtf "examples/jbeam-edit.yaml" @@ -102,3 +164,4 @@ main = hspec $ do mapM_ (testInputFile "cfg-default" newTransformationConfig) inputFiles mapM_ (testInputFile "cfg-example" tfConfig) inputFiles beamValidationSpec + ySortingBandingSpec From 185ca3fdd84ef31037b96c3456f5671aee8b9519 Mon Sep 17 00:00:00 2001 From: webdevred <148627186+webdevred@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:12:04 +0200 Subject: [PATCH 2/3] Sort by a precomputed Y band instead of a fuzzy comparison Treating two vertices as equal when their Y distance is under the threshold is not transitive, so sortBy had no defined result for a chain of vertices with small consecutive gaps and a large total spread. Walk the group in Y order instead, assigning an integer band that changes when a vertex sits more than the threshold from the band's start. Comparing integers cannot contradict itself, and the band is never wider than the configured value. supportThreshold becomes Scientific to match the other config fields. --- .../JbeamEdit/Transformation.hs | 49 ++++++++++++------- .../JbeamEdit/Transformation/Config.hs | 6 +-- test-extra/transformation/Spec.hs | 2 +- 3 files changed, 34 insertions(+), 23 deletions(-) diff --git a/src-extra/transformation/JbeamEdit/Transformation.hs b/src-extra/transformation/JbeamEdit/Transformation.hs index 34681f0e..67cb47b4 100644 --- a/src-extra/transformation/JbeamEdit/Transformation.hs +++ b/src-extra/transformation/JbeamEdit/Transformation.hs @@ -179,7 +179,6 @@ moveSupportVertices newNames tfCfg connMap vsPerType = ] brks = xGroupBreakpoints tfCfg - thr = ySortingThreshold tfCfg assignSupportNames = assignNames newNames brks SupportTree @@ -194,12 +193,12 @@ moveSupportVertices newNames tfCfg connMap vsPerType = ( SupportKey , VertexTree [sideComment SupportTree] - ( snd - . mapAccumL - assignSupportNames - M.empty - . NE.sortBy (compareAV thr SupportTree) - $ NE.map (uncurry updateSupportVertexName) vs + ( let renamedVertices = NE.map (uncurry updateSupportVertexName) vs + sorted = NE.sortBy (on compare $ vY . aVertex) renamedVertices + (_, bandIndices) = mapAccumL (indexBand tfCfg) (0, 0) sorted + bandSortedVertices = NE.map snd $ NE.sortBy (compareAV SupportTree) bandIndices + (_, renamedVertices') = mapAccumL assignSupportNames M.empty bandSortedVertices + in renamedVertices' ) ) ) @@ -331,19 +330,15 @@ treesOrder :: [VertexTreeType] treesOrder = [LeftTree, MiddleTree, RightTree, SupportTree] compareAV - :: Scientific -> VertexTreeType -> AnnotatedVertex -> AnnotatedVertex -> Ordering -compareAV thr treeType vertex1 vertex2 = + :: VertexTreeType -> (Int, AnnotatedVertex) -> (Int, AnnotatedVertex) -> Ordering +compareAV treeType (band1, vertex1) (band2, vertex2) = let supportNameCompare = bool EQ (on compare (dropIndex . vName . aVertex) vertex1 vertex2) (treeType == SupportTree) - y1 = vY . aVertex $ vertex1 - y2 = vY . aVertex $ vertex2 compareZ = comparing (vZ . aVertex) vertex1 vertex2 - compareY = - let yDiff = abs $ y1 - y2 - in bool EQ (compare y1 y2) (yDiff > thr) + compareY = compare band1 band2 compareX = on compare (vX . aVertex) vertex1 vertex2 in mconcat [ supportNameCompare @@ -397,6 +392,21 @@ assignNames newNames brks treeType prefixMap av = prefixMap' = M.insert cleanPrefix (lastIdx + 1) prefixMap in (prefixMap', av {aVertex = newVertex}) +indexBand + :: TransformationConfig + -> (Int, Scientific) + -> AnnotatedVertex + -> ((Int, Scientific), (Int, AnnotatedVertex)) +indexBand tfCfg (bandIndex, bandY) av = + let nodeY = vY (aVertex av) + distance = abs (nodeY - bandY) + thr = ySortingThreshold tfCfg + in if distance >= thr + then + ((bandIndex + 1, nodeY), (bandIndex + 1, av)) + else + ((bandIndex, bandY), (bandIndex, av)) + sortVertices :: VertexTreeType -> UpdateNamesMap @@ -404,11 +414,12 @@ sortVertices -> VertexTree -> VertexTree sortVertices treeType newNames tfCfg (VertexTree comments vertices) = - let thr = ySortingThreshold tfCfg - brks = xGroupBreakpoints tfCfg - sortedGroups = NE.sortBy (compareAV thr treeType) vertices - - renamedGroups = snd $ mapAccumL (assignNames newNames brks treeType) M.empty sortedGroups + let brks = xGroupBreakpoints tfCfg + sorted = NE.sortBy (on compare $ vY . aVertex) vertices + (_, bandIndices) = mapAccumL (indexBand tfCfg) (0, 0) sorted + bandSortedAnnotated = NE.map snd $ NE.sortBy (compareAV treeType) bandIndices + renamedGroups = + snd $ mapAccumL (assignNames newNames brks treeType) M.empty bandSortedAnnotated in VertexTree comments renamedGroups updateVerticesInNode diff --git a/src-extra/transformation/JbeamEdit/Transformation/Config.hs b/src-extra/transformation/JbeamEdit/Transformation/Config.hs index 1070f9b7..6579da3e 100644 --- a/src-extra/transformation/JbeamEdit/Transformation/Config.hs +++ b/src-extra/transformation/JbeamEdit/Transformation/Config.hs @@ -49,7 +49,7 @@ import Text.Read defaultSortingThreshold :: Scientific defaultSortingThreshold = 0.05 -defaultSupportThreshold :: Double +defaultSupportThreshold :: Scientific defaultSupportThreshold = 96 defaultMaxSupportCoordinates :: Natural @@ -66,7 +66,7 @@ defaultBreakpoints = data TransformationConfig = TransformationConfig { ySortingThreshold :: Scientific , xGroupBreakpoints :: XGroupBreakpoints - , supportThreshold :: Double + , supportThreshold :: Scientific , maxSupportCoordinates :: Natural } deriving (Generic) @@ -125,7 +125,7 @@ instance FromJSON XGroupBreakpoints where ) pure $ XGroupBreakpoints lst -parseSupportThreshold :: Object -> Parser Double +parseSupportThreshold :: Object -> Parser Scientific parseSupportThreshold o = do thr <- o .: "support-threshold" when (thr < 1) failWithMessage $> thr diff --git a/test-extra/transformation/Spec.hs b/test-extra/transformation/Spec.hs index 1964de99..754bfe5f 100644 --- a/test-extra/transformation/Spec.hs +++ b/test-extra/transformation/Spec.hs @@ -138,7 +138,7 @@ ySortingBandingSpec = -- are unrelated blocks in the file, not a single ordered -- sequence, so their relative position isn't meaningful. outOfOrder = - [ (n1, n2) + [ (y1, y2) | ((n1, y1), i1) <- positions , ((n2, y2), i2) <- positions , i1 < i2 From 26d6bc23f6357651718d94b8c200130be648db28 Mon Sep 17 00:00:00 2001 From: webdevred <148627186+webdevred@users.noreply.github.com> Date: Tue, 11 Aug 2026 21:19:05 +0200 Subject: [PATCH 3/3] Seed the first band on the first vertex instead of on zero The accumulator started at Y=0, which is a point in the middle of the vehicle rather than a vertex. Every vertex within the threshold of the centreline therefore shared one band, which could be twice as wide as configured. Two vertices 0.08 apart across zero came out ordered by height instead of by Y at threshold 0.05. --- src-extra/transformation/JbeamEdit/Transformation.hs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src-extra/transformation/JbeamEdit/Transformation.hs b/src-extra/transformation/JbeamEdit/Transformation.hs index 67cb47b4..873ea0e3 100644 --- a/src-extra/transformation/JbeamEdit/Transformation.hs +++ b/src-extra/transformation/JbeamEdit/Transformation.hs @@ -195,7 +195,7 @@ moveSupportVertices newNames tfCfg connMap vsPerType = [sideComment SupportTree] ( let renamedVertices = NE.map (uncurry updateSupportVertexName) vs sorted = NE.sortBy (on compare $ vY . aVertex) renamedVertices - (_, bandIndices) = mapAccumL (indexBand tfCfg) (0, 0) sorted + (_, bandIndices) = mapAccumL (indexBand tfCfg) (0, firstY sorted) sorted bandSortedVertices = NE.map snd $ NE.sortBy (compareAV SupportTree) bandIndices (_, renamedVertices') = mapAccumL assignSupportNames M.empty bandSortedVertices in renamedVertices' @@ -392,6 +392,10 @@ assignNames newNames brks treeType prefixMap av = prefixMap' = M.insert cleanPrefix (lastIdx + 1) prefixMap in (prefixMap', av {aVertex = newVertex}) +-- | Y of the first vertex, to seed the first band on a vertex rather than on 0. +firstY :: NonEmpty AnnotatedVertex -> Scientific +firstY = vY . aVertex . NE.head + indexBand :: TransformationConfig -> (Int, Scientific) @@ -416,7 +420,7 @@ sortVertices sortVertices treeType newNames tfCfg (VertexTree comments vertices) = let brks = xGroupBreakpoints tfCfg sorted = NE.sortBy (on compare $ vY . aVertex) vertices - (_, bandIndices) = mapAccumL (indexBand tfCfg) (0, 0) sorted + (_, bandIndices) = mapAccumL (indexBand tfCfg) (0, firstY sorted) sorted bandSortedAnnotated = NE.map snd $ NE.sortBy (compareAV treeType) bandIndices renamedGroups = snd $ mapAccumL (assignNames newNames brks treeType) M.empty bandSortedAnnotated