Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ensure no overlaps in initial placement of recognized structures #2212

Merged
merged 6 commits into from
Nov 29, 2024

Conversation

kostmo
Copy link
Member

@kostmo kostmo commented Nov 18, 2024

Implements overlap resolution for pre-placed recognized structures.

Also introduces a NonEmptyGrid type, towards #2004.

Testing

scripts/play.sh --scenario data/scenarios/Testing/1575-structure-recognizer/2201-initial-recognition-overlap.yaml --autoplay

Other changes

  • Enrich structure logging (e.g. OriginalName -> OrientedStructure)
  • Consolidate record fields (e.g. in LocatedStructure)
  • Introduce zipNumberedNE function
  • Rename some functions (e.g. mapIndexedMembers -> mapWithCoords)
  • Use StructureName, remove OriginalName

@kostmo kostmo added the Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality. label Nov 18, 2024
@kostmo kostmo changed the title initial placement ensure non-overlapping recognition ensure no overlaps in initial placement of recognized structures Nov 18, 2024
@kostmo kostmo force-pushed the feature/non-overlapping-initial-placement branch 2 times, most recently from b96c3fa to c11fe89 Compare November 19, 2024 20:23
@kostmo kostmo force-pushed the feature/non-overlapping-initial-placement branch from c11fe89 to c890f00 Compare November 19, 2024 20:31
@kostmo kostmo marked this pull request as ready for review November 19, 2024 20:39
Copy link
Member

@xsebek xsebek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does the overlap need to be detected? Does the structure recognition algorithm only find one?

Comment on lines 101 to 102
-- So we just use the same sorting criteria as the one used to resolve recognition
-- conflicts at entity placement time.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow. What are those criteria? A link to another comment or function would be helpful.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added more comments and cross-references.

@kostmo kostmo force-pushed the feature/non-overlapping-initial-placement branch from 0e1a798 to c2061c3 Compare November 24, 2024 22:33
@kostmo
Copy link
Member Author

kostmo commented Nov 24, 2024

Why does the overlap need to be detected? Does the structure recognition algorithm only find one?

While the game is being played, the structure recognition algorithm ensures that only one of potentially multiple overlapping structures will be recognized. However, at game state initialization time, we're not actually running the structure recognition automata. Instead we simply check all the locations of pre-placed structures and ensure they are still intact, given potential overlapping placements. Importantly, two structures could overlap but still be independently "intact"; this PR adds logic to ensure mutual-exclusivity.

import Swarm.Language.Syntax.Direction (AbsoluteDir)

newtype StructureName = StructureName Text
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
newtype StructureName = StructureName Text
newtype StructureName = StructureName { getStructureName :: Text }

Then we don't need the separate declaration of getStructureName.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This results in some kind of JSON parse error I've yet to look into.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, weird. Well, don't worry about it then, it's not a big deal.

@kostmo kostmo force-pushed the feature/non-overlapping-initial-placement branch from 097d111 to 587ad2b Compare November 26, 2024 06:48
@kostmo
Copy link
Member Author

kostmo commented Nov 29, 2024

Would you like to approve this, @byorgey?

Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry, thanks for the nudge!

@kostmo kostmo added the merge me Trigger the merge process of the Pull request. label Nov 29, 2024
@mergify mergify bot merged commit 3748049 into main Nov 29, 2024
14 checks passed
@mergify mergify bot deleted the feature/non-overlapping-initial-placement branch November 29, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge me Trigger the merge process of the Pull request. Z-Refactoring This issue is about restructuring the code without changing the behaviour to improve code quality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants