This new option, when enabled, forces the in-tree front ends
(Emcscripten, GTK, NestedVM, OS X, and Windows) to use the recently
introduced draw_polygon_fallback() in place of their native
draw_poly(). This will enable easy testing of this function in the
future.
This new option is off by default. To enable it, run CMake as:
$ cmake -DUSE_DRAW_POLYGON_FALLBACK=on
Note that I did _not_ update the Postscript frontend (ps.c) to use
this new option, as I don't think draw_polygon_fallback() would work
at all in Postscript, where the drawing units are no longer guaranteed
to be pixels. The envisioned use case for this option is a developer
testing changes to this function for sanity and/or performance, which
I only foresee happening on a standard GUI front end.
This adds a portable, scanline-based polygon filling algorithm, which
fills a polygon by drawing a collection of adjacent horizontal lines.
This change is motivated by the Rockbox port's current lack of a true
polygon fill capability. Until now, it attempted to approximate a
polygon fill by performing a series of triangle fills, but this worked
reliably only for convex polygons. I originally considered making this
new rasterizer part of the Rockbox front end itself, but I ultimately
decided that it made more sense to include it here, in the Puzzles
distribution, where other platforms may benefit from it in the future.
No in-tree front ends use this new function quite yet, but I plan to
follow this commit with a compile-time option to force front ends to
use it for testing.
This new polygon drawing code also comes with its own standalone
driver code to test it out in isolation. This code currently relies on
SDL 2.0 to present a GUI window to the user, which unfortunately adds
a build-time dependency. To lessen the impact of this change, this
program is gated behind a CMake build option. To use it, run:
$ cmake -DBUILD_SDL_PROGRAMS=true
I updated Emscripten recently, to version 3.1.54. The result was that
none of the WASM puzzles run any more, because they produce a stack
trace at startup with the error message "to do setValue(i64) use
WASM_BIGINT".
I don't fully understand this. The stack trace comes via a JS wrapper
around a WASM-compiled function called __main_argc_argv, which sounds
like something in the Emscripten library startup. Somewhere in there
it goes via _js_get_date_64(), which tries to write an i64 into the
WASM memory array, which hits this abort in setValue().
Web searching for the error message turned up
https://github.com/godotengine/godot/pull/88594 which gave me the clue
that '-s WASM_BIGINT' is a command-line setting for the Emscripten
linker. And indeed, setting that makes the startup-time error go away
and the puzzles run again. But it also causes older versions of all
browsers to be unsupported, presumably on the grounds that they don't
have whatever WASM bigint feature this flag causes the code to use.
I don't really understand what's going on here. I assume
_js_get_date_64 is being called to handle a 64-bit time_t. But the
Puzzles code doesn't work with time_t at all, so this is entirely in
the Emscripten standard library. And if the pre-main() startup code
needs a 64-bit integer write, which won't work without this flag, then
surely _nothing_ would work without this flag, and surely someone
would have noticed that, and made that flag the default? This all
seems confusing and I wonder if I've misunderstood something.
However, if I don't fix it in _some_ way, the web puzzles will stay
out of action for days and I'll get lots of email complaints. So
here's something that makes them basically work again, and maybe we
can figure out the rest of the story later.
This includes the textual version number in its existing
form (yyyymmdd followed by an abbreviated git hash). The four-part
binary version is set to 1 followed by year, month and day; if I ever
want to change that, I can increment the initial 1.
FileDescription is taken from the existing DESCRIPTION string provided
to each puzzle() statement in CMakeLists.txt.
This means that puzzles.rc now always defines at least one resource,
so we can remove the workaround for MinGW's windres not being able to
cope with an empty .rc file, which added a dummy resource in the
absence of an icon.
There's no reason to put the .rc file into developer tools like
galaxieseditor at all. Its current job is to add an icon, and those
tools don't have any. I'm about to add version information, and they
won't have that either (in particular, no description string like the
games do).
The CLI developer tools already don't include puzzles.rc, and GUI dev
tools are more like those than they are like puzzles.
puzzles.rc was being added to an aux GUI tool's source file list by
get_platform_puzzle_extra_source_files(), which is called for aux GUI
tools as well as for puzzles proper. However, it's not as simple as
just eliminating that call, because on Unix, we _do_ need to add the
same extra source files to GUI dev tools that we do for puzzles,
because gtk.c contains external references to either an array of the
puzzle's icons or an empty array indicating that there aren't any, so
_something_ has to provide that.
So instead, get_platform_puzzle_extra_source_files now takes an extra
argument saying whether the program is a real puzzle or an aux tool;
windows.cmake leaves out puzzles.rc in the latter case, but unix.cmake
puts the icon array in unconditionally.
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>
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.
The 'core' library contains almost all the same objects as 'common',
but leaves out hat.c. And the auxiliary program 'hatgen' now links
against that slightly reduced core library instead of 'common'.
This avoids a dependency loop: one of hatgen's jobs is to generate
hat-tables.h, but hat-tables.h is a dependency of it.
Of course, the generated hat-tables.h is already committed, so this
doesn't present a bootstrapping problem in a normal build. But if
someone modifies hatgen.c in order to regenerate hat-tables.h, and
does so in a way that makes it uncompilable, they can't rebuild hatgen
and try again! Of course you can always revert changes with git, but
it's annoying to have to. Better to keep the dependencies non-cyclic
in the first place.
The C stack used by Emscripten is quite small, so passing more than a
few klilobytes of data on it tends to cause an overflow. Current
versions of puzzles will only generate tiny preferences files, but this
might change in future and in any case Puzzles shouldn't crash just
because the preferences in local storage have got corrupted.
To fix this, we now have JavaScript allocate a suitable amount of C heap
memory using malloc() and stick the preferences file in there.
This could plausibly fail if the preferences file were really big, but
that's unlikely since browsers generally limit an origin to about 5 MB
of local storage. In any case, if malloc() does fail, we'll just ignore
the preferences file, which is probably the right thing to do.
Here, user preferences are stored in localStorage, so that they can
persist when you come back to the same puzzle page later.
localStorage is global across a whole web server, which means we need
to take care to put our uses of it in a namespace reasonably unlikely
to collide with unrelated web pages on the same server. Ben suggested
that a good way to do this would be to store things in localStorage
under keys derived from location.pathname. In this case I've appended
a fragment id "#preferences" to that, so that space alongside it
remains for storing other things we might want in future (such as
serialised saved-game files used as quick-save slots).
When loading preferences, I've chosen to pass the whole serialised
preferences buffer from Javascript to C as a single C string argument
to a callback, rather than reusing the existing system for C to read
the save file a chunk at a time. Partly that's because preferences
data is bounded in size whereas saved games can keep growing; also
it's because the way I'm storing preferences data means it will be a
UTF-8 string, and I didn't fancy trying to figure out byte offsets in
the data on the JS side.
I think at this point I should stop keeping a list in the docs of
which frontends support preferences. Most of the in-tree ones do now,
and that means the remaining interesting frontends are ones I don't
have a full list of. At this moment I guess no out-of-tree frontends
support preferences (unless someone is _very_ quick off the mark), but
as and when that changes, I won't necessarily know, and don't want to
have to keep updating the docs when I find out.
Emscripten has settings indicating which browser versions it should
build code for. These are now by default slightly newer than I'd been
targeting with my hand-written JavaScript. They also don't include
Firefox 48, which KaiOS 2.5 is based on.
This commit adds CMake variables to set the minimum versions that we
pass to Emscripten. They default to the earliest versions with
WebAssembly support, except that Firefox 48 is also supported.
I think the main consequence of this change is to stop Emscripten using
sign-extension and mutable-globals in WebAssembly, which it's done by
default since version 3.1.26.
Puzzles only runs in Web browsers, so there's no need to include
support for Node or (for now at least) running in a Web worker. This
removes about 5 kiB of code from the boilerplate JavaScript.
This fixes a build failure introduced by commit 2e48ce132e011e8
yesterday.
When I saw that commit I expected the most likely problem would be in
the NestedVM build, which is currently the thing with the most most
out-of-date C implementation. And indeed the NestedVM toolchain
doesn't have <tgmath.h> - but much more surprisingly, our _Windows_
builds failed too, with a compile error inside <tgmath.h> itself!
I haven't looked closely into the problem yet. Our Windows builds are
done with clang, which comes with its own <tgmath.h> superseding the
standard Windows one. So you'd _hope_ that clang could make sense of
its own header! But perhaps the problem is that this is an unusual
compile mode and hasn't been tested.
My fix is to simply add a cmake check for <tgmath.h> - which doesn't
just check the file's existence, it actually tries compiling a file
that #includes it, so it will detect 'file exists but is mysteriously
broken' just as easily as 'not there at all'. So this makes the builds
start working again, precisely on Ben's theory of opportunistically
using <tgmath.h> where possible and falling back to <math.h>
otherwise.
It looks ugly, though! I'm half tempted to make a new header file
whose job is to include a standard set of system headers, just so that
that nasty #ifdef doesn't have to sit at the top of almost all the
source files. But for the moment this at least gets the build working
again.
It was originally used for probing the available warning flags in the
compiler, by adding them one by one and making sure this test file
still compiled.
But that whole mechanism was removed in commit 306fab356e357ef, and
since then, testbuild.c has been unused. Belatedly throw it away.
The KaiOS build includes compiled versions of various Emscripten library
files. These are generally under the MIT licence like Puzzles itself.
The MIT licence requires that the licence, and the copyright notice, be
"included in all copies or substantial portions of the Software."
Since each KaiOS package includes the full manual, which already
contains the licence for Puzzles itself, adding the copyright notices
there seems like the best approach. I've done this by providing an
additional input file that contains the licences for source files used
by a current Emscripten build. More automation might be nice, but the
set of copyright notices is unlikely to change very much. There are
basically one for Emscripten, one for musl, and a few for odd bits of
third-party code embedded in musl.
C89 provided only double-precision mathematical functions (sin() etc),
and so despite using single-precision elsewhere, those are what Puzzles
has traditionally used. C99 introduced single-precision equivalents
(sinf() etc), and I hope it's been long enough that we can safely use
them. Maybe they'll even be faster.
Rather than directly use the single-precision functions, though, we use
the magic macros from <tgmath.h> that automatically choose the precision
of mathematical functions based on their arguments. This has the
advantage that we only need to change which header we include, and thus
that we can switch back again if some platform has trouble with the new
header.
After commit 1470c9530b1cff3 enabled assertions, I found that my build
scripts were complaining that the Windows binaries built from the same
source twice were not generating the same output, and it turned out to
be because the use of __FILE__ in every assert was baking in a
pathname from my build setup containing a mkstemp()-randomised path
component.
I've found the '-fmacro-prefix-map' option, available in both gcc and
clang (except the archaic gcc used by my NestedVM build, alas), which
lets you remap pathnames for purpose of what __FILE__ expands to. So
now our assertion statements should look as if the puzzle source just
lived in /puzzles, and (just in case a pathname in a generated header
ever becomes relevant) the cmake build directory is /build.
Despite the name, COMPILE_DEFINITIONS was only ever used to set a
single definition, and as far as I can tell that's all it could do
even when I tried to put them in a single word separated by
semicolons. Turning COMPILE_DEFINITIONS into a multi-valued argument
seems to make it work much better.
Assertion failures are ugly, but they're better than the alternative.
Defensive coding is a general principle throughout Puzzles and I don't
think it's sensible to selectively turn that off.
The mechanism by which we re-enable assertions is stolen from PuTTY
(with an enhancement to cover MinSizeRel builds as well) and is pretty
ugly because CMake doesn't seem to have a good way to do it.
After Ben fixed all the unwanted global functions by using gcc's
-Wmissing-declarations to spot any that were not predeclared, I
remembered that clang has -Wmissing-variable-declarations, which does
the same job for global objects. Enabled it in -DSTRICT=ON, and made
the code clean under it.
Mostly this was just a matter of sticking 'static' on the front of
things. One variable was outright removed ('verbose' in signpost.c)
because after I made it static clang was then able to spot that it was
also unused.
The more interesting cases were the ones where declarations had to be
_added_ to header files. In particular, in COMBINED builds, puzzles.h
now arranges to have predeclared each 'game' structure defined by a
puzzle backend. Also there's a new tiny header file gtk.h, containing
the declarations of xpm_icons and n_xpm_icons which are exported by
each puzzle's autogenerated icon source file and by no-icon.c. Happily
even the real XPM icon files were generated by our own Perl script
rather than being raw xpm output from ImageMagick, so there was no
difficulty adding the corresponding #include in there.
Ben added -Wmissing-declarations in commit 3a3e491a8bc624e for gcc
builds, and observed that clang's option of the same name doesn't do
the same job. But clang does _have_ an option to do the same job: it's
just spelled differently. Added -Wmissing-prototypes in clang builds,
so that those will check the same thing.
Clang's -Wmissing-declarations means something quite different, so we
only enable it on GCC. This is slightly silly since Clang's
-Wmissing-declarations is enabled by default, but it makes it clear that
we know they're different.
Rather than installing the documentation only when Halibut is
available, install the documentation whenever it exists. This allows
for the way that Buildscr injects the documentation into official
KaiOS builds.
The Kai Store makes display of advertisements provided by the KaiAds API
mandatory. I don't want such adverts to be inconvenient for the users,
so I've just gone for adding a menu item that will display one. This is
probably a little too crude, but it's good for testing things.
The actual KaiAds API code is not free software, so it's not included
here. My intention is to add it by hand to the Zip files for Kai Store
uploads. Without it, the advertising code does nothing.
Previously, we initialised all of the JavaScript event handlers as soon
at the DOM was loaded, and then called main() ourselves once the
Emscripten runtime was ready. This was slightly dangerous since it
depended on none of those event handlers' being called before main().
In practice this was difficult because most of the elements the event
handlers were attached to were invisible, but it did limit what event
handlers could safely be used.
Now, the event handlers are initialised from main(). This makes things
work in a sufficiently conventional way that we can just let the
Emscripten run-time call main() in its usual way, rather than involving
ourselves in the minutiae of Emscripten's startup.
KaiOS (which is based on Firefox OS, formerly Boot to Gecko) runs its
"native" apps in a Web browser, so this is essentially a rather
specialised version of the JavaScript front-end. Indeed, the JavaScript
and C parts are the same as the Web version.
There are three major parts that are specific to the KaiOS build.
First, there's manifest.pl, which generates a KaiOS-specific JSON
manifest describing each puzzle.
Second, there's a new HTML page generator, apppage.pl, that generates an
HTML page that is much less like a Web page, and much more like an
application, than the one generated by jspage.pl. It expects to build a
single HTML page at a time and gets all its limited knowledge of the
environment from its command line. This makes it gratuitously different
from jspage.pl and javapage.pl, but makes it easier to run from the
build system.
And finally, there's the CMake glue that assembles the necessary parts
for each application in a directory. This includes the manifest, the
HTML, the JavaScript, the KaiOS-specific icons (generated as part of the
GTK build) and a copy of the HTML documentation. The directory is
assembled using CMake's install() function, and can be installed on a
KaiOS device using the developer tools.
If I re-run cmake in a Unix build directory, it unconditionally
rewrites generated-games.h, which causes fuzzpuzz to be rebuilt. This
is a waste of effort in the extremely common case where the rewritten
generated-games.h is identical to the old one.
Now we write the data to a temporary file first, and use cmake's
'configure_file' command to copy that to generated-games.h, because it
so happens that configure_file checks if the two files are identical
and avoids updating the timestamp on the destination file if so.
(This will presumably also be a beneficial change on any other
platform that uses generated_games.h in the build, such as OS X. I
just hadn't noticed until it hit the build I most often re-run in an
existing build directory.)
cmake 3.21 has a more intuitively spelled command I could have used,
called 'file(COPY_FILE src dst ONLY_IF_DIFFERENT)'. But we currently
permit cmake all the way back to 3.5, so I can't use that.
All the not-quite-puzzle GUI programs (only galaxieseditor at this
point) were using the extra source files from Null Game, presumably as a
convenient way of asking not to have an icon on platforms where icons
are provided as extra source files. Unfortunately, this all went wrong
if Null Game did have a save file for an icon, causing CMake to complain
about multiple definitions of a target.
Now these programs look for extra files under their own names, which
will work just as well since those have no save files to generate icons
from either.
This adds a new callback, rescale_puzzle(), that's called when the
device pixel ratio changes. This means that resize_puzzle() can safely
set the nominal canvas size, which means that manual resizing of the
puzzle now sticks.
Still missing: paying attention to the device pixel ratio when choosing
the initial (or reset) size.
- Generate HTML pages from the manual, and install them
- Add "Contents" and "Help on <name>" menu items that will open the
appropriate page in a web browser
I've had a lot of Puzzles nightly builds fail recently in the NestedVM
stage, with a 'jar' command producing a message along these lines:
java.util.zip.ZipException: attempt to write past end of STORED entry
at java.base/java.util.zip.ZipOutputStream.write(ZipOutputStream.java:337)
at jdk.jartool/sun.tools.jar.Main.copy(Main.java:1250)
at jdk.jartool/sun.tools.jar.Main.copy(Main.java:1263)
at jdk.jartool/sun.tools.jar.Main.addFile(Main.java:1211)
at jdk.jartool/sun.tools.jar.Main.create(Main.java:879)
at jdk.jartool/sun.tools.jar.Main.run(Main.java:319)
at jdk.jartool/sun.tools.jar.Main.main(Main.java:1681)
Suppressed: java.util.zip.ZipException: invalid entry size (expected 0 but got 786 bytes)
at java.base/java.util.zip.ZipOutputStream.closeEntry(ZipOutputStream.java:288)
at java.base/java.util.zip.ZipOutputStream.finish(ZipOutputStream.java:361)
at java.base/java.util.zip.DeflaterOutputStream.close(DeflaterOutputStream.java:238)
at java.base/java.util.zip.ZipOutputStream.close(ZipOutputStream.java:378)
at jdk.jartool/sun.tools.jar.Main.create(Main.java:854)
... 2 more
It's hard to work out exactly what this error dump means, and
web-searching for the error message isn't much help because the same
exception can occur in application code using java.util.zip, and most
mentions on the web are about that, and not about what I want to know,
which is why it might happen in the 'jar' program in particular.
However, the clues visible in that message suggest that 'jar' had
somehow got confused about the size of one of the files it was adding
to the jar archive, in that it initially decided it was 0 bytes long
and later found it was longer. That suggests a problem of excessive
parallelism between the build steps, perhaps due to a missing
dependency in the makefile, which might plausibly cause the 'jar' step
to be running already while some file it needs to read is still being
written. (Which would also explain why it doesn't happen every time.)
An eyeball review of cmake/platforms/nestedvm.cmake didn't find any
obvious missing dependencies. But I vaguely remembered that in some
other context I'd had trouble with cmake 'add_custom_command'. So in
this commit I replace all those custom commands with custom _targets_,
listing the previous OUTPUT files as BYPRODUCTS. And then the
dependencies are written using the target names, instead of the file
names.
I don't fully understand why this should make a difference. But it
seems more reliable in a soak test, and still builds the right things,
so I'll commit it and see if it makes the flakiness in the actual
nightly builds stop happening.
I don't expect this to actually come up in any circumstance, but it
prevents a warning in some versions of gcc that would otherwise arise
from the use of 'int' to compute the input size: if gcc isn't
confident that the int is positive, then it complains that possible
inputs to malloc might be in the region of 2^64 - (small multiple of a
negative 32-bit int).
I would hope malloc would fail in any case on such an input, so
failing a couple of lines earlier makes no important difference.
Annoyingly, stdint.h is missing in my NestedVM build setup (though it
has stdbool.h - it's not _totally_ C90). So I have to check that at
cmake time.
Also, removed the #defines for smalloc and friends from the tree234
test mode. These were needed in the old build system, when
tree234-test was built ad-hoc without being linked against malloc.c.
But now tree234-test links against the same utils library as
everything else, and can use the real smalloc - and doing so prevents
another of these warnings when compiling with -flto.
It relied on reading gamedesc.txt to find a list of puzzle binaries to
run. But gamedesc.txt is now specific to the Windows build (since it
contains Windows executable names), and isn't available in the Unix
cmake build directory.
Fixed by making a simpler gamelist.txt available on all platforms.
These look like puzzles, in that they link against a frontend and
provide the usual 'struct game', but they don't count as a puzzle for
purposes of shipping, or even having to have descriptions and icons.
There's one of these buried in the code already under an ifdef, which
I'll re-enable in the next commit.
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.
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>
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.
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.
