This restrict effects of paste-time de-stacking to positions under particles being pasted. If this is not done, particles beyond the paste area can be wrongfully killed, see #889.
Some operator[]s that know the size of the container they wrap like to assert(index >= 0 && index < size), which is bad for us because we sometimes use &container[size]. This is not undefined behaviour until that pointer is dereferenced, but certain operator[]s choose to ignore this fact and err on the side of caution. The solution is to use &container[0] + size instead of &container[size].
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 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.
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 made it possible to get rid of two GameSave constructors.
Also clean up Client::LoadSaveFile, Client::ReadFile, and Client::WriteFile in the process, and remove unused SaveRenderer::Render
This fixes bugs like "type\0hello mom" being a property name sim.partProperty accepts and half-fixes bugs like text formatting codes making gfx.drawText exit prematurely.
