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

Fix world DSL coordinate bug #1988

Merged
merged 3 commits into from
Jun 25, 2024
Merged

Fix world DSL coordinate bug #1988

merged 3 commits into from
Jun 25, 2024

Conversation

byorgey
Copy link
Member

@byorgey byorgey commented Jun 25, 2024

The special variables x and y in the world DSL did not refer to the correct thing --- in the interpreter apparently I forgot to convert from Coords to Location and just used the row, column in Coords as the x and y. I have also fixed up all the scenarios which used x and y in their world description so that they now look identical to before, by replacing every use of x with -y and every use of y with x (though in some cases where the result would be algebraically equal I did not literally do this replacement, e.g. (x + y) % 2 is equal to ((-y) + x) % 2, so I left it alone).

@kostmo
Copy link
Member

kostmo commented Jun 25, 2024

There are actually a few scenarios that use x and y:

find data/scenarios -name "*.yaml" -exec sh -c "yq .world.dsl \"{}\" | grep --color=always '\b[xy]\b'" \; -print

shows

, if (x/5 + y/5) % 2 == 0 then {terrain: dirt} else {blank}
, if ((x + 3) % 19)/12 + (y % 19)/12 == 0 then {terrain: grass} else {blank}
data/scenarios/Testing/1535-ping/1535-in-range.yaml
, if (x/4 + y/4) % 2 == 0 then {terrain: dirt} else {blank}
, if ((x + 3) % 19)/12 + (y % 19)/12 == 0 then {terrain: grass} else {blank}
data/scenarios/Testing/1535-ping/1535-out-of-range.yaml
, if (x + y) % 2 == 0 then {erase} else {blank}
data/scenarios/Testing/1320-world-DSL/erase.yaml
  , mask (flowers && (x + y) % 3 == 0) {wildflower}
  , mask (rubble && (x + y) % 2 == 0) {rock}
  , mask (outerShore && (x + y) % 2 == 0) {reed}
data/scenarios/Challenges/Ranching/beekeeping.yaml
, if (x/5 + y/5) % 2 == 0 then {terrain: dirt} else {blank}
, if (((x + 3) % 20)/11 + ((y + 3) % 20)/11) == 0 then {terrain: grass} else {blank}
data/scenarios/Challenges/gallery.yaml
, if (x + y / 2) % 5 == 0 then {dirt, wavy water} else {blank}
data/scenarios/Challenges/wave.yaml
  , mask (y < -80 || y > 80 || x < -60 || x > 60) (overlay [{water}])
data/scenarios/Fun/horton.yaml

In fact I do recall some bewilderment about the horizontal and vertical directions while authoring wave.yaml, but I suppose I brushed it off :)

@byorgey
Copy link
Member Author

byorgey commented Jun 25, 2024

@kostmo Oh, right, of course! I just observed that all the tests still passed and assumed that meant nothing was using them. I'll go through and fix these up so the worlds look the same as before.

@byorgey
Copy link
Member Author

byorgey commented Jun 25, 2024

@kostmo should be all fixed up now!

@byorgey byorgey added the merge me Trigger the merge process of the Pull request. label Jun 25, 2024
@mergify mergify bot merged commit a687d54 into main Jun 25, 2024
14 checks passed
@byorgey byorgey deleted the push-munwpnkvlzkq branch June 25, 2024 21:37
mergify bot pushed a commit that referenced this pull request Jun 25, 2024
It turns out that the `hash` primitive was always computing the same deterministic hash of the coordinates regardless of the seed.  This meant that *e.g.* any world features placed in a way that depended on the hash were always in the same locations, and if you created a world that only relied on `hash` and not on, say, Perlin noise, it would always look the same regardless of the seed.  This does not seem desirable.

This PR fixes the DSL interpreter by passing the seed to the `murmur3` hash function (we used to always pass the constant seed 0).  In theory, this does mean that all the "classic" worlds are now slightly different than what they used to be (except for seed 0).  However, there's no way to tell the difference between one random placement and another.  The only scenario where we really depend on the particular locations of entities for a particular seed is the `world101` tutorial, but that used seed 0 anyway, which did not change.

Depends on merging #1988 first.
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants