<feed xmlns='http://www.w3.org/2005/Atom'>
<title>puzzles/grid.c, branch master</title>
<subtitle>My sgt-puzzles tree</subtitle>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/'/>
<entry>
<title>grid_edge_bydots_cmpfn: remove dangerous pointer comparison.</title>
<updated>2023-07-14T07:09:51+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-07-14T07:09:51+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=40d0de7a668ea4c95cdf06af4a1554ff0be6936d'/>
<id>40d0de7a668ea4c95cdf06af4a1554ff0be6936d</id>
<content type='text'>
Commit e6cdd70df867f06 made the grid_dot structures for a grid no
longer be elements of the same array. But I didn't notice that
grid_edge_bydots_cmpfn was doing pointer subtraction on them on the
assumption that they were.

Fixed by comparing the dots' new index fields, which should correspond
exactly to their previous positions in the single array, so the
behaviour should be just what it was before the change.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Commit e6cdd70df867f06 made the grid_dot structures for a grid no
longer be elements of the same array. But I didn't notice that
grid_edge_bydots_cmpfn was doing pointer subtraction on them on the
assumption that they were.

Fixed by comparing the dots' new index fields, which should correspond
exactly to their previous positions in the single array, so the
behaviour should be just what it was before the change.
</pre>
</div>
</content>
</entry>
<entry>
<title>grid.c: new and improved Penrose tiling generator.</title>
<updated>2023-07-07T17:17:02+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-07-07T17:16:04+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=61e9c782487ea982498955b93d1b94921131059e'/>
<id>61e9c782487ea982498955b93d1b94921131059e</id>
<content type='text'>
The new generator works on the same 'combinatorial coordinates' system
as the more recently written Hats and Spectre generators.

When I came up with that technique for the Hats tiling, I was already
tempted to rewrite the Penrose generator on the same principle,
because it has a lot of advantages over the previous technique of
picking a randomly selected patch out of the subdivision of a huge
starting tile. It generates the exact limiting distribution of
possible tiling patches rather than an approximation based on how big
a tile you subdivided; it doesn't use any overly large integers or
overly precise floating point to suffer overflow or significance loss,
because it never has to even _think_ about the coordinates of any
point not in the actual output area.

But I resisted the temptation to throw out our existing Penrose
generator and move to the shiny new algorithm, for just one reason:
backwards compatibility. There's no sensible way to translate existing
Loopy game IDs into the new notation, so they would stop working,
unless we kept the old generator around as well to interpret the
previous system. And although _random seeds_ aren't guaranteed to
generate the same result from one build of Puzzles to the next, I do
try to keep existing descriptive game IDs working.

So if we had to keep the old generator around anyway, it didn't seem
worth writing a new one...

... at least, until we discovered that the old generator has a latent
bug. The function that decides whether each tile is within the target
area, and hence whether to make it part of the output grid, is based
on floating-point calculation of the tile's vertices. So a change in
FP rounding behaviour between two platforms could cause them to
interpret the same grid description differently, invalidating a Loopy
game on that grid. This is _unlikely_, as long as everyone uses IEEE
754 double, but the C standard doesn't actually guarantee that.

We found this out when I started investigating a slight distortion in
large instances of Penrose Loopy. For example, the Loopy random seed
"40x40t11#12345", as of just before this commit, generates a game
description beginning with the Penrose grid string "G-4944,5110,108",
in which you can see a star shape of five darts a few tiles down the
left edge, where two of the radii of the star don't properly line up
with edges in the surrounding shell of kites that they should be
collinear with. This turns out to be due to FP error: not because
_double precision_ ran out, but because the definitions of COS54,
SIN54, COS18 and SIN18 in penrose.c were specified to only 7 digits of
precision. And when you expand a patch of tiling that large, you end
up with integer combinations of those numbers with coefficients about
7 digits long, mostly cancelling out to leave a much smaller output
value, and the inaccuracies in those constants start to be noticeable.

But having noticed that, it then became clear that these badly
computed values were also critical to _correctness_ of the grid. So
they can't be fixed without invalidating old game IDs. _And_ then we
realised that this also means existing platforms might disagree on a
game ID's validity.

