diff --git a/examples/regression_jbeam/y-sorting-repro.jbeam b/examples/regression_jbeam/y-sorting-repro.jbeam index 2e3f2f13..87e9ec38 100644 --- a/examples/regression_jbeam/y-sorting-repro.jbeam +++ b/examples/regression_jbeam/y-sorting-repro.jbeam @@ -2,10 +2,16 @@ "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. + // Synthetic regression-test fixture, 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. + // the left side, kept because this spacing reproduces two real bugs. + // Issue #214: at y-sorting-threshold 0.1 the frontmost node sorts to + // the back of its group. + // X columns: the five frontmost nodes sit in two vertical columns + // that Y and Z cannot separate, so a band sorted by Z alone climbs + // one column, jumps to the other and comes back. + // Change these positions and both tests are measuring something else. ["nl0", 0.953, -1.967, 0.122], ["nl2", 0.92, -1.953, 0.439], ["nl4", 0.78, -1.815, 0.719], diff --git a/src-extra/transformation/JbeamEdit/Transformation/Config.hs b/src-extra/transformation/JbeamEdit/Transformation/Config.hs index 6579da3e..f58141eb 100644 --- a/src-extra/transformation/JbeamEdit/Transformation/Config.hs +++ b/src-extra/transformation/JbeamEdit/Transformation/Config.hs @@ -1,4 +1,5 @@ {-# LANGUAGE DeriveGeneric #-} +{-# LANGUAGE MultiWayIf #-} module JbeamEdit.Transformation.Config ( loadTransformationConfig, @@ -18,6 +19,7 @@ import Control.Monad (forM, when) import Data.Bifunctor (first) import Data.ByteString.Lazy qualified as LBS import Data.Functor (($>)) +import Data.Maybe (isJust, isNothing) import Data.Scientific (Scientific) import Data.Text (Text) import Data.Text qualified as T @@ -46,6 +48,9 @@ import Numeric.Natural (Natural) import System.OsPath import Text.Read +defaultXSortingThreshold :: Maybe Scientific +defaultXSortingThreshold = Nothing + defaultSortingThreshold :: Scientific defaultSortingThreshold = 0.05 @@ -65,6 +70,7 @@ defaultBreakpoints = data TransformationConfig = TransformationConfig { ySortingThreshold :: Scientific + , xSortingThreshold :: Maybe Scientific , xGroupBreakpoints :: XGroupBreakpoints , supportThreshold :: Scientific , maxSupportCoordinates :: Natural @@ -75,6 +81,7 @@ newTransformationConfig :: TransformationConfig newTransformationConfig = TransformationConfig defaultSortingThreshold + defaultXSortingThreshold defaultBreakpoints defaultSupportThreshold defaultMaxSupportCoordinates @@ -134,10 +141,25 @@ parseSupportThreshold o = do fail "'support-threshold' must be a percentage value of 1 or higher (e.g., 80 or 80.8). Values below 1 (e.g., 0.80) are not allowed." +parseXSortingThreshold :: Object -> Parser (Maybe Scientific) +parseXSortingThreshold o = do + thr <- o .:? "x-sorting-threshold" + let cleanThr = T.unpack . T.strip <$> thr + maybeValid = cleanThr >>= readMaybe + in if + | thr == Just "off" || isNothing thr -> pure defaultXSortingThreshold + | isJust maybeValid -> pure maybeValid + | True -> failWithMessage + where + failWithMessage = + fail + "TODO: proper error message" + instance FromJSON TransformationConfig where parseJSON = withObject "TransformationConfig" $ \o -> TransformationConfig <$> o .:? "y-sorting-threshold" .!= defaultSortingThreshold + <*> parseXSortingThreshold o <*> o .:? "x-group-breakpoints" .!= defaultBreakpoints <*> parseSupportThreshold o <*> o .:? "max-support-coordinates" .!= defaultMaxSupportCoordinates diff --git a/test-extra/transformation/Spec.hs b/test-extra/transformation/Spec.hs index ce7067c5..23b89f18 100644 --- a/test-extra/transformation/Spec.hs +++ b/test-extra/transformation/Spec.hs @@ -157,6 +157,7 @@ main = hspec $ do supportRenameIdempotencySpec letterEndingNodesSpec ySortingBandingSpec + xColumnSortingSpec metadataAcrossTreesSpec metadataPreservedSpec triangleMetadataSpec diff --git a/test-extra/transformation/Spec/Helpers.hs b/test-extra/transformation/Spec/Helpers.hs index 07ae87d4..2b8b944f 100644 --- a/test-extra/transformation/Spec/Helpers.hs +++ b/test-extra/transformation/Spec/Helpers.hs @@ -2,6 +2,7 @@ module Spec.Helpers ( parseJbeamFile, vertexPositionsInOrder, + vertexCoordinatesInOrder, vertexCoordinates, effectiveMetaByCoordinate, metaNumber, @@ -53,6 +54,25 @@ vertexPositionsInOrder topNode = , Just (Number yNum) <- [inner V.!? 2] ] +{- | Every vertex coordinate in a top node's "nodes" section, in the order +`transform` wrote them out. Use this where the defect is about which vertex +ended up where, rather than about which vertices survived. +-} +vertexCoordinatesInOrder :: Node -> [(Double, Double, Double)] +vertexCoordinatesInOrder topNode = + case NP.queryNodes nodesQuery topNode >>= NP.expectArray nodesQuery of + Left _ -> [] + Right rows -> + [ (realToFrac (nvValue x), realToFrac (nvValue y), realToFrac (nvValue z)) + | row <- V.toList rows + , Just inner <- [expectArray row] + , Just (String name) <- [inner V.!? 0] + , name /= "id" + , Just (Number x) <- [inner V.!? 1] + , Just (Number y) <- [inner V.!? 2] + , Just (Number z) <- [inner V.!? 3] + ] + {- | Every vertex coordinate in a top node's "nodes" section. Positions survive renaming, so they identify a vertex across a transform. -} diff --git a/test-extra/transformation/Spec/Regression.hs b/test-extra/transformation/Spec/Regression.hs index ff041164..69425272 100644 --- a/test-extra/transformation/Spec/Regression.hs +++ b/test-extra/transformation/Spec/Regression.hs @@ -9,6 +9,7 @@ module Spec.Regression ( ySortingBandingSpec, metadataAcrossTreesSpec, metadataPreservedSpec, + xColumnSortingSpec, ) where import Data.Map qualified as M @@ -171,3 +172,43 @@ metadataPreservedSpec = ] M.keys metaAfter `shouldBe` M.keys metaBefore changed `shouldBe` [] + +{- | The same gen4-style left-side positions as the y-sorting fixture, read +for a different defect. Its five frontmost nodes sit in two vertical +columns: an inner one at X 0.780/0.920/0.953 and an outer one at X +0.998/1.036, the nose face and the fender beside it. Y and Z interleave +between the two columns, so no y-sorting-threshold can separate them; only +X can. Sorting a band by Z alone therefore climbs one column, jumps to the +other and comes back, which is what the jbeam maintainer marked up on his +render. + +The Y threshold here is 0.31 rather than the default because that is what puts +all five in one band, which is where the defect lives. It is not free choice: +between 0.153 and 0.16 the Y bands land on exactly the two columns, and this +assertion passes with nothing fixed at all. + +`xSortingThreshold` has to be set explicitly because it has no default. There is +no number that means off (0 gives every vertex its own band, which is the most +X sorting rather than none), so the field is optional and absent means the pass +does not run at all. +-} +xColumnSortingSpec :: Spec +xColumnSortingSpec = + describe "vertices in one Y band but different X columns" + . it "keeps each column contiguous instead of interleaving them" + $ do + let cfg = + newTransformationConfig + { ySortingThreshold = 0.31 + , xSortingThreshold = Just 0.2 + } + inner = [(0.953, -1.967, 0.122), (0.92, -1.953, 0.439), (0.78, -1.815, 0.719)] + outer = [(1.036, -1.807, 0.125), (0.998, -1.791, 0.473)] + topNode <- parseJbeamFile ySortingReproFixture + case transform M.empty cfg topNode of + Left err -> expectationFailure ("transform failed: " ++ T.unpack err) + Right (_, _, _, resultNode) -> + take 5 (leftGroup (vertexCoordinatesInOrder resultNode)) + `shouldBe` inner ++ outer + where + leftGroup = filter (\(x, _, _) -> x >= 0.09)