On many Rockbox devices, the cursor in Guess is invisible due to the low
screen resolution, which causes CGAP to round down to zero. This change
imposes a lower bound of 1 pixel.
Interestingly, this is less of an issue on front ends with antialiasing,
since the cursor peg's black border is overdrawn twice with, causing its
hazy antialiased boundary to get noticeably darker, even when CGAP is zero.
The recent addition of graph editor functionality to Untangle in
2ac951e broke redraws while dragging a point using the cursor keys. In
particular, the new ui->dragtype field was not being set by the cursor
dragging code path, causing redraws to erroneously exit early.
Generally, there is quite a bit of code duplication between the cursor
and mouse dragging code paths in interpret_move(). Some refactoring
could perhaps reduce the chances of this happening again.
I'm starting to question why the "label" field of the key_label struct
is dynamically allocated in the first place. Only Undead sets it to
anything other than NULL, and I see no reason that wouldn't work with
statically allocated string constants.
(Doing some archaeology shows that the dynamically allocated "label"
field was introduced by... me! I can only wonder what I was thinking 7
years ago when I first wrote that code. But that's a problem for
another time...)
Commit 97a0dc0 adjusted the backing surface size based on the GTK
window scale, and used cairo_scale so that drawing code is oblivious
to this. The blitter operates on the underlying pixels and so should
account for the scaling, but no code was added for this.
Now we multiply the blitter dimensions by the window scale, scale the
pixel coordinates to save from, and set the scaling factor to 1:1 when
loading. This fixes rendering artefacts with "GDK_SCALE=2", or when
using display scaling with Wayland.
Also adjust the backing surface position in draw_area so that it is
centered properly.
Reproduced via 'groupsolver -v 5i:p1g4' (thanks to Arun Giridhar for
the report).
The Group-specific solver_hard() function ruled out a bunch of
possibilities by deducing various things that couldn't be the group
identity, but forgot to set done_something = true, so that it return 0
claiming it hadn't done anything.
So latin_solver_top progressed to the next difficulty level and tried
recursion. And latin_solver_recurse failed an assertion because it was
surprised to find a cell with only one possibility - it expected that
the _simple_ deductions would have ruled out any of those, which they
would have if solver_hard() had returned >0, because the loop would
have reset to the top and tried the easy deductions again after
solver_hard() had given them something to work with.
Reproduced via 'groupsolver -v 5i:l5_2b5h' (thanks to Arun Giridhar
for the report).
We had filled in subsolver.names, but then called
latin_solver_alloc(&subsolver), which nulled out that pointer again.
Solution: do those two things in the opposite order.
Clicking just outside the left or top edges of the grid in Pattern
has the same effect as clicking just inside the same edge.
This happens because the FROMCOORD macro uses integer division,
which rounds towards zero rather than towards negative infinity.
One way to fix this is to add TILE_SIZE before the division
and subtract 1 afterwards.
This change to Untangle allows for custom frontends to optionally
define their own CIRCLE_RADIUS and DRAG_THRESHOLD parameters.
Rationale: The hardcoded values provided here (specifically a 6px
circle radius) is far too small for many modern pixel-dense devices,
requiring frontends to duplicate files locally to fix. Adding #ifndef
conditionals around these two values allow them to be configured by
the frontend without the need to fork the files.
The changes here are what I made to support my custom iOS
frontend (Puzzles Reloaded) and work well.
The mostly-unpopulated game_drawstate created in game_print for the
sake of the TILE_SIZE macro was being passed to a subroutine which
expected another field of it to be filled in.
Rather than continue filling in one field at a time, let's just make a
complete one.
A user reported that, on the default square board size, the clues were
printed in the right places, but the grid contents were accidentally
transposed by swapping the x and y coordinates. Oops!
Trying to reproduce this, my dev build was running under ASan, which
pointed out two further buffer handling bugs. The initial sprintf put
the terminating NUL one char too early in the buffer, so that it was
overwritten by the bottom right corner of the grid and no NUL ended up
in the buffer at all; also, the separate buffer for formatting numeric
clues in the left margin was sized one char too short, because the NUL
at the end of it hadn't been counted. Apparently in a non-ASan
environment both of those could work anyway by chance.
Thanks to Amir Livne Bar-on, who implemented this variant from a
description posted in the Mastodon thread following up my recent blog
post about loop-finding.
The revised algorithm has the same asymptotic complexity as the one I
already had. But it has better constant factors, and less code: it can
run in a single depth-first pass over the graph instead of three
separate passes, and it needs to store fewer variables per vertex.
This version relies on the insight that if you DFS over an undirected
graph and imagine it constructing a rooted spanning forest with each
component's tree rooted at whatever vertex you started that component
from, then every edge that the DFS visits without making it part of
the spanning forest must join a vertex to one of its direct ancestors
in that component's tree.
(Because the other options are that it joins the DFS's current vertex
to one the search hasn't visited at all yet – in which case the DFS
_would_ follow it, and make it a forest edge after all. Or else it
joins this vertex to a cousin in an earlier finished subtree – but
then when the DFS processed that subtree, it would have explored the
same edge in the other direction, and added our current vertex to that
subtree, which by assumption it didn't.)
Hence, instead of assigning every vertex a distinct integer label and
calculating the min/max label reachable from each subtree, we can
instead assign each vertex its tree depth, and simply calculate the
minimum _depth_ of vertex reachable from each subtree: if a subtree
starting at depth D can reach a vertex at depth <D, it's because
there's one of those non-tree edges to a vertex outside the subtree,
so the tree edge entering the subtree isn't a bridge.
And since every non-tree edge must point to a vertex we've already
seen (and hence assigned a depth to), this can be done in the same
pass as calculating the depths in the first place - and we don't even
need to _store_ the spanning forest we generate.
This increases the chance of detecting a bug in a simple fixed case
before a complicated random one that's harder to debug. It also allows
adding regression tests for bugs found later, if any.
This builds a secondary GUI program sharing most of the Untangle code,
similar to galaxieseditor. You can still drag points around as in the
actual Untangle game, but also you can right-drag to add or remove an
edge between two points. And the 'copy to clipboard' action generates
the Untangle game id corresponding to whatever you ended up with.
This could be used for hand-designing actual Untangle games, but my
more immediate use for it is as a convenient method of constructing
test cases for the new graph testing code.
Thanks to Amir Livne Bar-on for contributing this. It's about to be
used to build confidence in a rewrite of findloop.c in a followup
commit. I also expect that it will be useful later on for testing
other graph algorithms.
This patch is not Amir's original. I've polished the code in various
small ways. The biggest change is that when it outputs a failing
graph, the graph is printed in the form of an Untangle game id, in the
hope that that will make it possible to visualise easily while
debugging the problem!
This week I expanded that comment into a blog post:
https://www.chiark.greenend.org.uk/~sgtatham/quasiblog/findloop/
which improves on the comment in three ways:
1. diagrams
2. adds a further reason why the footpath-dsf algorithm was
unsatisfactory, pointed out by a Mastodon comment after I
published the original version of the blog post
3. adds the punchline that the loop tracing approach _could_ have
been made to work after all!
So I've deleted the comment and replaced it with a link to the article.
The text format includes newline characters that weren't being
included in the buffer length calculation. Fix the calculation and
assert before returning that the string offset matches the calculated
length.
The lack of this pair of parens triggered a compiler warning, which
turned into an error at -Werror. Oops - I thought I'd folded that fix
in before pushing Franklin's series.
I had tried to make this comment refer to the commit in which the
change in question (the new drawing API semantics) was introduced, but
something about Simon applying the patch caused its hash to change.
Now that the commit in question is in Simon's branch, it should remain
constant forever.
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'm guessing that front end authors might look here to investigate a
build failure due to the recent changes to the drawing API.
(This unfortunately needs to be a separate commit from its parent --
otherwise it would need to reference its own hash!)
This changes the drawing API so that implementations receive a
`drawing *` pointer with each call, instead of a `void *` pointer as
they did previously. The `void *` context pointer has been moved to be
a member of the `drawing` structure (which has been made public), from
which it can be retrieved via the new `GET_HANDLE_AS_TYPE()` macro. To
signal this breaking change to downstream front end authors, I've
added a version number to the `drawing_api` struct, which will
hopefully force them to notice.
The motivation for this change is the upcoming introduction of a
draw_polygon_fallback() function, which will use a series of calls to
draw_line() to perform software polygon rasterization on platforms
without a native polygon fill primitive. This function is fairly
large, so I desired that it not be included in the binary
distribution, except on platforms which require it (e.g. my Rockbox
port). One way to achieve this is via link-time optimization (LTO,
a.k.a. "interprocedural optimization"/IPO), so that the code is
unconditionally compiled (preventing bit-rot) but only included in the
linked executable if it is actually referenced from elsewhere.
Practically, this precludes the otherwise straightforward route of
including a run-time check of the `draw_polygon` pointer in the
drawing.c middleware. Instead, Simon recommended that a front end be
able to set its `draw_polygon` field to point to
draw_polygon_fallback(). However, the old drawing API's semantics of
passing a `void *` pointer prevented this from working in practice,
since draw_polygon_fallback(), implemented in middleware, would not be
able to perform any drawing operations without a `drawing *` pointer;
with the new API, this restriction is removed, clearing the way for
that function's introduction.
This is a breaking change for front ends, which must update their
implementations of the drawing API to conform. The migration process
is fairly straightforward: every drawing API function which previously
took a `void *` context pointer should be updated to take a `drawing *`
pointer in its place. Then, where each such function would have
previously casted the `void *` pointer to a meaningful type, they now
instead retrieve the context pointer from the `handle` field of the
`drawing` structure. To make this transition easier, the
`GET_HANDLE_AS_TYPE()` macro is introduced to wrap the context pointer
retrieval (see below for usage).
As an example, an old drawing API function implementation would have
looked like this:
void frontend_draw_func(void *handle, ...)
{
frontend *fe = (frontend *)handle;
/* do stuff with fe */
}
After this change, that function would be rewritten as:
void frontend_draw_func(drawing *dr, ...)
{
frontend *fe = GET_HANDLE_AS_TYPE(dr, frontend);
/* do stuff with fe */
}
I have already made these changes to all the in-tree front ends, but
out-of-tree front ends will need to follow the procedure outlined
above.
Simon pointed out that changing the drawing API function pointer
signatures to take `drawing *` instead of `void *` results only in a
compiler warning, not an outright error. Thus, I've introduced a
version field to the beginning of the `drawing_api` struct, which will
cause a compilation error and hopefully force front ends to notice
this. This field should be set to 1 for now. Going forward, it will
provide a clear means of communicating future breaking API changes.
Previously, this function's documentation just stated that `coords`
contained the X and Y coordinates of the polygon's vertices, without
specifying in what order they were listed. Thus, it would have been
reasonable to (wrongly) assume that all the X coordinates were listed
in the first half of the array, followed by all the Y coordinates.
Now it's clear that each point's X and Y coordinates are stored
directly next to each other, and in that order.
Both Inertia and Twiddle previously included static implementations of this
exact same function, which was passed to `qsort()` as a comparator. With
this change, a single global implementation is provided in misc.c, which
will hopefully reduce code duplication going forward.
I'm refactoring this in preparation for the upcoming fallback polygon fill
function, which I'm about to add.
Last November in commit 08365fb260ae6e3 I added a VERSIONINFO resource
to the Windows puzzle binaries, with three of the four integer
components of the binary version number taken from the year, month and
day of the build date. The header file #define of those integers was
made by a Perl one-liner which just split up $(!builddate) into the
right groups of digits.
But it didn't trim leading zeroes. So the build failed today because
the month component of the version number was '08', which isn't a
valid C integer literal (the leading 0 means octal, but 8 isn't an
octal digit), and presumably therefore not valid according to llvm-rc
either. I have to assume that the previous months have all worked
because 01, ..., 07 _are_ valid octal integer literals and still mean
the right things.
I'm not 100% satisfied with this explanation, because surely the same
argument applied to the day field should have meant my builds failed
on the 8th and 9th of every month since I added this code last
November! But I don't have any evidence left over to show why it
_didn't_ fail. Perhaps I've upgraded llvm-rc past a relevant bug fix
in the last month, or something.
This adds a "half-grid" cursor mode, settable via a preference, which
doubles the resolution of the keyboard cursor, so that it can be over a
square center, vertex, or edge. The cursor select buttons then toggle the
edge directly under the cursor.
There are two advantages to this new behavior. First, the game can now be
played with only the cursor keys, doing away with the need to hold Control
or Shift, which are not currently emulated on Rockbox. And second, the new
interface is arguably more discoverable than the legacy mode,
which is still retained as a user preference.
This refactors all instances of bitwise-ANDs with `~MOD_MASK'. There is
a handful of more complex instances I left unchanged (in cube.c, midend.c,
and twiddle.c), since those AND with `~MOD_MASK | MOD_NUM_KEYPAD' or
similar. I don't think it's worth writing a macro for those cases.
Also document this new macro's usage in devel.but.
The cursor keys navigate amongst the points. CURSOR_SELECT toggles dragging;
CURSOR_SELECT2 and the Tab key cycle through the points.
The cursor navigation scheme jumps to the nearest point within the quadrant
of the cursor direction; this seems to yield fairly intuitive gameplay.
Unfortunately, the "quadrant-nearest-neighbors" digraph produced by this
scheme is not necessarily fully reciprocal; that is, pressing opposite
cursor keys in sequence does not always return to the original point. There
doesn't seem to be any immediately obvious way around this.
As for connectivity (i.e. whether all points are reachable from any given
point), I could not find a counterexample, but I don't yet have a formal
proof that this is the case in general. Hence, I've added the ability to
cycle through all the points with Tab. (This will probably also be useful
in conjunction with the "Numbers" point drawing preference.)
According to
https://mail.gnome.org/archives/gtk-list/1999-August/msg00145.html
pressing Shift+Tab generates a special keyval of ISO_Left_Tab, without
a GDK_SHIFT_MASK applied. We now handle this case so that the midend
receives ('\t' | MOD_SHFT) as intended. This will be used by the
upcoming Untangle keyboard interface.
Previously, a button code of ('\t' | MOD_SHFT) from a frontend would have
the MOD_SHFT flag stripped by midend_process_key, resulting in a bare '\t'
passed to the backend. Now, this combination is allowed to pass through to
the backend. This will be used in the keyboard interface to Untangle, which
I'm about to add.
When a drag started outside the grid (or no drag has started yet),
ensure the drag state in game_ui says so and bail out accordingly.
Previously, such a drag would manipulate the tile the last valid drag
started from, if any, else segfault.
Also, allow drags that start on-grid and then go off-grid to continue
rotating.
The foosolver.exe binaries aren't delivered out of the end of
Buildscr, so there's no point wasting time on signing them. Signing is
slow in wall-clock time (you have to wait for a timestamp server), so
this should significantly reduce overall build time.
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.
