Support a generic "original" payload in structure recognizer #2216

kostmo · 2024-11-30T07:40:54Z

The recognizer algorithm operates on a NonEmptyGrid of Maybe elements. However, there may exist some progenitor object with auxiliary metadata payload that is independent of the algorithm but should be presented along with the final recognized structure.

So long as an "extraction" function is provided that derives the nonempty grid of as from the original object b, we can send b along for the ride through the algorithm without concern to b's internals.

Changes

Overall this refactoring makes the code more generic and simpler.

Build the recognizer automata at scenario parse time
Enforce grid no emptiness earlier in recognizer construction
Subsume redundant _structureDefs member of StaticStructureInfo record with _staticAutomatons field
Make some type signatures in the UI more generic
Push the conversion from Cell to Entity further up the pipeline
Remove some derived Show instances (show-ing a Scenario is not likely to be useful due to sheer volume of content)

byorgey · 2024-12-02T23:31:05Z

@kostmo I am having trouble finding which modules contain the "main" changes (as opposed to other modules which "just" have to be updated to match the new types/definitions). Could you mention which modules are the interesting ones to look at?

kostmo

@byorgey I've annotated the most relevant changes now.

kostmo · 2024-12-03T02:13:22Z

src/swarm-scenario/Swarm/Game/Scenario.hs

-      symmetryAnnotatedGrids <- mapM checkSymmetry recognizableGrids
+          -- We exclude empty grids from the recognition engine.
+          nonEmptyRecognizableGrids = mapMaybe (traverse getNonEmptyGrid) recognizableGrids
+
+      myAutomatons <-
+        either (fail . T.unpack . renderRedundancy) return $
+          mkAutomatons (fmap cellToEntity) nonEmptyRecognizableGrids


@byorgey here is one interesting change. Previously the orientations to be recognized were checked for redundancy (according to symmetry) in the scenario parser. I have now moved that detail to within the recognizer API.

Related to this, we now instantiate the recognizer engine from within the scenario parsing code (instead of from within game state initialization), so that it can throw an error when a redundant recognition spec is provided.

kostmo · 2024-12-03T02:17:37Z

src/swarm-topography/Swarm/Game/Scenario/Topography/Structure/Recognition/Precompute.hs

 where
-  rotatedGrids = concatMap (extractGrids extractor . namedGrid) xs
+  extractedItems = map (uncurry ExtractedArea . fmap (extractor . structure) . dupe) rawGrids


Another major change: the "extraction" of Entities from Cells (which, from the perspective of the recognizer library, is the opaque type parameter b) is now only performed once in this single location, and the result is preserved and shared elsewhere it is needed. The "extraction" function also used to operate piecewise on each cell within a structure. Now, its contract is to take an entire "shape" of type b and produce a NonEmptyGrid (Maybe a).

kostmo · 2024-12-03T02:18:24Z

src/swarm-topography/Swarm/Game/Scenario/Topography/Structure/Recognition/Precompute.hs

-  Maybe (StructureWithGrid (Maybe b) a)
-extractOrientedGrid extractor x d =
-  case extractor <$> structure x of
-    EmptyGrid -> Nothing
-    Grid neGrid ->
-      let w = RowWidth . rectWidth . getNEGridDimensions $ neGrid
-       in Just $
-            StructureWithGrid wrapped d w $
-              applyOrientationTransformNE (Orientation d False) neGrid
+  StructureWithGrid b a
+extractOrientedGrid (ExtractedArea x neGrid) d =
+  StructureWithGrid d w $
+    ExtractedArea x $
+      applyOrientationTransformNE (Orientation d False) neGrid


This got much simpler since I pushed the enforcement of NonEmptyGrids further upstream.

kostmo · 2024-12-03T02:19:43Z

src/swarm-topography/Swarm/Game/Scenario/Topography/Structure/Recognition/Precompute.hs

-      x <- extractOrientedGrid extractor sGrid d
-      return $ PositionedStructure (Cosmic subworldName loc) x
+      return $
+        PositionedStructure (Cosmic subworldName loc) $
+          extractOrientedGrid (grid $ annotatedGrid sGrid) d



