Fix growable structure grids for multiple children with negative coordinates #2127
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
kostmo
force-pushed
the
testing/structure-expansion
branch
from
7bf111e
to
477b93d
Compare
kostmo
force-pushed
the
testing/structure-expansion
branch
from
477b93d
to
8df7dea
Compare
kostmo
added
the
T-Testing
kostmo
force-pushed
the
testing/structure-expansion
branch
from
8df7dea
to
6e7278d
Compare
kostmo
force-pushed
the
testing/structure-expansion
branch
from
2cfc312
to
6c93e28
Compare
kostmo
changed the title
kostmo
force-pushed
the
testing/structure-expansion
branch
from
6c93e28
to
f5b70f3
Compare
kostmo
changed the title
kostmo
force-pushed
the
testing/structure-expansion
branch
from
f5b70f3
to
0443bc2
Compare
kostmo
force-pushed
the
testing/structure-expansion
branch
from
0443bc2
to
5455c99
Compare
byorgey
reviewed
Sep 16, 2024
byorgey
approved these changes
Sep 16, 2024
mergify bot
pushed a commit
that referenced
this pull request
Sep 23, 2024
Builds upon #2127 to fix the remaining issues with #1826. If a structure incorporates sub-placements entailing northwesterly offsets, its "coordinate origin" will be shifted relative to the top-left cell in the grid. This updated coordinate origin should be propagated to parent structures for use when placing it. This includes placement of the main "area" onto the toplevel world map. This is essential when composing a large scene that needs to line up with features generated by the DSL. Another bug fixed in this PR involved incorrect "area" computation within sibling placements when both a "northward" and "westward" offset were used; existing tests only covered each of these directions separately. ## Changes in this PR * Refactoring for readability * Improved naming * Fixed typo `padSouthwest` -> `padNorthwest` * Export some functions for unit tests * Utilize propagated coordinate offset in `WorldDescription` ## Testing ### Unit tests ``` scripts/test/run-tests.sh swarm-unit --test-options '--pattern "Overlay"' ``` ### Scenarios ``` scripts/play.sh -i data/scenarios/Testing/1780-structure-merge-expansion/coordinate-offset-propagation.yaml --hide-goal ``` | Before | After | | --- | --- | |  |  | ``` scripts/play.sh -i data/scenarios/Testing/1780-structure-merge-expansion/simultaneous-north-and-west-offset.yaml --hide-goal ``` | Before | After | | --- | --- | |  |  |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes a bug from #1826.
Background
Structure definitions can be nested, in that a structure can reference and place multiple "substructures", each with an "offset", to compose something more complicated. Each placement onto a particular "base" structure is a "child". Multiple child placements atop the same base structure are "siblings".
It so happened that if a child placement with a "negative" horizontal offset preceded a sibling with non-negative offset, this would result in miscalculated placements. This had to do with the fact that "growing" the "base grid" in the negative direction means that the original "origin" (that the "offset" of subsequent placements are made with respect to) of the base grid must be shifted further right (to somewhere in the middle of the grid), rather than lying on the left edge.
Conversely, if negative offsets occurred as later siblings, then the placement would be correct (the bug would not be triggered).
The fix
This fix ensures that sibling placements can be re-ordered without affecting the end result. There were actually two bugs:
foldlM)originDeltain theSemigroupinstance ofPositionedGrid) was incorrect.Other changes
Also implements a
--refreshoption to thestandalone-topographytest for updating image-based test fixtures.Bug repro
Refreshing test images