If you want to remove your saved preferences for a puzzle for any
reason (perhaps because of one of those unsympathetic managers, or
perhaps because it's become corrupted in some way), you can of course
manually go and find the config file and delete it. But if you're not
sure where it is, it's helpful to have a method of telling the puzzle
itself to delete the config file.
Perhaps it would be useful to expose this in the GUI as well, though
I'm not quite sure where is the best place to put it.
(_Perhaps_ it would also be useful to have a more thorough option that
deleted _all_ puzzles' configurations - although that would involve
telling every separate puzzle binary in this collection what all the
other ones' names are, which would be an entirely new build headache.)
Finally, some user-visible behaviour changes as a payoff for all that
preparation work! In this commit, the GTK puzzles get a 'Preferences'
option in the menu, which presents a dialog box to configure the
preference settings.
On closing that dialog box, the puzzle preferences are enacted
immediately, and also saved to a configuration file where the next run
of the same puzzle will reload them.
The default file location is ~/.config/sgt-puzzles/<puzzlename>.conf,
although you can override the .config dir via $XDG_CONFIG_HOME or
override the Puzzles-specific subdir with $SGT_PUZZLES_DIR.
This is the first commit that actually exposes all the new preferences
work to the user, and therefore, I've also added documentation of all
the current preference options.
With this option turned off (it's on by default), the single-letter
keyboard shortcuts like 'q' for quit and 'n' for new game don't
function any more. You can still access the same functions via more
complicated shortcuts like Ctrl-Q or Ctrl-N, and front ends can
provide any other UI they like for the same operations, but this way,
people aren't at risk of blowing away half an hour of puzzling with
one misaimed key.
This is a thing people have occasionally asked for, and I've generally
resisted on the grounds that I have sympathy for people playing
puzzles at work who need to be able to close the game quickly when an
unsympathetic boss wanders by. But now we have a preferences system,
we can cater to those people _and_ the ones who don't mind.
More immediately useful: adding _at least one_ universal preference in
the initial version of this system means that there will be no games
with no preference options at all, and therefore, no need to put
conditionals all through the participating frontends to deal with
whether the Preferences menu option should even be provided. That
would have been a waste of time because all those conditionals would
just have to be removed again as soon as the first universal
preference came along - so adding an easy one _now_ means we can save
the effort of putting in the temporary conditionals in the first
place.
This commit introduces a serialisation format for the user preferences
stored in game_ui, using the keyword identifiers that get_prefs is
required to write into its list of config_item. As a result, the
serialisation format looks enough like an ordinary config file that a
user could write one by hand.
The preferences for the game backend are kept in serialised form in
me->be_prefs. The typical use of this is to apply it to a just-created
game_ui by calling midend_apply_prefs(), which deserialises the prefs
buffer into a list of config_item and passes it to the backend's
set_prefs function, overwriting the preference fields (but no others)
of the game_ui.
This is duly done when creating a new game, when loading a game from a
save file, and also when printing a puzzle. To make the latter work,
document_add_puzzle now takes a game_ui (and keeps ownership of it
afterwards), and passes that to the backend's compute_size and print
functions.
The backend's own get_prefs and set_prefs functions are wrapped by
midend_get_prefs and midend_set_prefs. This is partly as a convenience
(it deals with optionally constructing a game_ui specially to call the
backend with), but mostly so that there will be a convenient place in
the midend to add standard preferences applying across all puzzles.
No cross-puzzle preferences are provided yet.
There are two external interfaces to all this, and in this commit,
neither one is yet called by any frontend:
A new pair of midend functions is exposed to the front end, called
midend_load_prefs and midend_save_prefs. These have a similar API to
midend_serialise and midend_deserialise, taking a read/write function
pointer and a context. So front ends that can already load/save a game
to a file on disk should find it easy to add a similar set of
functions loading/saving user preferences.
Secondly, a new value CFG_PREFS is added to the enumeration of
configuration dialog types, alongside the ones for the Custom game
type, entering a game description and entering a random seed. This
should make it easy for frontends to offer a Preferences dialog,
because it will operate almost exactly like three dialogs they already
handle.
This will be necessary in the next commit, so that the midend can make
a game_ui out of nothing in order to store preferences in it.
The only game actually affected by this requirement is Pearl, which
has a preference (GUI style) and also allocates space based on the
game_state's grid size to store the coordinates of a path being
dragged, so if there is no game_state, it can't do the latter - which
is OK, because it also won't be expected to.
These are currently only used for internal serialisation of midend
state in order to undo/redo across New Game, and are named
accordingly. But I'm about to reuse them for another set of
serialisation functions, so they'll want more general names, and also
need to be moved higher up in the source file so as to have been
defined where I'll want to use them.
These are similar to the existing pair configure() and custom_params()
in that get_prefs() returns an array of config_item describing a set
of dialog-box controls to present to the user, and set_prefs()
receives the same array with answers filled in and implements the
answers. But where configure() and custom_params() operate on a
game_params structure, the new pair operate on a game_ui, and are
intended to permit GUI configuration of all the settings I just moved
into that structure.
However, nothing actually _calls_ these routines yet. All I've done in
this commit is to add them to 'struct game' and implement them for the
functions that need them.
Also, config_item has new fields, permitting each config option to
define a machine-readable identifying keyword as well as the
user-facing description. For options of type C_CHOICES, each choice
also has a keyword. These keyword fields are only defined at all by
the new get_prefs() function - they're left uninitialised in existing
uses of the dialog system. The idea is to use them when writing out
the user's preferences into a configuration file on disk, although I
haven't actually done any of that work in this commit.
Environment variables that set specific settings of particular
puzzles, such as SLANT_SWAP_BUTTONS, LIGHTUP_LIT_BLOBS and
LOOPY_AUTOFOLLOW, now all affect the game behaviour via fields in
game_ui instead of being looked up by getenv in the individual
functions that need to know them.
The purpose of this refactoring is to put those config fields in a
place where other more user-friendly configuration systems will also
be able to access them, once I introduce one. However, for the moment,
there's no functional change: I haven't _yet_ changed how the user
sets those options. They're still set by environment variables alone.
All I'm changing here is where the settings are stored inside the
code, and exactly when they're read out of the environment to put into
the game_ui.
Specifically, the getenvs now happen during new_ui(). Or rather, in
all the puzzles I've changed here, they happen in a subroutine
legacy_prefs_override() called from within new_ui(), after it's set up
the default values for the settings, and then gives the environment a
chance to override them. Or rather, legacy_prefs_override() only
actually calls getenv the first time, and after that, it's cached the
answers it got.
In order to make the override functions less wordy, I've altered the
prototype of getenv_bool so that it returns an int rather than a bool,
and takes its default return value in the same form. That way you can
set the default to something other than 0 or 1, and find out whether a
value was present at all.
This commit only touches environment configuration specific to an
individual puzzle. The midend also has some standard environment-based
config options that apply to all puzzles, such as colour scheme and
default presets and preset-menu extension. I haven't handled those
yet.
I'm about to move some of the bodgy getenv-based options so that they
become fields in game_ui. So these functions, which could previously
access those options directly via getenv, will now need to be given a
game_ui where they can look them up.
Turns out that was another case where we were assuming the canonical
dsf element was also the minimal one, because we process the clues in
order and expect to start a linked list at the canonical element of
each region and then add the rest of the cells to the existing one.
Easily fixed by using the minimal element again.
When playing on a high-DPI screen and running Loopy at a large tile
size to compensate, the faint lines along grid edges in LINE_NO state
are exceptionally hard to see, because they're still only one pixel
wide even when everything else has been expanded.
This has been true for ages, but it's more significant now, because
the new Hats tiling mode needs a lot of counting of edges in all
states (since the hats have 14 edges in total!), and it's awkward not
to be able to see exactly where the LINE_NO edges are, or where an
edge between two other tiles meets this hat's outline.
If you configure a Linux build of Puzzles with -fsanitize=address, it
will fail during the icons build, because the icon-maker programs leak
memory when they're run, and by default, this causes ASan to report
all the memory leaks at the end of the program *and then exit 1*.
I don't think 'just fix the memory leaks' is a viable answer. _Some_
of the leaks come from the Puzzles code, and could be fixed by being
more punctilious about freeing everything before exiting (although
that is not necessary for any actually sensible purpose and would
_only_ serve to stop Leak Sanitiser complaining). But some are outside
the Puzzles code completely, apparently from fontconfig. So even if we
fixed all the leaks we could find, we wouldn't prevent the failure
status.
When I want to build with ASan, I've been working around this by
setting ASAN_OPTIONS=detect_leaks=0 in the environment before running
the build command. Easier all round if we just do that _inside_ the
cmake setup.
This rewrite improves the core data structure implementation in two
ways. Firstly, when merging two equivalence classes, we check their
relative sizes, and choose the larger class's canonical element to be
the overall root of the new class tree. This minimises the number of
overlong paths to the root after the merge. Secondly, we defer path
compression until _after_ the two classes are merged, rather than do
it beforehand (via using edsf_canonify as a subroutine) and then have
to do it wastefully again afterwards.
The size-based root selection was what we _used_ to do, and delivers
the better asymptotic performance. I reverted it so that Keen could
track the min of each equivalence class. But since then I've realised
you can have the asymptotic goodness _and_ min-tracking if you store
the minima separately from the main data structure. So now Keen does
that, and other clients don't have to pay the cost.
Similarly, the flip tracking is now a cost that only users of flip
dsfs have to pay, because a normal one doesn't store that information
at all.
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.
print_dsf was declared in puzzles.h, but never called, and its
definition was commented out. So it probably wouldn't still have
worked anyway. The other commented-out printfs in that file don't look
very useful either, and they just mean more stuff will need messing
about with as I continue to refactor.
Now that the dsf knows its own size internally, there's no need to
tell it again when one is copied or reinitialised.
This makes dsf_init much more about *re*initialising a dsf, since now
dsfs are always allocated using a function that will initialise them
anyway. So I think it deserves a rename.
This permits bounds-checking of all inputs to dsf_canonify and
dsf_merge, so that any out-of-range values will provoke assertion
failure instead of undefined behaviour.
This makes good on all the previous preparatory commits, which I did
separately so that each one individually has a reasonably readable
diff, and all the mechanical changes are separated out from the
rewrites that needed actual thought.
Still no functional change, however: the DSF type wraps nothing but
the same int pointer that 'DSF *' used to store directly.
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.
Previously we were duplicating the contents of a dsf using straight-up
memcpy. Now there's a dsf_copy function wrapping the same memcpy.
For the moment, this still has to take a size parameter, because the
size isn't stored inside the dsf itself. But once we make a proper
data type, it will be.
I'm going to work towards turning 'dsf' into an opaque type, so that I
can improve its implementation without breaking clients. The first
step is to deal manually with puzzles that currently reuse a single
array of ints for multiple purposes, some of which are dsf and some
are not.
In divvy_rectangle_attempt, 'tmp' was used as an int array and later a
dsf, and I've made a new 'tmpdsf' to be the latter.
In Dominosa, solver->pc_scratch2 was sometimes a dsf, and now there's
a new solver->dsf_scratch.
In Map, parse_edge_list() needed a dsf internally and then never
exported it; it expected to be passed an array of 2*w*h ints and used
the second half as a dsf. Now it expects only w*h ints, and allocates
its own dsf internally, freeing it again before returning.
And in Tents, find_errors() was allocating a single block of 2*w*h
ints and using the second half of it as a dsf, apparently just to save
one malloc. Now we malloc and free the dsf separately.
unfinished/separate.c had its own declaration of divvy_rectangle(),
duplicating the one in puzzles.h. Probably that was where the
declaration originally lived, before I moved it out into the main
header.
I just found these self-tests lying around in mines.c under an #ifdef
that nobody ever enables. Let's put them somewhere more sensible! We
already have a separate tool for working with the obfuscation system
in a puzzle-independent way, and it seems reasonable to put them in
there.
In commit 8d6647548f7d005 I added the Hats grid type to Loopy, and
mentioned in the commit message that I was very pleased with the
algorithm I came up with.
In fact, I was so pleased with it that I've decided it deserves a
proper public writeup. So I've spent the Easter weekend producing one:
https://www.chiark.greenend.org.uk/~sgtatham/quasiblog/aperiodic-tilings/
In this commit I adjust the header comments in both penrose.c and
hat.c to refer to the article (replacing a previous comment in
penrose.c to a much less polished page containing a copy of my
jotting-grade personal notes that I sent James Harvey once). Also,
added some code to hatgen.c to output Python hat descriptions in a
similar style to hat-test, which I used to generate a couple of the
more difficult diagrams in the new article, and didn't want to lose.
Some nonogram implementations allow zero clues so that a row or column
with a single zero clue is equivalent to one with no clues, that is it
has no black squares in it. Pattern, however, doesn't interpret them
like this and treats a puzzle with a zero clue as insoluble, so it's
not helpful to permit them.
Permitting zero clues also confuses Pattern's memory allocation so
that it can suffer a buffer overrun. As an example, before this
commit a build with AddressSanitizer would report a buffer overrun
with the description "1:0/0.0" because it tries to put two clues in a
row that can have a maximum of one.
The offset and centre location should be within the grid. Otherwise the
redraw code will suffer an assertion failure. This save file
demonstrates the problem:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :3:Net
PARAMS :4:5x5w
CPARAMS :4:5x5w
DESC :25:9893e85285bb72e6de5182741
UI :9:O0,0;C6,6
NSTATES :1:1
STATEPOS:1:1
Some games would like a way to check that the parameters in the encoded
UI string are consistent with the game parameters. Since this might
depend on the current state of the game (this being what changed_state()
is for), implement this by adding a game_state parameter to decode_ui().
Nothing currently uses it, though Guess usefully could.
The majority of back-ends define encode_ui() to return NULL and
decode_ui() to do nothing. This commit allows them to instead specify
the relevant function pointers as NULL, in which case the mid-end won't
try to call them.
I'm planning to add a parameter to decode_ui(), and if I'm going to have
to touch every back-end's version of decode_ui(), I may as well ensure
that most of them never need to be touched again. And obviously
encode_ui() should go the same way for symmetry.
This avoids an out-of-range heap write shortly afterwards. An assertion
failure is better than a buffer overrun, but still not ideal. Fixing
the problem properly will require fairly wide-ranging changes, though.
The bug can be demonstrated by loading this save file into a build with
AddressSanitizer:
SAVEFILE:41:Simon Tatham's Portable Puzzle Collection
VERSION :1:1
GAME :3:Net
PARAMS :4:5x5w
CPARAMS :4:5x5w
DESC :25:9893e85285bb72e6de5182741
UI :9:O0,0;C6,6
NSTATES :1:1
STATEPOS:1:1
Emscripten has settings indicating which browser versions it should
build code for. These are now by default slightly newer than I'd been
targeting with my hand-written JavaScript. They also don't include
Firefox 48, which KaiOS 2.5 is based on.
This commit adds CMake variables to set the minimum versions that we
pass to Emscripten. They default to the earliest versions with
WebAssembly support, except that Firefox 48 is also supported.
I think the main consequence of this change is to stop Emscripten using
sign-extension and mutable-globals in WebAssembly, which it's done by
default since version 3.1.26.
Puzzles only runs in Web browsers, so there's no need to include
support for Node or (for now at least) running in a Web worker. This
removes about 5 kiB of code from the boilerplate JavaScript.
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 <tgmath.h> - but much more surprisingly, our _Windows_
builds failed too, with a compile error inside <tgmath.h> itself!
I haven't looked closely into the problem yet. Our Windows builds are
done with clang, which comes with its own <tgmath.h> 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 <tgmath.h> - 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 <tgmath.h> where possible and falling back to <math.h>
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.
It was originally used for probing the available warning flags in the
compiler, by adding them one by one and making sure this test file
still compiled.
But that whole mechanism was removed in commit 306fab356e357ef, and
since then, testbuild.c has been unused. Belatedly throw it away.
The KaiOS build includes compiled versions of various Emscripten library
files. These are generally under the MIT licence like Puzzles itself.
The MIT licence requires that the licence, and the copyright notice, be
"included in all copies or substantial portions of the Software."
Since each KaiOS package includes the full manual, which already
contains the licence for Puzzles itself, adding the copyright notices
there seems like the best approach. I've done this by providing an
additional input file that contains the licences for source files used
by a current Emscripten build. More automation might be nice, but the
set of copyright notices is unlikely to change very much. There are
basically one for Emscripten, one for musl, and a few for odd bits of
third-party code embedded in musl.
C89 provided only double-precision mathematical functions (sin() etc),
and so despite using single-precision elsewhere, those are what Puzzles
has traditionally used. C99 introduced single-precision equivalents
(sinf() etc), and I hope it's been long enough that we can safely use
them. Maybe they'll even be faster.
Rather than directly use the single-precision functions, though, we use
the magic macros from <tgmath.h> that automatically choose the precision
of mathematical functions based on their arguments. This has the
advantage that we only need to change which header we include, and thus
that we can switch back again if some platform has trouble with the new
header.
This is in place of the "loadend" event. In Chromium, (and in the
specification), "loadend" is triggered not only when the file is
loaded but also when loading fails. Obviously when loading fails we
don't want to try to parse the (nonexistent) resulting file.
Using the "load" event works better, since it's only fired on success,
and we can also have an "error" handler to report problems with
loading files, albeit with no detail at all.
This doesn't seem to make any difference in Firefox, which in my
testing fires "load" and "loadend" on success and nothing at all on
failure.
Before this commit, JavaScript Puzzles loaded a save file by pushing the
entire file onto the Emscripten stack and then reading it from there.
This worked tolerably for typical save files, but Emscripten's stack
defaults to only having 64 kiB of space. That meant that trying to load
something that wasn't a real save file tended to cause a stack overflow.
I expect that at least some real save files would suffer from the same
problem.
The stack overflow would generally cause a JavaScript exception and then
leave the stack pointer outside the stack, so that any future attempt to
call into C would fail as well.
To fix this, arrange that the C function for reading data from the save
file calls out to JavaScript. The JavaScript can then copy just the
requested data into the caller's buffer. We can't pass a JavaScript
function pointer to C, but since only one file can be loaded at a time,
we can just have a global variable that's the current loading callback.
There might still be a problem if you try to load a stupendously large
file, since I think FileReader.readAsArrayBuffer() reads the whole file
into the browser's RAM. It works on my laptop with files up to a few
hundred megabytes, though.
Using .readAsText() meant that trying to load a non-text file (for
instance something that's not a save file at all) would generate an
"RuntimeError: index out of bounds". This would then leave the
Emscripten runtime in a broken state.
It might even be possible for a real save file not to be valid UTF-8,
for instance if it came from a platform that used a different character
encoding for random seeds.
There's still a problem with opening very large files, apparently
because Emscripten tries to stuff the entire file onto the C stack.
That will probably have to be fixed by properly exposing the incremental
file-loading API to JavaScript.
This adds the ability to turn off hat-test's normal scaling of the
bounding box to fit on an A4 page, which I intended for printing test
patches (but never actually found a need to print one). The --unscaled
mode seems more useful if you're planning to turn the output into an
image, e.g. to use as a desktop background.
Also added --clip, which generates a rectangle completely covered in
hats (i.e. shows any hat that overlaps the output rectangle at all),
as opposed to the normal mode which omits any hat that doesn't fit
_entirely_ in the output rectangle (more similar to what Loopy wants).
Actually generating a desktop background by this method is still a bit
fiddly to get right, but it's better than before.
Having stated the principle in the previous commit, I should apply it
consistently. A source file linked into the Puzzles library of common
support code should not also define a main() under ifdef.
This commit only goes as far as the _library_ support modules. It
would be a much bigger job to do the same for all the actual _puzzles_
that have test main()s or standalone-solver main()s. And it's not
necessary, because modifying one of those source files only triggers a
rebuild of _one_ puzzle, not absolutely everything. (Not to mention
that it's quite likely the puzzle and the test main() will need to be
modified in conjunction anyway.)
As in the previous commit, this has required exposing a few internal
API functions as global, and maybe editing them a bit. In particular,
the one-shot internal function that divvy_rectangle() loops on until
it succeeds is now exposed as divvy_rectangle_attempt(), which means
the test program doesn't have to condition a failure counter into the
real function.
I've thrown away penrose-vector-test completely, because that didn't
look like a test program with any ongoing use at all - it was surely
vestigial, while James was getting the vector representation up and
running in the first place.
I noticed while hacking on hat-test recently that it's quite awkward
to be compiling a test main() program that lives in a source file also
built into the Puzzles support library, because every modification to
main() also triggers a rebuild of the library, and thence of all the
actual puzzles. So it's better if such a test main() has its own
source file.
In order to make hat-test work standalone, I've had to move a lot of
hat.c's internal declarations out into a second header file. This also
means making a bunch of internal functions global, which means they're
also in the namespace of programs other than hat-test, which means in
turn that they should have names with less implicit context.