So if we have to break compatibility anyway, we should go all the way,
and instead of fixing the old system, introduce the shiny new system
that gets all of this right. Therefore, the new penrose.c uses the
more reliable combinatorial-coordinates system which doesn't deal in
integers that large in the first place. Also, there's no longer any
floating point at all in the calculation of tile vertex coordinates:
the combinations of 1 and sqrt(5) are computed exactly by the
n_times_root_k function. So now a large Penrose grid should never have
obvious failures of alignment like that.

The old system is kept for backwards compatibility. It's not fully
reliable, because that bug hasn't been fixed - but any Penrose Loopy
game ID that was working before on _some_ platform should still work
on that one. However, new Penrose Loopy game IDs are based on
combinatorial coordinates, computed in exact arithmetic, and should be
properly stable.

The new code looks suspiciously like the Spectre code (though
considerably simpler - the Penrose coordinate maps are easy enough
that I just hand-typed them rather than having an elaborate auxiliary
data-generation tool). That's because they were similar enough in
concept to make it possible to clone and hack. But there are enough
divergences that it didn't seem like a good idea to try to merge them
again afterwards - in particular, the fact that output Penrose tiles
are generated by merging two triangular metatiles while Spectres are
subdivisions of their metatiles.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
The new generator works on the same 'combinatorial coordinates' system
as the more recently written Hats and Spectre generators.

When I came up with that technique for the Hats tiling, I was already
tempted to rewrite the Penrose generator on the same principle,
because it has a lot of advantages over the previous technique of
picking a randomly selected patch out of the subdivision of a huge
starting tile. It generates the exact limiting distribution of
possible tiling patches rather than an approximation based on how big
a tile you subdivided; it doesn't use any overly large integers or
overly precise floating point to suffer overflow or significance loss,
because it never has to even _think_ about the coordinates of any
point not in the actual output area.

But I resisted the temptation to throw out our existing Penrose
generator and move to the shiny new algorithm, for just one reason:
backwards compatibility. There's no sensible way to translate existing
Loopy game IDs into the new notation, so they would stop working,
unless we kept the old generator around as well to interpret the
previous system. And although _random seeds_ aren't guaranteed to
generate the same result from one build of Puzzles to the next, I do
try to keep existing descriptive game IDs working.

So if we had to keep the old generator around anyway, it didn't seem
worth writing a new one...

... at least, until we discovered that the old generator has a latent
bug. The function that decides whether each tile is within the target
area, and hence whether to make it part of the output grid, is based
on floating-point calculation of the tile's vertices. So a change in
FP rounding behaviour between two platforms could cause them to
interpret the same grid description differently, invalidating a Loopy
game on that grid. This is _unlikely_, as long as everyone uses IEEE
754 double, but the C standard doesn't actually guarantee that.

We found this out when I started investigating a slight distortion in
large instances of Penrose Loopy. For example, the Loopy random seed
"40x40t11#12345", as of just before this commit, generates a game
description beginning with the Penrose grid string "G-4944,5110,108",
in which you can see a star shape of five darts a few tiles down the
left edge, where two of the radii of the star don't properly line up
with edges in the surrounding shell of kites that they should be
collinear with. This turns out to be due to FP error: not because
_double precision_ ran out, but because the definitions of COS54,
SIN54, COS18 and SIN18 in penrose.c were specified to only 7 digits of
precision. And when you expand a patch of tiling that large, you end
up with integer combinations of those numbers with coefficients about
7 digits long, mostly cancelling out to leave a much smaller output
value, and the inaccuracies in those constants start to be noticeable.

But having noticed that, it then became clear that these badly
computed values were also critical to _correctness_ of the grid. So
they can't be fixed without invalidating old game IDs. _And_ then we
realised that this also means existing platforms might disagree on a
game ID's validity.

So if we have to break compatibility anyway, we should go all the way,
and instead of fixing the old system, introduce the shiny new system
that gets all of this right. Therefore, the new penrose.c uses the
more reliable combinatorial-coordinates system which doesn't deal in
integers that large in the first place. Also, there's no longer any
floating point at all in the calculation of tile vertex coordinates:
the combinations of 1 and sqrt(5) are computed exactly by the
n_times_root_k function. So now a large Penrose grid should never have
obvious failures of alignment like that.

The old system is kept for backwards compatibility. It's not fully
reliable, because that bug hasn't been fixed - but any Penrose Loopy
game ID that was working before on _some_ platform should still work
on that one. However, new Penrose Loopy game IDs are based on
combinatorial coordinates, computed in exact arithmetic, and should be
properly stable.

