Instead of repeatedly reusing the file name 'PuzzleEngine.class' in
the main build directory, now each puzzle's NestedVM translation is
left in a separate subdirectory so that they don't collide with each
other. A bonus is that we don't have to rename the file back and forth
between the rule that builds it and the one that consumes it.
OS X is beginning to show a warning when a 32-bit application is
opened, so it's high time that this gets enabled. Fix a clang warning
exposed by this build.
This consists of setting a flag in the Info.plist. Everything in the
game is resolution-independent, of course, and has been thoroughly
tested on other platforms. The only issue I found is cosmetic: The
rounded window corners become more dramatic and don't look as good
against the square status bar.
Increasing the resolution also exposes two graphical quirks that I
don't think are new. Curved rails in Tracks seem to be made of short
segments that don't quite connect, but I don't see this on Android,
and on closer inspection this is already present on low resolution in
OS X. Lines in Untangle, and also Galaxies, that are at a multiple of
45 degrees seem thinner than other lines, but I also see this on
Android — I think it's just more obvious on high resolution, and could
be adjusted with antialiasing. Everything else looks as it should, for
example when moving a window between low and high dpi displays.
I've just upgraded my build machine to Ubuntu 18.04, which has come
with a version of javac that complains about both -source 1.3 and
-target 1.3. Both are surely pretty out of date anyway, so the path of
least resistance is to just increase them to the earliest version that
javac doesn't currently complain is deprecated.
If I want to rebuild just the Javascript puzzles (for example) in
circumstances where I don't expect to need a great many
edit-compile-link cycles, it's easier to get bob to do it for me than
to remember how to set up the development tools on my path. But it
takes ages to run the whole build script if I also have to wait for
the Windows, Mac and Java puzzles to be built, not to mention the
initial Unix build that runs for no purpose other than generating the
icon images.
So now I can run the build with various time-consuming parts
conditioned out, for development purposes. Of course, the default is
still to build absolutely everything.
Oops! This was left over from an early development version of commits
4408476b7 and 000ebc507, in which I initially arranged for each
adjacency list to be terminated with the sentinel value -1 instead of
separately storing an array of the lists' lengths.
I later changed the representation to make randomising the algorithm
easier (it's much easier to shuffle an array uniformly at random if
you _don't_ have to faff endlessly to work out its length). But this
write of a no-longer- needed sentinel value in the client code must
have survived the rewrite by mistake, and also somehow evaded all my
pre-commit testing with valgrind and asan.
A user reported that the Towers Javascript version was crashing on
startup, and I think this is the cause, because it seems to fix it for
me.
Checking with the standards, I think this is legal C99, but not legal
C89 - and we are compiling in C89 mode. Why _every_ version of gcc
didn't object, given all the warning and pedantry options, I'm not
sure, but one did, so I should fix it.
This function gives the front end a way to find out what keys the back
end requires; and as such it is mostly useful for ports without a
keyboard. It is based on changes originally found in Chris Boyle's
Android port, though some modifications were needed to make it more
flexible.
Its ability to solve general network flow problems was never actually
used in this code base; it was _always_ used for the restricted
problem of finding a matching in an unweighted bipartite graph. So now
I've switched all its clients over to the new matching.c, maxflow is
no longer needed.
Tents needs to construct maximal matchings in two different
situations. One is the completion check during play, in which the
existence of a perfect matching between tents and trees is part of the
win condition; the other is the initial grid generation, in which we
find a _maximal_ matching between the tents we've already placed and
all the possible neighbouring squares that are candidates for the tree
positions. Both of those are switched over.
The new client code is a lot smaller and nicer, because we can throw
away the col[] and num[] permutations entirely.
Of course, this also means that every puzzle that incorporated latin.c
now has to link against matching.c instead of maxflow.c - but since I
centralised that secondary dependency into Recipe a few commits ago,
it's trivial to switch them all over at once.
This is a dedicated algorithm for finding a maximal matching in a
bipartite graph.
Previously I've been solving that problem in this code base using
maxflow.c, modelling the graph matching problem as a restricted case
of the optimal network flow problem and then using a full-strength
algorithm for the latter. That's overkill, and also algorithmically
wasteful - the H-K algorithm is asymptotically better, because it can
find multiple augmenting paths in each pass (hence getting the maximum
benefit out of each expensive breadth-first search), as a consequence
of not having to worry about lower- or higher-value augmenting paths
in this restricted problem.
So I expect this algorithm to be faster, at least in principle or in
large cases, when it takes over the jobs currently being done by
maxflow. But that's not the only benefit. Another is that the data
representation is better suited to the problems actually being solved,
which should simplify all the call sites; a third is that I've
incorporated randomisation of the generated matching into the H-K
module itself, which will further save effort at each call site
because they will no longer have to worry about permuting the
algorithm's input - they just have to pass it a random_state and it
will take care of that internally.
This commit only introduces the new matching.c and builds a test
utility for it. I haven't yet migrated any clients of maxflow.
It's silly to have every puzzle using latin.c separately specify in
its .R file the list of additional modules that latin.c depends on, or
for that matter to have them all have to separately know how to adjust
that for the STANDALONE_SOLVER mode of latin.c.
So I've centralised a new pair of definitions into the core Recipe
file, called LATIN and LATIN_SOLVER, and now a client of latin.c need
only ask for that to get all the necessary dependencies too.
Also, while I'm here, I've moved the non-puzzle-specific 'latincheck'
test program out of unequal.R into the central Recipe.
A user mailed me today having found it less than clear from the docs
that Galaxies will only accept a solution if the set of filled-in grid
edges consists of _exactly_ the ones that separate two distinct
regions, rather than consisting of _at least_ those and perhaps others
which neither break rotational symmetry or disconnect any region.
Firstly, it tries to use newstate after freeing it. I haven't come up
with any way of actually triggering that bug. I suppose you'd need a
grid that required recursion to solve, which the generator of course
does not normally create.
Secondly, it stores M_BLACK or M_WHITE in the grid before doing the
recursion, whereas it should store BLACK or WHITE. I believe that since
it tries M_BLACK first, which is zero, and since it's trying it on an
undecided square (also zero), this leads to infinite recursion, and an
eventual crash once you run out of stack space.
You can (sometimes) trigger this by asking for a hint on a grid where
you've already made a mistake.
e.g. on the puzzle desc below there's a "9" at (13,7). If you place a
black tile to its immediate left and right - (12,7) and (14,7) - then
press 'h', you get a beachball and then crash (on macOS at least - I
presume other OSes are similar).
15x15:i5g4d16a7a7c3b3b11f11_15d3p8b7_8b4h18c4d13b4i7a16k9a9i4b2d10c14h11b10_10b17p6d8_7f9b8b11c10a2a5d5g7i
Michael Quevillon points out that neither 2x1 nor 3x1 Solo can be made
into an X Sudoku puzzle, on the grounds that whatever number goes in
one corner of the grid is ruled out from both ends (and the centre, if
any) of the opposing diagonal, and hence the X constraint can't be
satisfied.
(Also fixed a spurious full stop on a neighbouring line.)
Spotted by Michael Quevillon. After checking the set of digits
appearing in one of the two main grid diagonals, we weren't clearing
the used[] array before using it to check the same thing about the
other diagonal. So if the first diagonal we check has one of
everything, then the second will be misidentified as correct (for the
purposes of game-completion detection, although not error
highlighting) even if it doesn't have one of everything.
The branch of the code that claimed the puzzle to be ambiguous was not
also re-running the solver with diagnostics enabled, so that if a user
tries to use this tool to hand-design a puzzle, they do not get
feedback on what the multiple legal solutions actually are.
In solver_show_working mode, we were logging all the deductions,
guesswork and backtracking, but not printing out the actual solution
(if any) reached at the end of each branch of the tree.
This is another situation in which the midend's state, at the time
midend_new_game tries to save it, is not such as to generate a viable
serialisation. In this case, it's because after the user enters a game
id into the Game > Specific or Game > Random Seed dialog box,
midend_set_config will have _already_ started overwriting important
fields of the midend such as me->desc and me->seed, before
midend_new_game is even entered.
In fact this caused an assertion failure (thanks to Lennard Sprong for
reporting it), because one of those fields was set to NULL, so the
resulting serialisation buffer was incomplete, leading to a
deserialisation error later. But I was lucky: if I hadn't been, the
serialisation might have been complete, and valid according to the
deserialisation code, but would have contained a mixture of the new
game's description and the old one's move chain, which would surely
have had far stranger results.
For the moment, I'm fixing this by simply not storing a serialisation
in that situation. Perhaps a nicer approach might be to store one
before starting to overwrite midend fields, and then have
midend_new_game swap it in if it's already been generated. That way
you _could_ still undo past an action like this. But preventing the
assertion failure is a good start.
Now, when we undo a New Game by deserialising the stored version of
the previous game, we start by serialising the _current_ game into a
second serialisation buffer in the midend. Then, if the user tries to
redo back past that undo action, we can re-serialise the earlier game
and re-deserialise the newer one.
A few users had complained (with various degrees of politeness) at the
lack of this ability, because in true xkcd #1172 style, it broke their
workflow. Specifically, if you were a fan of holding down 'u' to undo
all the way back to the start of your current game, you'd have
overshot into the previous game and then have no way to return to the
game you wanted to be at the start of.
This slightly changes the previous interaction of Redo with New Game.
Previously, New Game would save the entire undo chain of the
serialised game, including anything forward of the current position;
so if you took actions move1,move2,undo,newgame then the serialised
game state would include both move1 and move2 with the current
position between them; hence, an undo would restore the old game to a
position just after move1, and then a redo action would re-enact
move2. Now, midend_purge_states is called before serialising the old
game, so in that scenario move2 would be completely lost as a side
effect of the new-game action. (Just as it would be if you made any
_other_ undoable move in that situation.)
Conversely, making a move in the old game after you've undone back
into it will now wipe out the record of the later game, so you can't
redo into it any more.
The three related buf/len/size fields are now a sub-structure of the
main midend, and the serialise/deserialise functions that address
those fields now take a pointer to that sub-structure rather than to
the midend as a whole. This will make it easy for me to drop in a
second substructure alongside it, for redo, and reuse those read and
write functions by just changing the context pointer.
Editing LICENCE just now, I happened to notice that the accented
letter in Jonas Kölker's name was encoded in ISO 8859-1, as is the
occurrence of the same name in filling.c - but _not_ the one in
guess.c, which was in UTF-8 already. That seems needlessly confusing,
so let's sort it out. Now every text file in this git repository is
suitable for interpreting as UTF-8.
Regular hexagons and equilateral triangles in strict alternation, with
two of each interleaved around each vertex.
https://en.wikipedia.org/wiki/Trihexagonal_tiling
Thanks to Michael Quevillon for the patch.
There were a couple of places where we enforced by assertion that our
solver state had not become inconsistent, on the assumption that some
previous piece of solver would have detected that the puzzle was
impossible before getting to that place in the code.
But in fact the combination of Killer and Unreasonable modes falsified
at least two of those assumptions: 'solo --test-solve --generate 100
3x3kdu#12345' triggered an assertion failure in solver_set, and with
that one fixed, the same command triggered a second failure in
solver_killer_sums.
In both cases, the fix is simply to return -1 for 'puzzle is
inconsistent', which will cause the Unreasonable recursive solver to
abandon that branch of its search tree, backtrack, and try a different
guess at some previous square.
Thanks to Anders Höglund for the report.
The fields (ui->dragx,ui->dragy) stored the pixel coordinates of the
visible cursor (if any), no matter whether that cursor was being
displayed in response to a mouse dragging action or arrow-key
activity. But this meant that resizing the window while the keyboard
cursor was visible would cause the cursor to be drawn in totally the
wrong place in the newly resized window: you get a new drawstate with
a fresh blitter (so at least no part of the old-size window is
accidentally 'restored' on to the new-size one), but the ui fields
saying where _next_ to draw the cursor would still have bogus values
left over from the previous window size.
To fix this, I've arranged that we simply don't use ui->dragx and
ui->dragy any more in keyboard cursor mode: instead, we leave it to
game_redraw to spot that the keyboard cursor is visible and compute
its pixel coordinates for display.
A knock-on effect is that when we need to know which region is under
the cursor during interpret_move, we can't use the previous strategy
of just calling region_from_coords(ui->dragx, ui->dragy) regardless of
which kind of cursor is active. So I've split that function up into an
inner section taking a tile and a displacement from the tile's centre
and an outer part which derives those from real pixel coordinates in
the case where the cursor is originating from a mouse drag; then
there's a new alternative outer part which derives the same (tile,
displacement) pair purely from the game_ui keyboard cursor data
without having to explicitly translate into pixels and back in the
middle. Probably a less fragile strategy anyway.
I had left one mention of the new GTK3-only variable
'awaiting_resize_ack' unprotected by #ifdefs. Also, the GTK2 version
of message_box() was missing some consts in its prototype, presumably
because when I had that #ifdeffed out the compiler didn't warn me
about those ones.
A user sent in the game id
7:0,0L,0,0,0L,0L,1,0U,0U,0D,0UD,0,0,7,0,0D,0,0D,0,0,0,0,0,0,0,0,0,0D,0,0,3,0,0U,0,0,0,0,2,6,0,0U,0,0,0,7,2,0,0U,5L,
starting from which, one press of 'h' will fill in a 2 in the top left
corner and possibilities 345 to its right; if you then fill in 5 there
and press 'h' again, the hint system fills in the whole of an apparent
solution which isn't the one a proper use of the Solve feature would
give you - except that it's not actually a second solution, because
one < clue on the top row is violated. Without this fix, that < failed
to light up red, making the puzzle look ambiguous if you're not paying
enough attention to spot it.
prev[sink] == 0 means there was a path found with the last step being a
forward edge to the sink, and the edge being at index 0 in the array.
This is a valid path (the same as any other path indicated by prev[sink]
> 0).
The initial call to midend_new_game() was creating a partial
serialisation containing no game states at all, which meant that if
your first UI action was an undo operation, the game would try to
deserialise that and complain that it was incomplete. Now we detect
that in advance and don't create a useless serialisation in the first
place.
I went through all the char * parameters and return values I could see
in puzzles.h by eye and spotted ones that surely ought to have been
const all along.
This allows me to use different types for the mutable, dynamically
allocated string value in a C_STRING control and the fixed constant
list of option names in a C_CHOICES.
Now midend.c directly tests the returned pointer for equality to this
value, instead of checking whether it's the empty string.
A minor effect of this is that games may now return a dynamically
allocated empty string from interpret_move() and treat it as just
another legal move description. But I don't expect anyone to be
perverse enough to actually do that! The main purpose is that it
avoids returning a string literal from a function whose return type is
a pointer to _non-const_ char, i.e. we are now one step closer to
being able to make this code base clean under -Wwrite-strings.
If I increase clang-cl's warning pickiness, it starts objecting to my
cast to HMENU from a (potentially, in 64 bits) smaller integer type.
Actually I don't think there's a problem there - all the integer ids I
cast to HMENU are nice small numbers and a widening cast is just fine.
But I can suppress the warning by using INT_PTR instead of int in the
prototype for mkctrl, so it's easiest just to do that.
The newgame_undo data was being saved on every call to
midend_new_game, including the one just after midend_set_params when a
new puzzle preset was selected. So you could select a new preset from
the menu, and then press 'u', and the midend would _try_ to undo that
operation and restore the previous game with a different set of
parameters.
This would do the wrong thing in the front end, because front ends in
general will not be expecting that a change of game parameters might
result from an arbitrary keyboard event - they won't be expecting to
have to call the function that moves the highlight in the game-type
menu, for example, and they _certainly_ won't be expecting that a
window resize might be necessary in response to a random keystroke.
One possible response would be to fix all the front ends so that they
_are_ prepared for either of those consequences of a keystroke event,
and then it would be possible to undo not only the New Game menu
option and the 'n' key but also undo any selection of a preset from
the game-type menu, or even a full re-customisation of the game
settings. But that would be quite an upheaval even in _my_ front end
collection, and also probably be awkward for downstream front ends, so
until I'm convinced of the value of going to all the effort, the
simpler approach is just to disallow undoing a new game in those
situations.
(This does mean that re-selecting the _already active_ game preset
from the type menu will be treated as an undoable new-game event,
which I think is an acceptable UI oddity.)
I've renamed the new midend variables to match my usual naming
convention of using 'size' for the total buffer size and 'len' for the
amount of currently used space (and a couple of other variables to
match those in turn), partly for consistency and also because the name
'midend_undo_used' made me half-expect it to be a boolean. The buffer
itself is called 'midend_undo_buf', again to avoid making it sound
like a function or flag.
Buffer growth is still geometric but less aggressive (factor of 5/4
each time instead of 2), in the hope of wasting less memory on low-RAM
platforms; newgame_undo_deserialise_read should have been static, and
now is; newgame_undo_buf is initialised using NULL rather than 0 so it
doesn't look like an integer, and is freed with the rest of the
midend.
And I think we _should_ enforce by assertion that midend_deserialise
didn't return an error, because there's no reason it ever should in
this situation (unlike when reading from a file, where the user might
have provided the wrong file or a corrupted one). This immediately
allowed me to spot a bug in the existing deserialisation code :-)
It is annoying when one intends to choose "restart" and chooses "new
game" instead. Right now, the puzzle one wanted to try again is
discarded.
To fix this we are going to have to save a lot more information than a
normal game state. Handily, we already have the serialise/deserialise
machinery.
The advantage of using this is that the previous game is easily saved
in its entirety, including its own undo history, and also probably in
a more compact format.
The (de)serialisation interface is rather clunky for use with a memory
target. Sadly none of the existing implementations of a resizing
memory array have been conveniently brought out into puzzles.h, and I
think that that's beyond the scope of what I wanted to do here.
We don't serialise the new game undo serialisation data. So
loading/saving doesn't preserve any "new game" undo, as does "new
game" twice (and as does context switching on a Palm Pilot).
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
