As far as I could tell, cur_x, cur_y, prev_cur_x, and prev_cur_y in
game_drawstate were never used. prev_cur_x and prev_cur_y in game_ui
were assigned but never referenced. So they may as well all go.
It looks as if it's been broken for about nine years, ever since
commit 822243de1bc1fc6d introduced the system for drag-selecting a
diagonal of squares. The effect of moving the keyboard cursor and then
pressing a button was to cause crashes because the ui fields
introduced for that system to use (ohx, ohy, odx, ody, odn) were all
completely uninitialised.
If you're using the mouse to change pencil marks, you have to
right-click to pencil-highlight a square, then press a number or
letter key to add or remove a highlight. That causes the highlight to
vanish again. So adding or removing multiple pencil marks requires a
right-click + keypress per mark.
Chris's Android port reversed that decision, making the pencil
highlight persist so that you could 'click' just once and then press
multiple pencil keys. That makes it easier to add lots of highlights,
but harder to just remove a single one (click + press + click to
remove the highlight), unless you don't mind keeping the highlight
around afterwards cluttering up your view.
In other words, this is just the sort of thing users might reasonably
disagree on. So now we have an organised preferences system, we can
let them disagree, and each configure it whichever way they like!
This only affects mouse-based play. The keyboard cursor has _always_
worked this way, because it doesn't disappear at all; its behaviour is
unchanged, and independent of the new preference.
Light Up is unusual in that clicking outside the grid hides the
cursor, so the return value from clicks outside the grid is
MOVE_NO_EFFECT or MOVE_UI_UPDATE rather than the more usual
MOVE_UNUSED.
This adds an extra parameter to move_cursor() that's an optional pointer
to a bool indicating whether the cursor is visible. This allows for
centralising the common idiom of having the keyboard cursor become
visible when a cursor key is pressed. Consistently with the vast
majority of existing puzzles, the cursor moves even if it was invisible
before, and becomes visible even if it can't move.
The function now also returns one of the special constants that can be
returned by interpret_move(), so that the caller can correctly return
MOVE_UI_UPDATE or MOVE_NO_EFFECT without needing to carefully check for
changes itself.
Callers are updated only to the extent that they all pass NULL as the
new argument. Most of them could now be substantially simplified.
This means that it now potentially overlaps the peg above it (part of
the current guess), rather than potentially overlapping the empty hole
below. More importantly, it means that the hold marker is erased by
the erasure of the rest of the peg area, so there's no need to
explicitly draw absent hold markers in the background colour. That in
turn means that absent hold markers don't nibble the tops off all the
pegs at some tile sizes.
Instead of this fix, I could have properly made the hold markers part
of the first row of empty holes, but that would have been rather
fiddly and I've long thought that the hold markers were too far from
the peg that they're holding.
I've also removed part of a comment about the drawing order of hold
markers that seems to have been obsolete even before this commit.
Pearl generally has to generate quite a lot of candidate loops before
it can find one that makes a viable puzzle. Before this change it
generated a new grid structure for each of those candidate loops. The
result was that grid_new() accounted for over 5% of the
puzzle-generation time.
Pulling grid_new() out of the loop-generation loop makes "pearl
--generate 100 8x8dt#0" about 6% faster on my laptop, while producing
precisely the same output. Most of this change is just renaming the
"grid" variable in new_clues() so it doesn't collide with the typedef
of the same name.
Same Game doesn't want to show the keyboard cursor when the game is in a
state where no move is possible. Previously, it did this by having
game_changed_state() hide the cursor on entry to such a state. That
meant that reaching a dead end and undoing out of it hid the cursor,
which was confusing.
Now the cursor is hidden in game_redraw() if the game is in a dead-end
state without changing the displaysel flag in the game_ui. That way, if
you undo out of a dead end, the cursor becomes visible again if it was
visible before.
This does mean that you can move the cursor in a dead-end state without
being able to see where it's going. I think that's tolerable, but maybe
the cursor keys should be disabled in that state as well.
TILE_GAP represents the gap between the coloured parts of adjacent
different-coloured tiles. Rather than a fixed 2 pixels, it's now 1/16
of the tilesize (rounded to nearest).
This looks the same at the default tilesize of 32, but behaves better
with larger or smaller tilesizes. At tilesizes below 8, the gap
disappears altogether, but that seems appropriate: at 7 pixels there's
not much space for the inner and outer squares and the cursor, and those
are all more important than the gap..
Flood's draw_tile() extravagantly uses up to eight rectangles to draw
separators around each tile. This could be substantially improved,
but a particularly low-hanging optimisation is not do draw them when
the separator width is zero.
This at least means that Flood completes its initial drawing on my
test KaiOS device.
I've rewritten tile_redraw to reduce the number of calls to
draw_rect(). Before, it would generally make five calls to
draw_rect() when drawing a tile. Now it makes at most three, and
usually two. That's one draw_rect() for each colour that appears in
the tile, which is as good as it can get. This reduces the time to
draw a large puzzle by about 35% on Firefox 102.
This is of significance to me because CanvasRenderingContext2D on my
test KaiOS device seems to have a limit on the number of fill() and
fillRect() calls that it will tolerate in a short time. This means
that if you issue more than 1024 fillRect() calls in rapid succession,
the later ones are simply ignored.
Same Game's largest preset called draw_rect() so much that it hit this
limit. That meant that the right-hand side of the grid didn't get
properly drawn when starting a new game. Now that it is less
profligate with draw_rect() it fits comfortably within the limit and I
get to see the entire grid.
The drawing routines in JavaScript used to take pointers to a C string
containing a CSS colour name. That meant that JavaScript had to create
a new JavaScript string on ever call to a drawing function, which seemed
ugly.
So now we instead pass colour numbers all the way down into JavaScript
and keep an array of JavaScript strings there that can be re-used. The
conversion from RGB triples to strings is still done in C, though.
This doesn't seem to have fixed either of the bugs I hoped it would, but
it does measurably improve drawing performance so I think it's worth
doing.
We were using free_cfg(be_prefs) after we copied be_prefs to
all_prefs, but we actually want to use sfree(be_prefs) since we don't
want to free each element that has been copied. None of the games so
far use string preferences, which is why they haven't been affected by
this bug.
The keyboard cursor in Same Game is white. The default yellow,
cyan, and light green were light enough to make the cursor hard to
see. I've darkened them all (without changing their hues) so that the
cursor is acceptably visible. This doesn't leave an ideal set of
colours, but they are at least still adequately distinct from one
another.
That way, any application displaying the .desktop with its icon will
pick the icon size it deems the best one for the current rendering.
See the Icon Theme Specification:
https://specifications.freedesktop.org/icon-theme-spec/latest/ar01s07.html
Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
This adds sizes 24×24 (common on Linux desktop, for instance in
application bars), as well as 64×64 and 128×128 (common on Linux
mobile).
I kept the existing border sizes, but using the same one from 44×44 to
96×96 sounds a bit weird, it’d probably be best to revisit them at some
point.
Signed-off-by: Emmanuel Gil Peyrot <linkmauve@linkmauve.fr>
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.
When we set up a configuration sheet, we track the minimum overall
width that the controls will fit into (in a variable 'totalw'), and
separately, the minimum width needed by each of the left and right
columns containing control labels and actual controls ('leftw' and
'rightw'). If totalw > leftw+rightw at the end of the process, then we
must expand the two columns so that they have the right sum.
However, sometimes leftw+rightw can be zero, while totalw > 0. This
occurs if _no_ control in the box was of a type that used the left and
right columns for different things, so that the entire loop over the
controls only incremented totalw, and not leftw or rightw. For
example, in a puzzle such as Cube that defines no preferences of its
own, the only control in the preferences pane is midend.c's standard
"Keyboard shortcuts without Ctrl" preference, which is C_BOOLEAN and
only uses totalw.
In that situation, the code for proportionate distribution of the
excess divides by zero. So it needs a special case.
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.
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.
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->faces, g->edges and g->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->faces' or 'e - g->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.
Now using the browser's "copy" operation while the focus is in the
puzzle will copy the puzzle state to the clipboard. Browsers seem to
have odd ideas about whate element to target with the "copy" event:
Firefox targets the parent of the <canvas> while Chromium targets the
<body>. To cope with these and possible future weirdness I attach the
event handler to the document and then look to see if it's plausibly
related to the canvas.
Arguably we might want to handle a wider range of "copy" events, maybe
any where the selection isn't empty. I'm not sure, though, so we'll
start with the minimal change.
I missed this in my previous addition of preferences for UI controls
(4227ac1fd5dc25c247e7526526079b85e1890766) because it wasn't documented.
Now it is documented and it has a preference.
I've phrased it as showing possible bridge locations, which doesn't
really make clear that "possible" relates only to the locations of
islands and not to anything already placed. Improvements welcome!
The change I made in c224416c76e41f284b318adc51f08c3ed11de8e2 was
incorrect: I accidentally removed a "return" statement and left in a
debugging printf() when I meant to keep the return and drop the
printf().
The boundary between them for mouse clicks probably wants to be
revisited because at the moment it's slightly inside the edge of the
grid. I tried using INUI() instead of INGRID() but that just gives a
different wrong answer, so I may need to actually understand what's
going on here.
In Pearl, a mouse-down outside the grid sets ui->ndragcoords to -1.
The intended effect of this is to make sure that future drags are
ignored, so you can't try to draw a line starting off the grid.
However, this also has the effect of clearing any in-progress drag.
This can happen if there's a keyboard "drag" in progress at the time.
This is arguably wrong, but much more wrong was that interpret_move
returned MOVE_UNUSED (and previously NULL) in this case. That meant
that the display didn't get updated to show the abandonment of the
drag, or the removal of the keyboard cursor that also happened. This
commit changes MOVE_UNUSED to MOVE_UI_UPDATE so that at least the
effect is correctly visible.
Chris Boyle points out that outline_block_structure has a comment
saying that we're supposed to have picked a square with a boundary to
its left. dsf_canonify no longer guarantees that, but dsf_minimal
does. Switch to using that throughout the function.
'keen --generate 10#12345 --print 5x2' failed an assertion before this
fix, and now doesn't.
midend_process_key() has some generic code for converting MOD_CTRL along
with a printing character into a control character. This is derived
from the Emscripten front-end because browsers don't do this themselves.
Most other front ends seem to depend on the platform for this mapping.
The mapping was applied to all printable ASCII characters, but this
meant that Ctrl+-, which is commonly used by browsers to mean "zoom out"
got converted into CR and then CURSOR_SELECT. That was confusing to say
the least.
So now, the CTRL mapping is only applied to characters in the roughly
alphabetic range (0x40 to 0x7f), and MOD_CTRL applied to a character in
the range 0x20 to 0x3f gets a return of PKR_UNUSED instead. That means
that Ctrl+- in browsers now works properly.
I don't think this will affect other front-ends because they're
generally a lot less generous about passing MOD_CTRL to the mid-end.
I've tested the GTK port nonetheless and not found any problems.
