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/src-extra/transformation/JbeamEdit/Transformation.hs b/src-extra/transformation/JbeamEdit/Transformation.hs index 34681f0e..873ea0e3 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, firstY sorted) 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,25 @@ 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) + -> 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 +418,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, firstY sorted) 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 c3463f55..754bfe5f 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 = + [ (y1, y2) + | ((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