The new code looks suspiciously like the Spectre code (though
considerably simpler - the Penrose coordinate maps are easy enough
that I just hand-typed them rather than having an elaborate auxiliary
data-generation tool). That's because they were similar enough in
concept to make it possible to clone and hack. But there are enough
divergences that it didn't seem like a good idea to try to merge them
again afterwards - in particular, the fact that output Penrose tiles
are generated by merging two triangular metatiles while Spectres are
subdivisions of their metatiles.
</pre>
</div>
</content>
</entry>
<entry>
<title>grid.c: add dot coordinates to debugging dumps.</title>
<updated>2023-07-07T17:17:02+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-07-06T17:20:37+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=23d4322fec38f8ffac930bc541e08309e1d02f11'/>
<id>23d4322fec38f8ffac930bc541e08309e1d02f11</id>
<content type='text'>
I wanted these in order to try to check whether all the faces of a
grid were being traversed in the right orientation. That turned out
not to be the cause of my problem, but it's still useful information
to put in diagnostics.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
I wanted these in order to try to check whether all the faces of a
grid were being traversed in the right orientation. That turned out
not to be the cause of my problem, but it's still useful information
to put in diagnostics.
</pre>
</div>
</content>
</entry>
<entry>
<title>grid.c: allocate face/edge/dot arrays incrementally.</title>
<updated>2023-07-07T17:17:02+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-07-06T11:50:49+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=e6cdd70df867f064b0358dc3dba56d2667ab80a5'/>
<id>e6cdd70df867f064b0358dc3dba56d2667ab80a5</id>
<content type='text'>
Previously, the 'faces', 'edges' and 'dots' arrays in a grid structure
were arrays of actual grid_face, grid_edge and grid_dot structures,
physically containing all the data about the grid. But they also
referred to each other by pointers, which meant that they were hard to
realloc larger (you'd have to go through and rewrite all the pointers
whenever the base of an array moved). And _that_ meant that every grid
type had to figure out a reasonably good upper bound on the number of
all those things it was going to need, so that it could allocate those
arrays the right size to begin with, and not have to grow them
painfully later.

For some grid types - particularly the new aperiodic ones - that was
actually a painful part of the job. So I think enough is enough:
counting up how much memory you're going to need before using it is a
mug's game, and we should instead realloc on the fly.

So now g-&gt;faces, g-&gt;edges and g-&gt;dots have an extra level of
indirection. Instead of being arrays of actual structs, they're arrays
of pointers, and each struct in them is individually allocated. Once a
grid_face has been allocated, it never moves again, even if the array
of pointers changes size.

The old code had a common idiom of recovering the array index of a
particular element by subtracting it from the array base, e.g. writing
'f - g-&gt;faces' or 'e - g-&gt;edges'. To make that lookup remain possible,
I've added an 'index' field to each sub-structure, which is
initialised at the point where we decide what array index it will live
at.

This should involve no functional change, but the code that previously
had to figure out max_faces and max_dots up front for each grid type
is now completely gone, and nobody has to solve those problems in
advance any more.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
Previously, the 'faces', 'edges' and 'dots' arrays in a grid structure
were arrays of actual grid_face, grid_edge and grid_dot structures,
physically containing all the data about the grid. But they also
referred to each other by pointers, which meant that they were hard to
realloc larger (you'd have to go through and rewrite all the pointers
whenever the base of an array moved). And _that_ meant that every grid
type had to figure out a reasonably good upper bound on the number of
all those things it was going to need, so that it could allocate those
arrays the right size to begin with, and not have to grow them
painfully later.

For some grid types - particularly the new aperiodic ones - that was
actually a painful part of the job. So I think enough is enough:
counting up how much memory you're going to need before using it is a
mug's game, and we should instead realloc on the fly.

So now g-&gt;faces, g-&gt;edges and g-&gt;dots have an extra level of
indirection. Instead of being arrays of actual structs, they're arrays
of pointers, and each struct in them is individually allocated. Once a
grid_face has been allocated, it never moves again, even if the array
of pointers changes size.

The old code had a common idiom of recovering the array index of a
particular element by subtracting it from the array base, e.g. writing
'f - g-&gt;faces' or 'e - g-&gt;edges'. To make that lookup remain possible,
I've added an 'index' field to each sub-structure, which is
initialised at the point where we decide what array index it will live
at.

