I've seen the nullptr deref, but I don't see how it's possible at all. A condition for file->LazyUnload to be called is for SaveButton::wantsDraw to be false, but for that to happen, SaveButton::Tick has to be called after a call to SaveButton::Draw to reset it to false, and then *again* for it to see it being false on entry.
Whatever, the bug is genuinely there and is bad, and easy to fix, no need to figure out in what way it is bad exactly.
So instead of loading every save in sight and rendering the thumbnails for them too, SaveButtons will only do this when they are actually visible, and unload saves and thumbnails when they are not.
Also remove the "Rendering thumbnails" progress bar, which did absolutely nothing.
I took extra care to not mess up signedness in readOPS in ab600780d0, but apparently didn't do the same in readPSv.
Also fix a bound check that was broken since aac6b7258c. It's a good thing this was broken, because this allowed negative type values from broken signedness readPSv to get past and cause a crash later on, rather than just cause particles to disappear or something.
When PHOT fails to move (do_move or eval_move return "no move"), it looks for
a surface (a contour of boundaries, as reported by is_boundary) along its path
and reflects off (or refracts into, see below) it, using get_normal_interp to
find the point of incidence and get_normal to deduce the surface normal.
get_normal is given the point and angle of incidence, and attempts to traverse
the surface the point belongs to by running two "surface scout" processes.
These processes remember their own position and "heading", a subset of the
eight cardinal directions on the grid. They are initialized with the point of
incidence and a heading that includes all directions whose dot product with
the angle of incidence is non-negative (see direction_to_map). They then
perform a few iterations (SURF_RANGE).
In each iteration, the processes check all eight neighbours of the cell they
are on and select the first neighbouring cell they find that is both a
boundary (as reported by is_boundary) and that is within their heading. They
then move to this neighbouring cell and update their heading by discarding
directions that are not similar enough to (differ by more than 45 degrees
from) the one that took them where they are now (see find_next_boundary). If
they find no such neighbour, they stop.
Continuing the militaristic line of thinking introduced by the term "surface
scout", you can imagine the two processes as two paratroopers who arrive from
above, land on a horizontal surface, and one starts going left, while the
other starts going right. They initially expect the surface they land on to be
close to horizontal, but are also prepared for not too erratic changes in its
angle as they go. Changes too erratic (imagine a precipice) scare them and
force them to stop.
Once the processes finish, an imaginary line segment is drawn between the
cells they ended up on. If the line segment is long enough (estimated by j,
and compared against NORMAL_MIN_EST), get_normal returns a normal that is
perpendicular to it. If it is too short, get_normal gives up and returns
nothing (which results in the PHOT being killed).
This amounts to our paratroopers attempting to get the "lay of the land" by
walking away from where they landed and comparing where they end up. They also
know that if they are still relatively close to each other at the end of their
walk, their measurement is probably wrong and their mission should be aborted.
The bug this commit fixes is that get_normal returns bogus surface normals
when it encounters thin walls of particles, defined as walls exactly two
layers of particles thick. One-layer walls are not really walls, as movement
code allows particles to penetrate these, and three-layer and thicker walls
are too thick for the bug to manifest.
The bug manifests for two-layer walls because the "left" scout process is
drawn to the side of the wall opposite to the one with the point of incidence.
This is because scout processes check neighbours in a clockwise order, and
always select the first suitable neighbour they find. As particles on the
other side of the wall are both boundaries and are within the heading of the
processes, they also qualify as suitable neighbours, so whether a scout
process selects the correct side of the wall depends on the order in which
neighbours are checked.
Essentially, the paratroopers look at their immediate surroundings in a
clockwise order. The right paratrooper always finds the ground and knows where
to step. The left paratrooper finds the Upside Down from Stranger Things and
teleports there.
This bug also affects refraction into and out of thin walls, but since these
walls are thin, the path the PHOT takes inside them is rather short and the
incorrect angle of travel is difficult to see. Furthermore, upon exit, the
same normal deduction bug causes the PHOT to take a path whose angle is almost
identical to that of the path that took it to the wall, so much so that it is
also difficult to see over shorter distances.
The solution is to have the left scout process check neighbours in reverse
order, so that it prefers the right side of the wall over the wrong one. This
does not affect its behaviour when facing thicker walls, but fixes its
behaviour when facing two-layer walls.
The changes in this commit also make find_next_boundary interact with
is_blocking directly to detect a change between the blocking trait of
immediate neighbours. This makes more sense than relying on is_boundary
because find_next_boundary is meant to find a transition from non-blocking to
blocking neighbours within the current heading, rather than to find any
boundary particle. The difference is subtle but important.
Namely:
- get rid of unsafe memory management;
- use vectors / Planes everywhere;
- return a vector from serialization functions;
- have read functions take a vector;
- improve constness;
- hide a few implementation details from GameSave.h;
- get rid of GameSave copy constructor;
- better member initialization;
- use the slightly more C++-looking BZ2 wrappers.
The BSON library still takes ownership of the data it parses, and GameSave
ownership is still a joke. Those will need to be fixed later.
... and everything built around them.
A GameSave would hold at least one but sometimes two representations of a save:
one serialized, and one "friendly", accessible for modification. Thus, a
GameSave would have three states:
- "Collapsed": only the serialized representation was present; this was the
initial state of GameSaves loaded from files;
- "Expanded With Data": both the serialized and the friendly representations
were present; this was the state of GameSaves loaded from files after a call
to Expand;
- "Expanded Without Data": only the friendly representation was present; this
was the initial state of GameSaves being prepared for being saved to files.
A GameSave would be able to go from Collapsed to Expanded With Data with a call
to Expand, and back with a call to Collapse. Of course, this latter transition
would discard any changes made to the friendly representation, for example with
Translate. A GameSave would however be unable to go from Expanded Without Data
to any other state; a call to Collapse in this state would have been a no-op.
There were two instances of Collapse being called, one in the GameSave
constructor taking the serialized representation, immediately after a call to
Expand, and another in SaveRenderer, which would Collapse a save "back down" if
it had originally been Collapsed. Now, consider that there reasons for
constructing a GameSave from the serialized representation are as follows:
- loading an online save at startup from the command line;
- loading a local save at startup from the command line;
- loading a local save when it is dropped into the window;
- loading a local save for placement of the most recently used stamp;
- loading a local save for stamp placement via Lua;
- loading an online save for preview generation while browsing;
- loading a local save in the stamp browser for thumbnail generation;
- loading a local save in the local save browser for thumbnail generation.
In some cases, the friendly representation is needed for thumbnail generation
by ThumbnailRendererTask. ThumbnailRendererTask operates on its own copy of the
GameSave, because it runs SaveRenderer on a thread different from the main one
and cannot be sure of the lifetime of the original GameSave. It destroys this
copy when it is done rendering, so the call SaveRenderer makes to Collapse is
pointless.
In all other cases, the friendly representation is needed immediately. In some
of these, SaveRenderer is used from the main thread, but since the friendly
representation of the GameSave will be needed for pasting anyway, the call
SaveRenderer makes to Collapse is pointless again.
So, Collapse goes away. This also means that it is pointless for GameSaves to
hold on to the serialized representation, since in all cases in which they have
access to it, the friendly representation is needed immediately, and with
Collapse gone, they will never need it again.
Also make ENFORCE_HTTPS optional, but default to enabled, so unencrypted HTTP is disabled by default, and require it to be enabled for release binaries.
Also restore concurrent connection / stream counts, and fix a bug that would cause AvatarButtons to try to fetch avatars before they knew what name they belonged to. I apparently broke this in the first PNG commit.
Also write PNGs with libpng, and BMPs with SDL, and have the renderer only generate a large PNG thumbnail, and disable HTTP/2 multiplexing for now so we don't get banned when loading avatars.
simon pls reply to the stupid emails already.
Also make it and render get built as dynamic executables on ghactions, and disable dynamic+LTO, as it seems to be buggy in certain cases and isn't an important case.
Also Disallow linking against non-C++ system Lua, unless configuring with -Dworkaround_noncpp_lua=true, add -Dworkaround_elusive_bzip2 and friends, and get rid of the -image_base hack for macos.
I really don't like how the only way to return with an error from ParseFloatProperty is via an exception >_>
Also do a range check on airTemp only if isValid is true, otherwise it's uninitialized.
All of these are options that are specific to our ghactions builds and not something we want downstream and package maintainers to be locked into using.
This website API was created to enable TPTMP to prove the identity of connecting users, and while TPTMP works fine without explicit support for this from the game, it has to resort to parsing powder.pref. This is not only ugly but also likely to be disallowed by the next version of the script manager. This new script manager will probably come after 97.0, so it's okay for it to rely on a game feature that won't be available until 97.0.
get_normal_interp is given a PHOT and scans a line section starting from the PHOT, extended in the direction of its velocity, to determine the surface normal of the surface that is reflecting the PHOT. It uses is_boundary to detect "boundaries" on this line section, defined as one "blocking" cell with at least one "non-blocking" non-diagonal neighbour. It never actually makes sure the position it passes to is_boundary is within the simulation area, so I assume is_boundary is expected to handle this correctly.
Plot twist: it does not. It delegates checking whether a cell is "blocking" (defined as something PHOT would normally fail to move into as per eval_move, but GLAS and BLGA are handled specially) to is_blocking. is_blocking returns true for cells beyond the simulation area (as eval_move returns "no move" for such cells). Once is_boundary sees that the cell it's been given is blocking, it then proceeds to check whether any of its non-diagonal neighbours might be non-blocking. In most cases, non-diagonal neighbours of an out-of-bounds cell are also out of bounds and are thus blocking, but if the cell is_boundary is given is in the innermost layer of out-of-bounds cells, just beyond the simulation area, then it has a neighbour that is in bounds, and that one may not block PHOT. The takeaway is that out of bounds cells can indeed be boundaries, as far as is_boundary is concerned.
This is a problem because get_normal_interp's line section can easily reach beyond the simulation area if the PHOT's velocity is high enough, and if it finds a boundary along this line section, it immediately stops looking and passes its position to photoelectric_effect, which then uses this potentially out-of-bounds position to index pmap. A position with y = -1 causes photoelectric_effect to read from the last few slots of parts data that it then interprets as pmap entries, which then may direct it to particles to spark that are beyond parts. This eventually crashes.
This commit doesn't fix is_boundary's definition of boundaries, but it stops get_normal_interp looking at cells beyond the simulation area.
The crash is difficult to reproduce because there have to be many particles in the simulation for the very last slots of parts to be in use, and for them to point to memory that isn't accessible. PHOT also has to survive a try_move beyond the simulation area first (otherwise the reflection code isn't even run), which requires it to start from EHOLE. I have no idea why this is so. Reproduce the out of bounds read in photoelectric_effect with
break Simulation.cpp:2934 if nx < 0
in gdb and executing the following Lua code:
sim.clearSim()
tpt.set_wallmap(55, 44, 12)
local i = sim.partCreate(-1, 223, 178, 31)
sim.partProperty(i, "vx", -300)
sim.partProperty(i, "vy", 0)
sim.framerender(1)
This is how we'll handle systems where the cert bundle and cert directory is stored where mbedtls doesn't expect it.
Also update tpt-libs to get new curl and mbedtls.
Both the debug and release jobs would upload artifacts, with the same name, which ghactions of course accepted without question. Snapshot 240 happens to have been released correctly (so all binaries are release builds), but that's just luck. I'd thought I'd fixed this problem with another commit, but turns out I hadn't.
Also factor out a bunch of variables so I won't mess up in the future.
Command line is limited in length on windows so meson/ninja puts it in a response file and passes that to the linker instead. Only problem is, ninja -v displays only the part of the command line where it tells link.exe where the response file is, not what's in the response file. So I just had the script print it. Care must also be taken for the script to properly fail if ninja fails, but not before the command line is printed.
Also add /GL and /LCTG to msvc command lines because they can't hurt. Right?
