In this platform's set_platform_puzzle_target_properties, I referred
to ${EXENAME} twice, which is not one of the function parameters. It
worked anyway, because CMake has dynamic scope, and that variable was
defined - to the right value - within the local-variable scope of the
calling function. But that wasn't at all what I meant to do!
Renamed it to ${TARGET}, which is the actual parameter name we get
passed.
The previous fix worked OK, but it was conceptually wrong. Puzzles
save files are better regarded as binary, not text: the length fields
are measured in bytes, so translating the file into a different
multibyte character encoding would invalidate them.
So it was wrong to fetch a C byte string containing the exactly right
binary data, then translate it into a Javascript string as if decoding
from UTF-8, then retranslate to a representation of a bytewise
encoding via encodeURIComponent, and then label the result as
application/octet-stream.
This probably wouldn't have caused any problems in practice, because I
don't remember any situation in which my save files use characters
outside printable ASCII (plus newline). But it's not actually
forbidden, so a save file might choose to do that some day, so that
UTF-8 decode/reencode hidden in the JS was a latent bug.
Now the URI-encoding is done on the C side, while we still know
exactly what the binary data ought to look like and can be sure we're
translating it byte for byte into the output encoding for the data:
URI. By the time the JS receives a string pointer from get_save_file,
it's already URI-encoded, which _guarantees_ that it's in ASCII and
won't be messed about with by Emscripten's UTF8ToString.
In commit f6434e84964d840 I said I had replaced all uses of
old-Emscripten's Pointer_stringify() function with new-Emscripten's
UTF8ToString(). In fact, I only replaced the ones in emcclib.js, but I
missed one in emccpre.js used in preparing downloadable save files.
Those were therefore broken, with a Javascript undefined-name error.
The previous code had multiple bugs. We had completely left out the
draw_update after drawing each arrow; we omitted the usual
precautionary clip() that constrains each arrow draw to the same
rectangle we just saved in the blitter; we re-computed the coordinates
of the opposite arrow at undraw time, instead of saving the
coordinates we _actually_ used after computing them the first time.
And we restored from the two blitters in the same order we saved them,
instead of reverse order, which was harmless at the time (the drawing
happened after both saves), but is generally bad practice, and needed
to be fixed when the code was rearranged to fix the rest of these
issues.
I noticed these issues in passing, while hunting the diagonal-line bug
fixed in the previous commit. These fixes by themselves would have
prevented any persistent drawing artefact as a result of that bug (the
clip() would have constrained the spurious diagonal line to the region
saved by the blitter, so it would have been undrawn again afterwards);
but it's better to have fixed the root cause as well!
During an interactive drag of an arrow, it's possible to put the mouse
pointer precisely on the pixel the arrow is pointing at. When that
happens, draw_arrow computes the zero-length vector from the pointer
to the target pixel, and tries to normalise it to unit length by
dividing by its length. Of course, this leads to computing 0/0 = NaN.
Depending on the platform, this can cause different effects.
Conversion of a floating-point NaN to an integer is not specified by
IEEE 754; on some platforms (e.g. Arm) it comes out as 0, and on
others (e.g. x86), INT_MIN.
If the conversion delivers INT_MIN, one possible effect is that the
arrow is drawn as an immensely long diagonal line, pointing upwards
and leftwards from the target point. To add to the confusion, that
line would not immediately appear on the display in full, because of
the draw_update system. But further dragging-around of arrows will
gradually reveal it as draw_update rectangles intersect the corrupted
display area.
However, that diagonal line need not show up at all, because once
draw_arrow has accidentally computed a lot of values in the region of
INT_MIN, it then adds them together, causing signed integer overflow
(i.e. undefined behaviour) to confuse matters further! In fact, this
diagonal-line drawing artefact has only been observed on the
WebAssembly front end. The x86 desktop platforms might very plausibly
have done it too, but in fact they didn't (I'm guessing because of
this UB issue, or else some kind of clipping inside the graphics
library), which is how we haven't found this bug until now.
Having found it, however, the fix is simple. If asked to draw an arrow
from a point to itself, take an early return from draw_arrow before
dividing by zero, and don't draw anything at all.
Thanks to Kaz Kylheku for pointing out that commit ff3e762fd007883
didn't do a complete job: I removed the code under '#ifdef _WIN32_WCE'
in windows.c, but missed sections under the same ifdef in puzzles.h
and puzzles.rc, together with an entire header file resource.h that
was only included by code under those ifdefs.
Trivially, no edge of this kind can be part of any legal solution, so
it's a minor UI affordance that doesn't spoil any of the puzzly
thinking to prevent the user accidentally placing them in the first
place.
Suggestion from Larry Hastings, although this is not his patch.
I just introduced the 'first_draw' flag in the midend, which should
force a screen clear whenever we draw a puzzle with a fresh drawstate.
But in fact there were several places where the midend replaces the
drawstate and I hadn't set that flag to true.
In particular, a user just reported that when you press 'n' for a new
game in an existing Magnets window, the new puzzle's clues are drawn,
but any old clues in places where the new puzzle doesn't have one is
not _un_drawn. (Because Magnets has no code to undraw a single clue -
it never needs to!)
Added a set of first_draw wherever we call new_drawstate, which should
make this reliable again.
Negative numbers are used as a sentinel for an absent clue, so we have
to use a type that's guaranteed to have some negative numbers. char is
unsigned on some platforms. So now Mosaic runs apparently correctly on
Raspbian, for example.
I don't know how I've never thought of this before! Pretty much every
game in this collection has to have a mechanism for noticing when
game_redraw is called for the first time on a new drawstate, and if
so, start by covering the whole window with a filled rectangle of the
background colour. This is a pain for implementers, and also awkward
because the drawstate often has to _work out_ its own pixel size (or
else remember it from when its size method was called).
The backends all do that so that the frontends don't have to guarantee
anything about the initial window contents. But that's a silly
tradeoff to begin with (there are way more backends than frontends, so
this _adds_ work rather than saving it), and also, in this code base
there's a standard way to handle things you don't want to have to do
in every backend _or_ every frontend: do them just once in the midend!
So now that rectangle-drawing operation happens in midend_redraw, and
I've been able to remove it from almost every puzzle. (A couple of
puzzles have other approaches: Slant didn't have a rectangle-draw
because it handles even the game borders using its per-tile redraw
function, and Untangle clears the whole window on every redraw
_anyway_ because it would just be too confusing not to.)
In some cases I've also been able to remove the 'started' flag from
the drawstate. But in many cases that has to stay because it also
triggers drawing of static display furniture other than the
background.
This is similar in concept to Minesweeper, in that each clue tells you
the number of things (in this case, just 'black squares') in the
surrounding 3x3 grid section.
But unlike Minesweeper, there's no separation between squares that can
contain clues, and squares that can contain the things you're looking
for - a clue square may or may not itself be coloured black, and if
so, its clue counts itself.
So there's also no hidden information: the clues can all be shown up
front, and the difficulty arises from the game generator choosing
which squares to provide clues for at all.
Contributed by a new author, Didi Kohen. Currently only has one
difficulty level, but harder ones would be possible to add later.
Using a stunt webserver which artificially introduces a 3s delay just
before the last line of the HTML output, I have reproduced a
uwer-reported loading/startup race bug:
Previously the wasm loading was started by the <script> element,
synchronously. If the wasm loading is fast, and finishes before the
HTML loading, the onRuntimeInitialized event may occur before
initPuzzles. But initPuzzles sets up the event handler.
Fix this bug, and introduce a new comment containing an argument for
the correctness of the new approach.
Signed-off-by: Ian Jackson <ijackson@chiark.greenend.org.uk>
A grid based on dodecagons with square symmetry. In between dodecagons
there are 4 triangles around 1 square, which resembles a compass rose.
https://en.wikipedia.org/wiki/3-4-3-12_tiling
These came from Visual Studio, and seem to be real problems - we're
casting pointers to 32-bit integers, which surely only works by luck
of address-space allocation.
This set of rules should cover make and ninja on Linux, and all of
nmake, ninja and vcxproj on Windows, so that if someone follows the
README build instructions (by doing 'cmake .' in-tree), it should
generate no debris that .gitignore can't filter out.
How embarrassing. When I updated the Emscripten build to use WASM, a
major reason I bothered to do it at all was that I'd heard that WASM
was capable of reallocating its memory arena larger on the fly. Turns
out that it _can_, but only if you specifically set the option in
Emscripten to allow it.
With this option set, I can finish a 25x25 Galaxies, where previously
the game would crash part way through (and not even a very large part)
with errors about memory growth in the Javascript console.
This allows the icons build to automatically disable itself if Perl
can't be found at all (and print a warning explaining that that's
why). It also means that if Perl exists on the system but is somewhere
other than /usr/bin (where our #! lines expect it), the icons build
can still run.
Now I've had a chance to collect some more data from browser users,
it's got a list of browsers in which we've definitely seen the WASM
puzzle working (so that if _your_ instance of one of those browsers
fails, you should check it for problems at your end, like
configuration details or overzealous web filtering software), and some
thoughts about what to report.
The result is a lot longer than the previous error message, so I've
put the bulk of it in a <details>. That way it won't look so silly
when it initially flashes up while things are loading.
The old one was totally out of date (it mentioned typed arrays and a
specific set of browser versions against which the previous Emscripten
build had been tested).
Also, a couple of users in the last day or two have reported
mysterious failures of the WASM puzzles, which I haven't been able to
reproduce in the same version of the same browser. So something odd is
going on, and this seems like a good place to put suggestions about
what diagnostic information to send.
Various cmake variables that I was informally expecting users to set
on the cmake command line (e.g. cmake -DSTRICT=ON, or cmake
-DPUZZLES_GTK_VERSION=2) are now labelled explicitly with the CACHE
tag, and provided with a documentation string indicating what they're
for.
One effect of this is that GUI-like interfaces to your cmake build
directory, such as ccmake or cmake-gui, will show those variables
explicitly to give you a hint that you might want to change them.
Another is that when you do change them, cmake will recognise that it
needs to redo the rest of its configuration. Previously, if you sat in
an existing cmake build directory and did 'cmake -DSTRICT=ON .'
followed by 'cmake -DSTRICT=OFF .', nothing would happen, even though
you obviously meant it to.
I presume this will improve performance. Also, if I've understood
correctly, WASM-based compiled web code is capable of automatically
growing its memory, which the previous asm.js build of the puzzles
could not do, and occasionally caused people to complain that if they
tried to play a _really big_ game in their browser, the JS would
eventually freeze because the emulated memory ran out.
I've been putting off doing this for ages because my previous
Emscripten build setup was so finicky that I didn't like to meddle
with it. But now that the new cmake system in this source tree makes
things generally easier, and particularly since I've just found out
that the up-to-date Emscripten is available as a Docker image (namely
"emscripten/emsdk"), this seemed like a good moment to give it a try.
The source and build changes required for this update weren't too
onerous. I was half expecting a huge API upheaval, and indeed there
was _some_ change, but very little:
- in the JS initPuzzle function, move the call to Module.callMain()
into Module.onRuntimeInitialized instead of doing it at the top
level, because New Emscripten's .js output likes to load the
accompanying .wasm file asynchronously, so you can't call the WASM
main() until it actually exists.
- in the JS-side library code, replace all uses of Emscripten's
Pointer_stringify() function with the new name UTF8ToString(). (The
new version also has an ASCIIToString(), so I guess the reason for
the name change is that now you get to choose which character set
you meant. I need to use UTF-8, so that the × and ÷ signs in Keen
will work.)
- set EXTRA_EXPORTED_RUNTIME_METHODS=[cwrap,callMain] on the emcc
link command line, otherwise they aren't available for my JS setup
code to call.
- (removed -s ASM_JS=1 from the link options, though I'm not actually
sure it made any difference one way or the other in the new WASM
world)
- be prepared for a set of .wasm files to show up as build products
alongside the .js ones.
- stop building with -DCMAKE_BUILD_TYPE=Release! I'm not sure why
that was needed, but if I leave that flag on my cmake command line,
the output .js file fails to embed my emccpre.js, so the initial
call to initPuzzle() fails from the HTML wrapper page, meaning
nothing at all happens.
At least, for the Unix build, so as to support Debian stable and a
couple of prior Ubuntu LTSes.
Not much needed to change in the cmake scripts; the only noticeable
difference was that the 'install' command needs an explicit RUNTIME
DESTINATION.
The puzzle icons are built by compiling and running a preliminary
set of puzzle binaries. We can't do that if the binaries won't run
on the build host.
A distro maintainer reminds me that downstreams often want to rename
my quite generic executable names to avoid clashes in bin directories.
Added a cmake option -DOUTPUT_NAME to make that easy.
It's better to be lax for normal users trying to build the puzzles
from source to actually run them. That way, warning changes in some
particular compiler I haven't seen yet won't break the build.
Instead, I've invented a cmake setting -DSTRICT=ON which turns on all
those flags. So I can build with them myself, to ensure the code is as
portable as possible. And that flag is set in Buildscr, so that my
official builds won't complete until that warning mode is satisfied.
This reinstates the feature of the previous build system, that the C
icon files for the GTK puzzles were included in the source tarball, so
that users building from that instead of from the raw git repo would
not need to run the fiddly piece of build that regenerates them.
Running that fiddly piece of build is much easier in the CMake world
(because it's integrated with the main makefile), but it has a build
dependency on ImageMagick which is easily avoided.
The makefile will still build the icons if it _can_. But in the case
where it can't, it will use pre-built icon source files if they're
available, and only fall back to no-icon.c if it can't even do that.
(So a user checking out from git and building without ImageMagick
present will still be able to build _something_ playable.)
This way, ImageMagick is no longer a hard build dependency. For
developers or users, building puzzles without nice icons is preferable
to not building them at all.
(Also, thanks to Michael Quevillon for pointing out very promptly that
my use of 'REQUIRED' in the find_program command was implicitly
depending on a version of CMake in advance of my minimum_required
specification. This change fixes that too, in passing.)
If I built with an unfinished puzzle enabled, then it will end up in
the 'unfinished' subdir of the main build directory, so desktop.pl
will need to write out a reference to it there.
This is another modification to the same piece of code as the previous
commit. Previously, a square with a neighbour in a same-sized region
was fixed by choosing a neighbour to merge it with that was part of
the smallest region. Now, it's _usually_ that, but sometimes it can be
a larger neighbour instead.
Partly, I hope this might remove a potential source of regularity in
the random grids. But mostly, it prevents the grid generator from
hanging completely on 2x2 grids (e.g. if you gave "2x2#12345" in the
previous state of the code), because with the previous 'always
minimal' rule, the generator would merge together two squares of the
2x2 grid, then the other two, and then (due to maxsize==3) it would
have no merge remaining to clear the final error. Now, every so often,
it will take the unusual option of making a size-3 region instead,
which allows game generation to succeed.
The method of generating a solved Filling grid (before winnowing
clues) is to loop over every square of the board, and for each one, if
it has a neighbour which is part of a different region of the same
size (i.e. the board is not currently legal), fix it by merging with
one of its neighbours. We pick a neighbour to merge with based on the
size of its region - but we always loop over the four possible
neighbours in the same order, which introduces a directional bias into
the breaking of ties in that comparison.
Now we iterate over the four directions in random order.
This would come up on the game id "3x1#12345", for example. The
failing assertion was (s->board[f] != EMPTY) in expand(), called in
turn from learn_expand_or_one().
It looks as if the problem was that the #define SENTINEL was set too
small. It was intended to be a value that can't coincide with the true
size of any region - and it was set to precisely the area of the whole
board. But on a 3x1 grid, that _can_ coincide with the size of a
region! So a board entry was set to a real region size, and then
mistaken for SENTINEL by another part of the code.
Easy fix: set SENTINEL to be sz+1. Now it really can't coincide with a
region area.
The WinCE version of these puzzles hasn't been built for years, and
the last vestiges of its build system vanished with the migration to
CMake. So this seems like a good moment to lose the rest of it.
So the supporting Perl script wceinf.pl is deleted, and so is all the
unused code under '#ifdef _WIN32_WCE' in windows.c. (I removed that
using unifdef, which did a more reliable job than I would have done by
hand!)
I noticed this while I was overhauling the build system. We haven't
used an Inno Setup based installer for years, so the script that
constructed Inno Setup's input file is well and truly obsolete and
should have been deleted long since.
This completely removes the old system of mkfiles.pl + Recipe + .R
files that I used to manage the various per-platform makefiles and
other build scripts in this code base. In its place is a
CMakeLists.txt setup, which is still able to compile for Linux,
Windows, MacOS, NestedVM and Emscripten.
The main reason for doing this is because mkfiles.pl was a horrible
pile of unmaintainable cruft. It was hard to keep up to date (e.g.
didn't reliably support the latest Visual Studio project files); it
was so specific to me that nobody else could maintain it (or was even
interested in trying, and who can blame them?), and it wasn't even
easy to _use_ if you weren't me. And it didn't even produce very good
makefiles.
In fact I've been wanting to hurl mkfiles.pl in the bin for years, but
was blocked by CMake not quite being able to support my clang-cl based
system for cross-compiling for Windows on Linux. But CMake 3.20 was
released this month and fixes the last bug in that area (it had to do
with preprocessing of .rc files), so now I'm unblocked!
CMake is not perfect, but it's better at mkfiles.pl's job than
mkfiles.pl was, and it has the great advantage that lots of other
people already know about it.
Other advantages of the CMake system:
- Easier to build with. At least for the big three platforms, it's
possible to write down a list of build commands that's actually the
same everywhere ("cmake ." followed by "cmake --build ."). There's
endless scope for making your end-user cmake commands more fancy
than that, for various advantages, but very few people _have_ to.
- Less effort required to add a new puzzle. You just add a puzzle()
statement to the top-level CMakeLists.txt, instead of needing to
remember eight separate fiddly things to put in the .R file. (Look
at the reduction in CHECKLST.txt!)
- The 'unfinished' subdirectory is now _built_ unconditionally, even
if the things in it don't go into the 'make install' target. So
they won't bit-rot in future.
- Unix build: unified the old icons makefile with the main build, so
that each puzzle builds without an icon, runs to build its icon,
then relinks with it.
- Windows build: far easier to switch back and forth between debug
and release than with the old makefiles.
- MacOS build: CMake has its own .dmg generator, which is surely
better thought out than my ten-line bodge.
- net reduction in the number of lines of code in the code base. In
fact, that's still true _even_ if you don't count the deletion of
mkfiles.pl itself - that script didn't even have the virtue of
allowing everything else to be done exceptionally concisely.
Several of the source files here won't quite compile any more, because
of minor things like const-correctness and the UI_UPDATE change. Now
they should all build again (without prejudice to how useful they are
once they have built).
The biggest change was to remove the fatal() implementation from the
standalone path.c, because my new plan is that basically everything
that's not linked against a true puzzle frontend will be linked
against nullfe.c, which provides that function anyway.
Adding an association with an out-of-bounds square (i.e. by pressing Return
with a dot selected, and then moving the cursor so the `opposite' arrow was
off the screen) would cause space_opposite_dot() to return NULL, in turn
causing ok_to_add_assoc_with_opposite_internal() to return false, failing
the assertion.
This assertion appears to have been introduced in 68363231.
The Rockbox frontend allows games to be displayed in a "zoomed-in"
state targets with small displays. Currently we use a modal interface
-- a "viewing" mode in which the cursor keys are used to pan around
the rendered bitmap; and an "interaction" mode that actually sends
keys to the game.
This commit adds a midend_get_cursor_location() function to allow the
frontend to retrieve the backend's cursor location or other "region of
interest" -- such as the player location in Cube or Inertia.
With this information, the Rockbox frontend can now intelligently
follow the cursor around in the zoomed-in state, eliminating the need
for a modal interface.
Generating the game id 6dui#12345 would cause an assertion failure in
a call to latin_solver_place that should never have happened in the
first place, because the "return -1" that ought to have prevented it
was accidentally inside #ifdef STANDALONE_SOLVER.
This too seems to have made no difference: each of the commands
unequal --generate 1000 6dr#12345
unequal --generate 1000 6adr#12345
delivers the same list of puzzles before and after the fix.
This seems to make no difference that I can detect: a test generation
run of the form 'keen --generate 1000 6du#12345' outputs an identical
list of 1000 puzzle ids before and after. So the missing validation in
this puzzle seems to have been benign.