This should involve no functional change, but the code that previously
had to figure out max_faces and max_dots up front for each grid type
is now completely gone, and nobody has to solve those problems in
advance any more.
</pre>
</div>
</content>
</entry>
<entry>
<title>Move mul_root3 out into misc.c and generalise it.</title>
<updated>2023-07-07T17:17:02+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-07-02T20:22:02+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=6b5142a7a9b31922d9c7ef505b27c33d551f5016'/>
<id>6b5142a7a9b31922d9c7ef505b27c33d551f5016</id>
<content type='text'>
I'm going to want to reuse it for sqrt(5) as well as sqrt(3) soon.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
I'm going to want to reuse it for sqrt(5) as well as sqrt(3) soon.
</pre>
</div>
</content>
</entry>
<entry>
<title>Loopy / grid.c: support the new Spectre monotiling.</title>
<updated>2023-06-16T18:15:47+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-06-16T17:30:53+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=a33d9fad02d6319cb9061119a6165ed5493a3ba5'/>
<id>a33d9fad02d6319cb9061119a6165ed5493a3ba5</id>
<content type='text'>
This uses a tile shape very similar to the hat, but the tiling
_structure_ is totally changed so that there aren't any reflected
copies of the tile.

I'm not sure how much difference this makes to gameplay: the two
tilings are very similar for Loopy purposes. But the code was fun to
write, and I think the Spectre shape is noticeably prettier, so I'm
adding this to the collection anyway.

The test programs also generate a pile of SVG images used in the
companion article on my website.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
This uses a tile shape very similar to the hat, but the tiling
_structure_ is totally changed so that there aren't any reflected
copies of the tile.

I'm not sure how much difference this makes to gameplay: the two
tilings are very similar for Loopy purposes. But the code was fun to
write, and I think the Spectre shape is noticeably prettier, so I'm
adding this to the collection anyway.

The test programs also generate a pile of SVG images used in the
companion article on my website.
</pre>
</div>
</content>
</entry>
<entry>
<title>Reorganise the dsf API into three kinds of dsf.</title>
<updated>2023-04-20T17:39:41+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-04-20T16:27:21+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=c5e253a9f9d3d651227ccad56e2c7526ee1f3eba'/>
<id>c5e253a9f9d3d651227ccad56e2c7526ee1f3eba</id>
<content type='text'>
This is preparing to separate out the auxiliary functionality, and
perhaps leave space for making more of it in future.

The previous name 'edsf' was too vague: the 'e' stood for 'extended',
and didn't say anything about _how_ it was extended. It's now called a
'flip dsf', since it tracks whether elements in the same class are
flipped relative to each other. More importantly, clients that are
going to use the flip tracking must say so when they allocate the dsf.

And Keen's need to track the minimal element of an equivalence class
is going to become a non-default feature, so there needs to be a new
kind of dsf that specially tracks those, and Keen will have to call it.

While I'm here, I've renamed the three dsf creation functions so that
they start with 'dsf_' like all the rest of the dsf API.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
This is preparing to separate out the auxiliary functionality, and
perhaps leave space for making more of it in future.

The previous name 'edsf' was too vague: the 'e' stood for 'extended',
and didn't say anything about _how_ it was extended. It's now called a
'flip dsf', since it tracks whether elements in the same class are
flipped relative to each other. More importantly, clients that are
going to use the flip tracking must say so when they allocate the dsf.

And Keen's need to track the minimal element of an equivalence class
is going to become a non-default feature, so there needs to be a new
kind of dsf that specially tracks those, and Keen will have to call it.

