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>
This will let me do a 'conditional deserialisation' operation, in
which we fully decode the serialised data and then (assuming that gave
no errors) decide whether or not to actually install it based on some
arbitrary condition.
I don't think there's any possible use for the extra check function
_outside_ midend.c, so I've left the API for front ends as it is; the
externally visible midend_deserialise() doesn't have the new
parameter, and only midend_deserialise_internal() includes it.
Lots of the local variables in midend_deserialise are now fields of a
structure which contains everything that is _going_ to be written into
the midend once we finish validating it all. This makes it easy to
keep all that data together, and (in future) pass it to other
functions all in one go.
No functional change.
The serialised game stores a long-term and a short-term parameter
structure, which correspond to me->params (the thing that gets used by
the next New Game command) and me->curparams (the thing that _was_
used to generate _this_ game). So data relevant to the current game
ought to be validated against the latter, but in fact I was
accidentally passing the former to several validation calls.
I think this probably avoided causing a problem because typically
params and cparams don't differ very much: the usual reason why
they're not the same is that somebody has manually entered a game
description involving an incomplete description of the parameters
(lacking generation-specific details like difficulty level), but by
the very fact that those incomplete descriptions have to contain
_enough_ information to understand a specific game description,
copying just those parts of the description into the long-term params
structure makes the two similar enough that validation won't fail.
However, testing an upcoming patch which calls midend_deserialise at a
more difficult moment (specifically, just after midend_set_params,
meaning that the two params structures can now differ _arbitrarily_)
reveals my error. Fixed to use cparams where that's the right thing.
This makes it much easier to see the commonality in these formulaic
lines.
Whitespace change only.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
While working on the Net scalability today I noticed that changing
preset from (say) 13x11 to 5x5 in GTK3 Net while the window is
maximised does not have the desired effect (that being that, since the
maximised window does not change size, the new puzzle size is instead
scaled to fit neatly in the existing window).
A git bisect suggests that this was a side effect of commit 8dfe5cec3;
it looks as if there was a useful side effect of setting fe->area as
the 'geometry widget' for fe->window, namely, that any attempt to
resize the window thereafter (even if it had no effect on the window
size) would trigger a configure event on the geometry widget, so we'd
get a notification of our new size even if it was the same as our old
size.
But that 'geometry widget' feature is deprecated, so I have to work
around it another way. Fortunately, I've found a fallback event that
still does occur, namely "size_allocate" on fe->window. So I'm
trapping that as well and using it as an indication that a configure
event won't be forthcoming.
In commit a7dc17c42 I apparently introduced two bugs in
changed_preset(). Firstly, the Custom menu option was being written
into the 'found' variable in nearly all cases, because it has a NULL
user-data pointer which caused it to take the wrong branch of an if
statement due to an erroneous complex condition. Secondly, having
written _something_ into 'found', I had set it to inactive rather than
active due to forgetting to change a FALSE into a TRUE.
Now when I start up Net with my usual nonstandard default parameters
(I like the 13x11 wrapping, so I set NET_DEFAULT=13x11w in my
environment), the right menu entry comes up ticked.
Previously, the network, grid edges and barriers were all drawn with
fixed line thickness of one pixel, or three pixels in the case of
wires (one central one in a lit-up colour and a black border pixel on
each side). This worked badly on high-DPI displays, or in any other
environment where you expand the window noticeably.
I've tried a few times before to fix this, but the problem was always
that the Net drawing code was complicated and confusing, because Net
was one of the founding puzzles in this collection and its drawing
code predated a lot of the sensible organisation and best-practice
doctrines I've come up with since then and used in the later puzzles.
(The best example of this was the way that redrawing a grid square
always redrew _all_ the surrounding borders, which is very confusing
in the light of my later practice of dividing up the grid into
disjoint regions that are always redrawn with clipping.) It was hard
to make any change to the Net graphics with the code in that state; I
tried several times and decided I couldn't sensibly make it work
without throwing it all away and rewriting from scratch, which I was
always reluctant to do.
But not any more! Since Ian sent some patches to improve the graphics
scaling in Tracks, and since Net was at the top of my personal
wishlist of games that needed the same treatment, I've decided it's
time to do just that.
So, this commit throws out all of Net's previous redraw code, and
replaces it with something in the more modern style I usually adopt in
new puzzles. The new draw_tile() doesn't read data out of the game
state willy-nilly; instead it takes a single word of bit-flags
describing everything about the tile it's drawing, and makes all its
decisions based on that word. And the main redraw loop constructs a
whole array of flags words describing what it would _like_ the grid to
look like, and then calls draw_tile() for every square whose word
doesn't match the one that was previously drawn there. (With the one
exception that the presence of the TILE_ROTATING flag in either the
old or new word forces a redraw _even_ if the two words are identical,
because that's how the move animation works.)
The new graphics look more or less the same at the default resolution
(there may be a pixel difference here or there but I don't think it's
noticeable if so), but now they scale up properly if you enlarge or
maximise the puzzle window.
Net is one of the very oldest puzzles in the collection, and has
apparently been physically copying the complete collection of totally
immutable barrier data in every game state since 2004. About time it
stopped, I think!
Since these lines are always orthogonal, it's easier to draw them
using draw_rect than draw_thick_line.
Previously, the grid lines were drawn just inside the top and left
edges of the region redrawn by draw_square(), so only the bottom and
right edges of the whole grid were not covered by any draw_square
call. To avoid having to shift the logical grid centre, I'm now
drawing parts of the grid outline on all four sides of the
draw_square() region, so that half the thickened grid edge protrudes
on every side of the grid as a whole.
In the process of splitting up the grid line width into the part on
the top and left edges and the part on the bottom and right, I've
renamed the confusingly named BORDER_WIDTH define (which wasn't used
anyway) to a set of things that make it clear that they're referring
to the grid lines as opposed to the border of the whole puzzle.
The default of 1/30 is rather thin, and probably wasn't chosen
deliberately (since it was just inherited from the default 1-pixel
line width, and the preferred tile size).
Thicker crosses stand out more and make play easier.
Use 1/16 since it's a rounder number than 1/15 :-).
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
Simply replace the calls to draw_line with calls to draw_thick_line.
We need to choose a thickness parameter. The preferred tile size is
30, and the "draw_line" function draws a 1-pixel line, so the
thickness right now is 1/30 the tile size, at the preferred size.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
A line less than 1 pixel wide may not be visible. So if a backend
wants to draw a line whose width scaled by the window size, that line
thickness ought to be at least 1.0.
That way if the scale is small, but still big enough that there is a
straightforward interpretation of the drawing primitives which is
legible, we implement that interpretation.
If a frontend draws a narrower line, making it wider might cause
drawing anomalies, due to the line now having a bigger bounding box.
These anomalies should occur only at small scales where currently the
display is not legible, and we should fix them as we notice them.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
gcc objects to this in -pedantic mode, which means other compilers are
technically entitled to object too if they like. Usually I try to
avoid it by putting a dummy value at the end of the enum, but I forgot
in this case.
(And I didn't notice, because _my_ local builds run without -pedantic,
on the grounds that configure.ac autodetects that my system's GTK
headers are not pedantic-clean. Oh well.)
Now, with an odd grid size, we choose the posterisation threshold so
that half the time it delivers ceil(n/2) black squares and half the
time it delivers floor(n/2). Previously it only did the former, which
meant that asking Pattern to generate a 1x1 puzzle (with the bug in
the previous commit fixed) would always generate the one with a single
black square, and never the one with a single white square. Both are
trivial to solve, of course, but it seemed inelegant!
No change to the number of black squares in the puzzle solution can
constitute a spoiler for the player, of course, because that number is
trivial to determine without doing any difficult reasoning, just by
adding up all the clues in one dimension.
We were filling in a row immediately as all-white if it had no clues
at all, but weren't filling in a row as all-black if it had a single
clue covering the entire row. Now we do both.
In particular, this caused the Pattern solver to be unable to take
advantage of one of the two kinds of totally obvious clue across the
_easy_ dimension of a trivial 1xN puzzle - and a special case of
_that_, as a user pointed out, is that the game generator hangs trying
to create a 1x1 puzzle, which ought to be the easiest thing in the
world!
This should make it less annoying for me to do local testing of the JS
output of a build, before I push a change. There's a new
build.out/jstest directory containing .html files suitable for loading
in a local browser, which refer to the JS files via an appropriate
relative path to the existing build.out/js directory.
This is in addition to the existing keystrokes r, ^R and ^Y. I've
become used to Ctrl-Shift-Z in other GUI games, and my fingers keep
getting confused when my own puzzles don't handle it the same way.
This fixes an amusing UI bug that I think can currently only come up
in the unpublished puzzle 'Group', but there's no reason why other
puzzles _couldn't_ do the thing that triggers the bug, if they wanted
to.
Group has unusual keyboard handling, in that sometimes (when a cell is
selected for input and the key in question is valid for the current
puzzle size) the game's interpret_move function will eat keystrokes
like 'n' and 'u' that would otherwise trigger special UI events like
New Game or Undo.
The bug is that fake keypress events generated from the GUI menus
looked enough like those keystrokes that interpret_move would eat
those too. So if you start, say, a 16x16 Group puzzle, select an empty
cell, and then choose 'new game' from the menu, Group will enter 'n'
into the cell instead of starting a new game!
I've fixed this by inventing a new set of special keystroke values
called things like UI_NEWGAME and UI_UNDO, and having the GUI menus in
all my front ends generate those in place of 'n' and 'u'. So now the
midend can tell the difference between 'n' on the keyboard and New
Game from the menu, and so Group can treat them differently too. In
fact, out of sheer overcaution, midend.c will spot keystrokes in this
range and not even _pass_ them to the game back end, so Group
shouldn't be able to override these special events even by mistake.
One fiddly consequence is that in gtk.c I've had to rethink the menu
accelerator system. I was adding visible menu accelerators to a few
menu items, so that (for example) 'U' and 'R' showed up to the right
of Undo and Redo in the menu. Of course this had the side effect of
making them real functioning accelerators from GTK's point of view,
which activate the menu item in the same way as usual, causing it to
send whatever keystroke the menu item generates. In other words,
whenever I entered 'n' into a cell in a large Group game, this was the
route followed by even a normal 'n' originated from a real keystroke -
it activated the New Game menu item by mistake, which would then send
'n' by mistake instead of starting a new game!
Those mistakes cancelled each other out, but now I've fixed the
latter, I've had to fix the former too or else the GTK front end would
now undo all of this good work, by _always_ translating 'n' on the
keyboard to UI_NEWGAME, even if the puzzle would have wanted to treat
a real press of 'n' differently. So I've fixed _that_ in turn by
putting those menu accelerators in a GtkAccelGroup that is never
actually enabled on the main window, so the accelerator keys will be
displayed in the menu but not processed by GTK's keyboard handling.
(Also, while I was redoing this code, I've removed the logic in
add_menu_item_with_key that reverse-engineered an ASCII value into
Control and Shift modifiers plus a base key, because the only
arguments to that function were fixed at compile time anyway so it's
easier to just write the results of that conversion directly into the
call sites; and I've added the GTK_ACCEL_LOCKED flag, in recognition
of the fact that _because_ these accelerators are processed by a weird
mechanism, they cannot be dynamically reconfigured by users and
actually work afterwards.)
That's a case in which the current game IDs have changed, so the
midend ought to be calling the front-end function (if any) that
notifies it when that happens.
The only front end of mine that was affected by this missing call was
the Javascript one, which uses that callback to update the 'Link to
this puzzle' links below the game canvas - but, of course, that front
end didn't ever call midend_deserialise until this month, so no wonder
I never noticed before.
(But downstream front ends might be affected too, for all I know.)
Commit ef39e6e17 made a goof in which the 'New game' button had no
border on the left and an accidental extra one on the right, which I'm
really not sure how I failed to spot when I tested it yesterday.
The only user to send me a comment today on the new layout said that
that menu item in particular is annoying to have hidden behind more
clicks, so by a vote of one to nothing, it's back out in the open.
I've been thinking for a while that it's about time I did that. Those
images used to link to the Java versions of the puzzles, back when
Java was the in-browser applet platform of choice; then I made them
not link to either one when it wasn't clear which system would win;
but now JS has clearly won, it's past time the images linked to the JS
puzzles, as the obviously sensible default.
This is done by showing a dialog containing an <input type="file">
through which the user can 'upload' a save file - though, of course,
the 'upload' doesn't go to any HTTP server, but only into the mind of
the Javascript running in the same browser.
It would be even nicer to support drag-and-drop as an alternative UI
for getting the save file into the browser, but that isn't critical to
getting the first version of this feature out of the door.
This is done by getting midend_serialise to produce the complete
saved-game file as an in-memory string buffer, and then encoding that
into a data: URI which we provide to the user as a hyperlink in a
dialog box. The hyperlink has the 'download' attribute, which means
clicking on it should automatically offer to save the file, and also
lets me specify a not-too-silly default file name.
I'm about to want to call these from Javascript as well as from
Emscripten-compiled C, so I need versions of them that aren't wrapped
up in the Emscripten library object and also don't expect half their
parameters to be Emscripten-verse char pointers requiring calls to
Pointer_stringify.
The easiest way to achieve all of that is to turn the Emscripten-
ready versions of those functions in emcclib.js into tiny wrappers
around the JS versions, which do the pointer stringification and a
couple of other details like directing callbacks to the right C
functions.
I'm about to introduce a few more options, and the button bar is
already a bit wide, so I think I should shrink it horizontally before
putting more stuff on it. So I've organised the options into something
more like the Game and Type submenus that the desktop versions use.
However, I haven't gone quite all the way, on the basis that the web
versions will be at least slightly playable on devices without much
keyboard, which makes it worth keeping the in-play actions (Undo,
Redo, and to a lesser extent Restart and Solve) accessible as
top-level buttons in a single click each.
As part of this change, I've also separated the menu bar into a
drop-down menus section and a top-level buttons section with a gap
between them, and put a signalling "..." on the end of the titles in
the former section.
This change also removes the class="left" on the game-type menu and
its submenus, which were previously there to deal with that menu being
all the way over on the far right of the menu bar. But the CSS for
those classes is still there in jspage.pl, and should still work if I
need it again in future.
This is intended to make life easier for the _really_ dense grids in
which the generator algorithm falls back to a bare clearing, a tightly
packed section round the edges, and a fringe of deductions required in
between.
In that situation, you can deduce _in principle_ from the remaining-
mines counter that there are (say) one, or two, squares left to be
uncovered before everything remaining has to be a mine. And often the
game will require that deduction in order to solve it all by pure
logic. But actually doing it requires counting up the huge number of
covered squares in an irregularly shaped area and subtracting the mine
count in the status line, which is a real pain.
In fact, people failing to do that are the biggest source of (wrong)
bug reports about Mines games having no solution, so with any luck
this will make my own life easier.
This also switches them to being 64-bit, which I think is
probably acceptable in this modern age, especially for such
a non-essential piece of software. If anyone complains I can
always reinstate a parallel 32-bit build.
To support the switch to 64-bit, this commit also changes the default
install directory in the MSI to 'Program Files' rather than 'Program
Files (x86)'.
Mostly just cribbed from the corresponding changes in PuTTY's
build setup, although since the two mkfile.pl scripts are not
_quite_ identical, I had to make a few tweaks.
I'm getting rid of these installers in general, and also I'm about to
switch the Windows builds over to my new-look non-Windows non-Wine
system, which can't run the Inno Setup builder anyway.
The solver code still had an assumption, which must have dated before
the Solve menu option was introduced, that all puzzles presented to it
had at least one valid solution, and was enforcing that assumption by
assert(). Now the solver returns a more sensible failure code which
solve_game() can convert into a proper error message.