This was the other place that "extraction" was recomputed. Now we just re-use the result computed earlier.

kostmo · 2024-12-03T02:20:05Z

src/swarm-topography/Swarm/Game/Scenario/Topography/Structure/Recognition/Precompute.hs

-  definitionMap = M.fromList $ map ((name &&& id) . namedGrid) structDefs
+  definitionMap = theAutomatons ^. originalStructureDefinitions


We also no longer need to recompute this Map.

kostmo · 2024-12-03T02:21:10Z

src/swarm-topography/Swarm/Game/Scenario/Topography/Structure/Recognition/Static.hs

-data StaticStructureInfo b = StaticStructureInfo
-  { _structureDefs :: [SymmetryAnnotatedGrid (Maybe b)]
-  , _staticPlacements :: Map SubworldName [LocatedStructure]
-  }
-  deriving (Show)


This StaticStructureInfo record was relocated to the Swarm.Game.Scenario.Topography.Structure.Recognition.Type module. And its _structureDefs member is made redundant.

kostmo · 2024-12-03T02:22:42Z

src/swarm-topography/Swarm/Game/Scenario/Topography/Structure/Recognition/Tracking.hs

-  doAddition newEntity r = do
-    stateRevision <- case HM.lookup newEntity entLookup of
-      Nothing -> return oldRecognitionState
-      Just finder -> do
-        tell . pure . FoundParticipatingEntity $
-          ParticipatingEntity
-            newEntity
-            (finder ^. inspectionOffsets)
-        registerRowMatches entLoader cLoc finder oldRecognitionState
-
-    return $ r & recognitionState .~ stateRevision
-   where
-    oldRecognitionState = r ^. recognitionState
+  entLookup = autoRecognizer ^. automatonsByEntity

-  doRemoval = do
+  doAddition newEntity =
+    maybe return logAndRegister $ HM.lookup newEntity entLookup
+   where
+    logAndRegister finder s = do
+      tell . pure . FoundParticipatingEntity $
+        ParticipatingEntity
+          newEntity
+          (finder ^. inspectionOffsets)
+      registerRowMatches entLoader cLoc finder s


Here is just some independent refactoring to clean up this logic

kostmo · 2024-12-03T02:24:02Z

src/swarm-topography/Swarm/Game/Scenario/Topography/Structure/Recognition/Type.hs