While I'm here, I've renamed the three dsf creation functions so that
they start with 'dsf_' like all the rest of the dsf API.
</pre>
</div>
</content>
</entry>
<entry>
<title>Declare all dsfs as a dedicated type name 'DSF'.</title>
<updated>2023-04-20T16:23:21+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-04-20T13:06:43+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=89c438e149a91fffa74b2669f7e0cd05abc3420f'/>
<id>89c438e149a91fffa74b2669f7e0cd05abc3420f</id>
<content type='text'>
In this commit, 'DSF' is simply a typedef for 'int', so that the new
declaration form 'DSF *' translates to the same type 'int *' that dsfs
have always had. So all we're doing here is mechanically changing type
declarations throughout the code.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
In this commit, 'DSF' is simply a typedef for 'int', so that the new
declaration form 'DSF *' translates to the same type 'int *' that dsfs
have always had. So all we're doing here is mechanically changing type
declarations throughout the code.
</pre>
</div>
</content>
</entry>
<entry>
<title>Use a dedicated free function to free dsfs.</title>
<updated>2023-04-20T16:21:12+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-04-20T12:35:58+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=bb561ee3b18be69e52b17cedde50eac96ea409da'/>
<id>bb561ee3b18be69e52b17cedde50eac96ea409da</id>
<content type='text'>
No functional change: currently, this just wraps the previous sfree
call.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
No functional change: currently, this just wraps the previous sfree
call.
</pre>
</div>
</content>
</entry>
<entry>
<title>Fall back to &lt;math.h&gt; if &lt;tgmath.h&gt; doesn't work.</title>
<updated>2023-04-06T06:08:04+00:00</updated>
<author>
<name>Simon Tatham</name>
<email>anakin@pobox.com</email>
</author>
<published>2023-04-06T06:07:30+00:00</published>
<link rel='alternate' type='text/html' href='https://www.franklinwei.com/cgit/puzzles/commit/?id=3b9cafa09f783ccadda14d11fc8b73dc496368c0'/>
<id>3b9cafa09f783ccadda14d11fc8b73dc496368c0</id>
<content type='text'>
This fixes a build failure introduced by commit 2e48ce132e011e8
yesterday.

When I saw that commit I expected the most likely problem would be in
the NestedVM build, which is currently the thing with the most most
out-of-date C implementation. And indeed the NestedVM toolchain
doesn't have &lt;tgmath.h&gt; - but much more surprisingly, our _Windows_
builds failed too, with a compile error inside &lt;tgmath.h&gt; itself!

I haven't looked closely into the problem yet. Our Windows builds are
done with clang, which comes with its own &lt;tgmath.h&gt; superseding the
standard Windows one. So you'd _hope_ that clang could make sense of
its own header! But perhaps the problem is that this is an unusual
compile mode and hasn't been tested.

My fix is to simply add a cmake check for &lt;tgmath.h&gt; - which doesn't
just check the file's existence, it actually tries compiling a file
that #includes it, so it will detect 'file exists but is mysteriously
broken' just as easily as 'not there at all'. So this makes the builds
start working again, precisely on Ben's theory of opportunistically
using &lt;tgmath.h&gt; where possible and falling back to &lt;math.h&gt;
otherwise.

It looks ugly, though! I'm half tempted to make a new header file
whose job is to include a standard set of system headers, just so that
that nasty #ifdef doesn't have to sit at the top of almost all the
source files. But for the moment this at least gets the build working
again.
</content>
<content type='xhtml'>
<div xmlns='http://www.w3.org/1999/xhtml'>
<pre>
This fixes a build failure introduced by commit 2e48ce132e011e8
yesterday.

When I saw that commit I expected the most likely problem would be in
the NestedVM build, which is currently the thing with the most most
out-of-date C implementation. And indeed the NestedVM toolchain
doesn't have &lt;tgmath.h&gt; - but much more surprisingly, our _Windows_
builds failed too, with a compile error inside &lt;tgmath.h&gt; itself!

I haven't looked closely into the problem yet. Our Windows builds are
done with clang, which comes with its own &lt;tgmath.h&gt; superseding the
standard Windows one. So you'd _hope_ that clang could make sense of
its own header! But perhaps the problem is that this is an unusual
compile mode and hasn't been tested.

My fix is to simply add a cmake check for &lt;tgmath.h&gt; - which doesn't
just check the file's existence, it actually tries compiling a file
that #includes it, so it will detect 'file exists but is mysteriously
broken' just as easily as 'not there at all'. So this makes the builds
start working again, precisely on Ben's theory of opportunistically
using &lt;tgmath.h&gt; where possible and falling back to &lt;math.h&gt;
otherwise.

It looks ugly, though! I'm half tempted to make a new header file
whose job is to include a standard set of system headers, just so that
that nasty #ifdef doesn't have to sit at the top of almost all the
source files. But for the moment this at least gets the build working
again.
</pre>
</div>
</content>
</entry>
</feed>