+data ExtractedArea b a = ExtractedArea
+  { originalItem :: NamedArea b
+  , extractedGrid :: NonEmptyGrid (AtomicKeySymbol a)


This new ExtractedArea type is what propagates the original b through the algorithm.

kostmo · 2024-12-03T02:24:28Z

src/swarm-topography/Swarm/Game/Scenario/Topography/Structure/Recognition/Type.hs

-data NamedOriginal b = NamedOriginal
-  { getName :: StructureName
-  , orig :: NamedGrid b


Don't need NamedOriginal anymore; we just access the name through the NamedArea record.

byorgey

Thanks, the additional comments were very helpful! I think this looks good --- I'm always in favor of nice refactoring that makes things simpler + more modular!

byorgey · 2024-12-02T23:26:14Z

src/swarm-topography/Swarm/Game/Scenario/Topography/Structure/Recognition/Log.hs

@@ -20,7 +20,7 @@ import Swarm.Game.Universe (Cosmic)

 renderSharedNames :: ConsolidatedRowReferences b a -> NonEmpty StructureName
 renderSharedNames =
-  NE.nub . NE.map (getName . originalDefinition . wholeStructure) . referencingRows
+  NE.nub . NE.map (name . originalItem . entityGrid . wholeStructure) . referencingRows


I know the call to NE.nub wasn't added or changed in this PR, but I guess I didn't notice it before. Is it a reasonable guarantee that the list of things here will be small? I ask since NE.nub takes $O(n^2)$ time, so we should use if if and only if we know the list will not be too long.

Most likely the list is small (it's bounded by the number of user-defined structures/their sizes). I was aware of this $O(N^2)$ issue but did not see an alternative in Data.List.NonEmpty. However, now that I look, I see Data.List.NonEmpty.Extra.nubOrd, so I'll use that.

Huh, nubOrd uses a custom red-black tree implementation. I wonder why it doesn't just use Set. Well, it doesn't matter. Since the list we are nubbing is just a list of names, nubOrd should work well.

~~Could the red-black implementation possibly be order-preserving?~~

I wonder why it doesn't just use Set.

I presume it is to minimize dependencies of the extra package. Particularly, extra does not depend on containers.

Incidentally, now that I've read the code I see that nubOrd is order-preserving whereas nubSort is not. That fact would be good to note explicitly in the docstring (rather than indirectly by example).

kostmo force-pushed the refactor/more-nonempty-grid-usage branch 2 times, most recently from 69e4b82 to 3c4bc19 Compare December 1, 2024 07:08

github-actions bot mentioned this pull request Dec 1, 2024

Restyled [WIP] Support a generic "original" payload in structure recognizer #2217

Closed

kostmo force-pushed the refactor/more-nonempty-grid-usage branch 2 times, most recently from a204a0f to f863adc Compare December 2, 2024 06:25

kostmo changed the title ~~[WIP] Support a generic "original" payload in structure recognizer~~ Support a generic "original" payload in structure recognizer Dec 2, 2024

kostmo force-pushed the refactor/more-nonempty-grid-usage branch 2 times, most recently from b627c4d to fb8ea0d Compare December 2, 2024 23:13

kostmo requested a review from byorgey December 2, 2024 23:14

kostmo marked this pull request as ready for review December 2, 2024 23:14

kostmo added the Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality. label Dec 2, 2024

kostmo force-pushed the refactor/more-nonempty-grid-usage branch from fb8ea0d to e5decbf Compare December 3, 2024 02:07

kostmo commented Dec 3, 2024

View reviewed changes

byorgey approved these changes Dec 3, 2024

View reviewed changes

kostmo force-pushed the refactor/more-nonempty-grid-usage branch from e5decbf to 063dbc5 Compare December 4, 2024 00:04

github-actions bot mentioned this pull request Dec 4, 2024

Restyled Support a generic "original" payload in structure recognizer #2219

Closed

kostmo added the merge me Trigger the merge process of the Pull request. label Dec 4, 2024

refactor

e2094ff

kostmo force-pushed the refactor/more-nonempty-grid-usage branch from 063dbc5 to e2094ff Compare December 4, 2024 00:10

mergify bot merged commit 099b95f into main Dec 4, 2024
12 checks passed

mergify bot deleted the refactor/more-nonempty-grid-usage branch December 4, 2024 00:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support a generic "original" payload in structure recognizer #2216

Support a generic "original" payload in structure recognizer #2216

kostmo commented Nov 30, 2024 •

edited

Loading

byorgey commented Dec 2, 2024

kostmo left a comment

kostmo Dec 3, 2024

kostmo Dec 3, 2024

kostmo Dec 3, 2024

kostmo Dec 3, 2024

kostmo Dec 3, 2024

kostmo Dec 3, 2024

kostmo Dec 3, 2024

kostmo Dec 3, 2024

kostmo Dec 3, 2024

byorgey left a comment

byorgey Dec 2, 2024

kostmo Dec 3, 2024

byorgey Dec 3, 2024

kostmo Dec 3, 2024 •

edited

Loading

kostmo Dec 4, 2024

		definitionMap = M.fromList $ map ((name &&& id) . namedGrid) structDefs
		definitionMap = theAutomatons ^. originalStructureDefinitions

Support a generic "original" payload in structure recognizer #2216

Support a generic "original" payload in structure recognizer #2216

Conversation

kostmo commented Nov 30, 2024 • edited Loading

Changes

byorgey commented Dec 2, 2024

kostmo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byorgey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostmo Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kostmo commented Nov 30, 2024 •

edited

Loading

kostmo Dec 3, 2024 •

edited

Loading