`build_colormap_good` only took `used` to clear it. Only one caller
needed it cleared. Move the clear into that caller. Remove the
parameter from all calls.
The Windows build did not explicitly include <time.h> or equivalent, but
used `time` and `gmtime`, which are only available when <time.h> is
included. Other platforms include <time.h> incidentally.
Fixes: 131c1b9f4d ("Add support for PNG screenshots")
User TRUEpiiiicness reports that valptridx<player>::check_index_range
traps when "The Apocalyptic Factor"[1] level 14 boss fires its Omega-based
weapon. Code inspection shows that this is expected, since the original
designers assumed OMEGA_ID would only ever be used by players, and coded
various shortcuts accordingly. No one told the level author this. The
boss is an interesting concept and not difficult to support, so adjust
the code to handle this situation correctly:
- Check that the shooter is a player before checking its player ID.
Without this, a robot that happened to have the same ID as the player
would interpret robot data as player data, likely causing corruption
when it tried to update the omega cannon charge. Even if the robot ID
does not match, this step causes a diagnostic[2] reporting that a
robot ID is being misused as a player ID.
- Remove the shortcut that assumes the shooter is a player. Store the
shooter's actual type.
- Remove the unnecessary and counterproductive path that:
1. Uses the object pointer to get the player's ID
2. Uses that player ID to get a player pointer
3. Uses that player pointer to get an object number
4. Uses that object number to get an object pointer
When invariants are maintained, the pointer derived in step 4 is equal
to the pointer used at the start of step 1. Use that pointer directly
instead of rederiving it. The reported exception was due to step 2,
which requires that the player ID is in range. When the shooter is a
player, this is true. When the shooter is not a player, it may not
be true.
[1] http://www.enspiar.com/dmdb/viewMission.php?id=418
```
sha1sum af.hog af.mn2
133c52fb4b4e5fd40bf7b2321789841b727d1d0b af.hog
0bb5f0dd1803b0fc1aac7c2023eacc97e9ab872b af.mn2
stat -c '%s %Y %n' af.hog af.mn2
4024838 1094445016 af.hog
566 1013524628 af.mn2
```
[2] `similar/main/laser.cpp:560: BUG: object 0x555556078958 has type 2, expected 4`
Reported-by: TRUEpiiiicness <https://forum.dxx-rebirth.com/showthread.php?tid=1038>
AddressSanitizer in gcc-7 inhibits deletion of certain otherwise
unnecessary nullptr checks, causing otherwise absent references to
class null_pointer_exception. As a convenience to AddressSanitizer
users, instantiate all valptridx templates when AddressSanitizer is
enabled. This is slightly excessive, but users building for
sanitization are not aiming for a minimal size build anyway.
AlexanderBorisov reported[1] a broken demo that ultimately traced to
incorrect reuse of stale data. The campaign[2] includes per-level robot
definitions for level 3. Descent incorrectly accessed Robot_info before
reloading it for the new level, so it used the level 3 data on level 4
robots during a warm start of level 4 for regular play, but used only
the level 4 data for a cold start of level 4 for regular play and for
playback of a demo recorded solely on level 4. The particular
customizations applied on level 3 caused a level 4 demo recorded with
level 3 robot customizations to use different record lengths than a
level 4 demo recorded with level 4 robot customizations. Since demos do
not record their record lengths, mismatched record lengths lead to
effectively corrupt demos. In this case, it manifested as writing 8
anim_angles (per model_num 108, the model incorrectly inherited from
level 3) instead of 1 anim_angles (per model_num 37, used only on cold
starts). The extra anim_angles were then misinterpreted as a run of 42
ND_EVENT_EOF, because the extra angles were unused and the newdemo code
defines the null byte as EOF (separate from receiving an actual EOF
indicator from the file I/O library). That run of spurious EOF caused
the demo playback code to refuse to play past the spurious EOF,
resulting in a seemingly broken demo.
As an unfortunate, but unavoidable, consequence, this change modifies
the common path (warm playthrough of every level in the campaign) to
work like the uncommon path (cold start of each level). Further, the
affected robot triggers a trap in d2x::do_silly_animation[3] when using
the correct model_num, because that polygon model has too few models for
the joints used by the robot. When using the incorrect level 3 data,
the robot animates without trapping.
[1] https://forum.dxx-rebirth.com/showthread.php?tid=1023
[2] http://www.enspiar.com/dmdb/viewMission.php?id=212
```
sha1sum ENTROPY.HOG ENTROPY.MN2
7bc7a12d00a1ddd3ae92ce90eb67d581ecab004a ENTROPY.HOG
f2688a634f22b30a02c43d8fa1a049deb5a03f70 ENTROPY.MN2
stat -c '%s %Y %n' ENTROPY.HOG ENTROPY.MN2
2402638 875757712 ENTROPY.HOG
511 875757164 ENTROPY.MN2
```
[3]
```
799 if (jointnum >= Polygon_models[objp.rtype.pobj_info.model_num].n_models) {
800 Int3(); // Contact Mike: incompatible data, illegal jointnum, problem in pof file?
801 continue;
802 }
```
gcc silently permits this, as does every clang version I tested.
However, in issue #360, thfrwn referenced an e-mail thread that claims
OpenBSD clang warns for this. [1] This capture is not needed, and as
far as I can tell, was not needed even when it was added. It was likely
required in an earlier iteration of the code, but should not have been
pushed. Remove it.
[1] https://marc.info/?l=openbsd-ports&m=151402743101761&w=2
Fixes: d4df692d30 ("Use range_for in draw_all_edges")
Presently, user profiles are not portable between a build with
max_axes_per_joystick=0 and one with max_axes_per_joystick!=0.
Attempting to do so causes some buttons to be assigned to the wrong
action. Since the intended use case for max_axes_per_joystick=0 and
similar is to completely remove joystick support on a system that will
never use a joystick, this is acceptable for now. A future overhaul to
improve storage of user kconfig settings may fix this.
hengyn45 reports that D1 improperly allows cooperative games on anarchy
only maps.
D1 seems not to have implemented this for a very long time, if ever,
although the code existed in commented out form. When the trees were
combined, the commented out form was treated as blank, so the
restriction was placed inside a #if-d2 guard. There is no reason to
make this D2-only. Remove the guard so that D1 enforces anarchy-only.
Reported-by: hengyn45 <https://forum.dxx-rebirth.com/showthread.php?tid=1017>
vclip::frame_time equal to 0 would cause an infinite loop.
vclip::frame_time less than 0 would cause the loop to run until
underflow, which could be very slow if frame_time is near 0.
- Enable display of subsecond (Linux: microseconds; Windows:
milliseconds) precision on gamelog timestamps.
- Add disabled support for YYYY-MM-DD leaders on gamelog timestamps.
Activate it by defining DXX_CONSOLE_TIME_FORMAT_YMD to true.
- Add disabled support for capturing caller __FILE__, __LINE__ in calls
to con_printf, con_puts. Activate it by defining
DXX_CONSOLE_SHOW_FILE_LINE to true. If captured, write those to
gamelog after the timestamp and before the text. This feature (and
only this feature) requires that DXX_HAVE_CXX_BUILTIN_FILE_LINE be
defined, which is conditional on if the compiler has __builtin_FILE()
and __builtin_LINE(). If the compiler lacks this support, attempts to
enable this feature are ignored.
- Switch to using GetLocalTime on Windows.
It generates code different from the current definitions, even when no
logical changes are present. This code has not been used in at least 2
years, since commit bc7c469ab2 ("Use
array<> for more globals") changed the definitions of some variables
that this code regenerates, and this code still generates the old
definition. Future changes will cause further deviation.
Commit 7d36df315e restructured
draw_weapon_vclip, but unintentionally broke the spin of proximity bombs
because `modtime` could exceed `play_time * 2`, causing
`draw_vclip_object` to clamp `bitmapnum` to the last frame of the
animation. Switch from `-=` to `%=` so that modtime is always brought
back in range.
Reported-by: Descender1032 <https://forum.dxx-rebirth.com/showthread.php?tid=1005>
Fixes: 7d36df315e ("Simplify draw_weapon_vclip")
When `apply_force_damage` was called to damage a robot, it tested if the
`ctype.laser_info.parent_signature` of the other object matched
`ConsoleObject->signature` and, if so, awarded points to the console
player. This allowed the player to sometimes get points for ramming a
robot to death, but was buggy. When the player rams a robot, the player
is the other object, so accessing `ctype.laser_info` is wrong.
Historically, the player's signature was zero and player
`ctype.laser_info.parent_signature` (an inactive and therefore incorrect
branch of the union) happened to be zero, so the test succeeded. Commit
44753209d6 changed the layout of
`struct player_info` such that
`ConsoleObject->ctype.laser_info.parent_signature` was not zero, causing
the player not to be his own parent. Prior to this commit, the player
could still wrongly fail the test if the `player_info` member that
aliased `laser_info.parent_signature` was nonzero. In the commit
preceding the obvious break, `player_info.player_flags` aliased
`laser_info.parent_signature`, so a player with certain flags (such as
invulnerability) would wrongly fail the test. The exact manifestation
has changed over time as `struct player_info` gained members.
Restructure the test so that a ramming kill always awards points,
without checking ancestry.
Reported-by: Descender1032 <https://forum.dxx-rebirth.com/showthread.php?tid=1006> (no commit identifiers given)
Fixes: 44753209d6 ("Move homing_object_dist to object.ctype.player_info")
`Descent 2: Counterstrike` level 9 specifies a control center trigger on
a side with no wall. This is not valid. In normal play, wall_toggle
silently ignored this, and newdemo_pop_ctrlcen_triggers had undefined
behavior. Add a warning in wall_toggle to report levels like this. Add
handling in newdemo_pop_ctrlcen_triggers to detect and suppress this
bogus data.
`Descent 2: Counterstrike` level 11 specifies a control center trigger
on a side with no child segment. This is not valid, so
newdemo_pop_ctrlcen_triggers again had undefined behavior. Add handling
to detect and suppress this bogus data.
Reported-by: Descender1032 <https://forum.dxx-rebirth.com/showthread.php?tid=992>
clang rejects 0x80000000 as a non-type template argument of type `int`
due to narrowing. Use `INT32_MIN` instead, which has the same numeric
value, and should be accepted without requiring narrowing.
Reported-by: Havner <https://github.com/dxx-rebirth/dxx-rebirth/issues/353>
Fixes: 88832e3679 ("Use constexpr integral_constant for various magic numbers")
Global variable `grd_curcanv` is set to a variety of canvases, some of
which are local stack variables. Use of global variables in this way is
fragile, but works as long as the global is not used beyond the life of
the backing local.
Unfortunately, some existing uses do access the canvas beyond the
lifetime of the backing local. Playing movies sets the font of the
current canvas. If the current canvas is an expired stack variable,
setting the font overwrites other stack data. This data corruption
causes various symptoms, such as inability to play the escape tunnel
movie.
Prior to 03cca2b3dc, the corruption on
playing the endlevel movie had no user-visible effect. That commit
created a large local variable, which changed stack layout. Starting
with that commit, the corruption causes the movie to play as all black.
Fix this, and protect against some other data corruption possiblities,
by clearing the global when the local goes out of scope.
Reported-by: Havner <https://github.com/dxx-rebirth/dxx-rebirth/issues/345> (only as cutscene failure to play, not as the underlying corruption issue)
Rather than use an inline wrapper and rely on the compiler optimizer to
redirect gr_set_current_canvas(nullptr) to gr_set_default_canvas,
rewrite all relevant calls directly in the source.
git grep -l 'gr_set_current_canvas' | xargs sed -i -e 's:gr_set_current_canvas(\s*NULL\|nullptr\s*);:gr_set_default_canvas();:'
Defining SLEW_ON breaks the build due to use of member variable names
not used since before the D2X import in 2001.
`unifdef -USLEW_ON -m -- similar/main/endlevel.cpp`
This code was part of a feature abandoned before retail. It cannot be
usefully used in campaigns. Remove it to reduce code size and simplify
later changes.
Descent 1 bosses could always teleport, but were only placed in large
areas where free teleporting was always permitted.
Descent 2 boss 1 was placed in a confined segment and not permitted to
teleport out of it until it was opened. This was implemented by a
two-part change relative to Descent 1 rules:
- Descent 1 bosses were always permitted to teleport to any teleport
destination segment. Descent 2 bosses were only permitted to teleport
if the player was visible or the boss had recently been hit.
- When computing the permitted list of teleport destination segments,
Descent 1 used directly connected accessible segments, then stopped.
Descent 2 started with this rule, but if the list had at most 1 entry,
then it would assume this is the confined boss and recompute the list
with the first wall ignored. This recomputed list allowed the boss to
teleport to any segment in the larger arena outside its starting
segment.
After D2X-Rebirth support for emulating Descent 1 bosses was enhanced in
28bd4c1650, Descent 1 bosses gained the ability to teleport out of a
confining cube early, but only in D2X-Rebirth when emulating Descent 1.
In D1X-Rebirth, when a boss is placed in a confining cube, it can always
teleport, but only to that confining cube. In D2X-Rebirth, Descent 1
bosses retain the always-teleport rule of Descent 1, but gained the
Descent 2 rule for expanding its search to the surrounding arena. It
should only use the expanded search when it also abides by the Descent 2
restriction not to teleport before the first wall is opened. It did not
abide by that rule. This commit adds that restriction for Descent 1
bosses.
Reported-by: ef314159 <https://github.com/dxx-rebirth/dxx-rebirth/issues/348>
Fixes: 28bd4c1650 ("Enable D1 boss behavior in d2x build. So we get correct boss behavior when emulating D1, and 3rd party mn2s can include D1 bosses.")
Commit b1b5de4 switched from enabling select physics flags to setting
those flags and clearing all others. Unfortunately, flag PF_USES_THRUST
was omitted from the enabled list, so it was disabled on reset. It is
required to let player ships move.
Reported-by: Sottises <https://github.com/dxx-rebirth/dxx-rebirth/issues/347>
Fixes: b1b5de4297 ("Additional safeguard for bug #306")
Editor groups write `struct segment` to a file in raw form, but had no
code to enforce that this raw form remained stable over time. Various
changes to `struct segment` have repeatedly changed its internal
structure. Each change created an incompatible dialect of the editor
group file, and all the dialects share the same version number.
According to
```
git log -p -L'/struct segment {/,/};/:common/main/segment.h' HEAD --not 0.58.1-d1x 0.58.1-d2x --
```
`struct segment` changed layout in:
* d1c6b89f17 ("Move dsx::segment -> dcx::segment") [D1 only]
* 596ecbb38d ("Rename segment::value to segment::station_idx") [D1 only]
* 6f10a67c09 ("Move segment::sides to end")
* c53b734abb ("Compute slide segments early") [D2 only]
* 40e90fea22 ("Move Light_subtracted[] to Segments[].light_subtracted")
* a65d774c83 ("Improve packing of struct segment")
* c70c6c98b3 ("Remove obsolete segment::degenerated flag")
Mark editor groups as broken to avoid making the mess worse. If anyone
cares about group support, it needs to be rewritten not to depend on the
internal layout of `struct segment`.
Descent 2, but not Descent 1, had a useless test in render_side:
type == QUAD ? 0 :
type == TRI_13 ? 1 :
0
This is useless since, if type is not QUAD, the second expression will
apply. If type is QUAD, then the type is not TRI_13, so the second
expression would choose the same result as the first. The extra
comparison does not save any work, so it is useless. Remove it.
Commit 1335af4b51 restructured briefing number parsing, but
unintentionally changed the parser not to drop the newline after a
number. The briefing parsing code is very sensitive to minor changes,
so this broke parsing some briefing screens, including Vertigo briefing
screens after the first. After the change, clicking through the Vertigo
briefing aborts on failure to load a PCX file.
Fix this by consuming the newline before returning. This matches the
previous semantics.
Reported-by: Havner <https://github.com/dxx-rebirth/dxx-rebirth/issues/343>
Fixes: 1335af4b51 ("Simplify get_message_num")
Like many things in the main game loop, lavafall handling was
historically done on a per-frame basis, then its effects were scaled to
FrameTime to normalize the results. Ignoring rounding errors, this
produced roughly equivalent damage for high framerate users (who
experienced many but small damage hits) and low framerate users (who
experienced few but large damage hits). However, the randomized
movement was not scaled to FrameTime, which caused differing results for
high framerate users versus low framerate users. Commit b36c6f20c7
tried to fix this by forcing scrape_player_on_wall to run at a capped
maximum effective frame rate, then scaling the damage in
check_volatile_wall accordingly, so that high framerate users would
experience fewer damage hits, but the ones they received were larger,
thus maintaining approximately the same damage as before.
Prior to b36c6f20c7, damage was always scaled to FrameTime. In
b36c6f20c7 and later, damage scaled to max(FrameTime,
DESIGNATED_GAME_FRAMETIME), causing users with a high frame rate (and
thus low FrameTime) to take more damage on each pass. This damage
increase was balanced by an added hack in scrape_player_on_wall to limit
the frequency of the scrape so that high framerate users would skip some
scrapes, giving them a virtual frame rate appropriate to
DESIGNATED_GAME_FRAMETIME. However, the damage is only balanced if the
new governor is used consistently. It is not used consistently, so it
caused a regression for passable lava surfaces. Scraping on a solid
lava surface goes through scrape_player_on_wall and respects the
governor. Touching a passable lava surface (only available in Descent
2) bypasses scrape_player_on_wall and jumps directly to
check_volatile_wall, thereby bypassing the governor. This allows high
framerate users touching a passable lava surface to take many hits (as
they always did), but not receive the full benefit of downward scaling
the damage (as they once did) due to the new max() expression. Thus,
they are damaged frequently, but still take damage consistent with being
damaged infrequently.
Fix this by moving the hack from scrape_player_on_wall to
check_volatile_wall, so that both solid lava surfaces and passable lava
surfaces respect the governor. The governor is still an ugly hack that
should not be global, but this is a spot fix for the immediate problem.
Fixes: b36c6f20c7 ("Made scrape_player_on_wall() based on a timer. Due to the player being pushed away from the lava/water surface in every frame in a random vector (wrong, too), player movement per frame was not enough to counter this on FPS rates > ~120 which made damage scaling per frame nonsensical in these situations. Instead, execute scrape results in intevals based on DESIGNATED_GAME_FRAMETIME (or per frame if FrameTime>DESIGNATED_GAME_FRAMETIME) which fixes the issues and generally works much better for the purpose of this function.")
Commit f4b21088a0 ("Track vulcan ammo explicitly") fixed an original
retail bug that prevented the thief from stealing energy weapons,
because the thief could only steal weapons for which the player had ammo
and energy weapons never have ammo. This went unremarked for several
years, until a recent report of the new semantics as a game-breaking
regression because the thief is now "ridiculously potent".
Address this report, as well as an intermittently raised issue from
various users over time, by adding two new knobs to both the single
player "Gameplay" menu and the multiplayer setup screen: "Remove Thief
at level start" and "Prevent Thief Stealing Energy Weapons".
"Remove Thief" deletes the thief object during level load. It has no
impact on save games, and changing it after entering a level has no
effect on any thief already in the level.
"Prevent Thief Stealing" is checked at the moment of theft and, when
enabled, prevents stealing primary weapons other than Vulcan/Gauss.
This can be changed at will in single player and is immediately
effective. In multiplayer, this option can only be changed by the game
host in the pre-game setup.
For both knobs, there is one pair of checkboxes to control this as a
player preference, which applies in single player games. There is a
second pair of checkboxes in the multiplayer setup, which applies only
to multiplayer games. Therefore, in multiplayer, the host chooses thief
settings and all clients use the host's choice. The host may configure
the thief differently in multiplayer from how the host plays in single
player.
For users who wanted to remove the thief, no specific tally has been
kept for who requested it or when. Now that the code is being updated,
this is thrown in as an easy addition.
Reported-by: MegaDescent <http://forum.dxx-rebirth.com/showthread.php?tid=980> (for the thief stealing energy weapons as a game-breaking regression)
When reporting use of SDL_mixer, report its loaded version. When
running in verbose mode, report both the compile-time and load-time
versions of PhysFS, libSDL, and SDL_mixer.
Some Linux libraries print their own messages to stdout/stderr,
particularly in case of severe errors. Decorate messages generated by
Rebirth to distinguish them from library generated messages.
For specific blacklisted renderers, add a message informing the user
that the blacklist matched and changed settings.
Only one caller exists, and that caller alway passes a non-nullptr
value. Switch to a reference and remove the unused special case to
handle a nullptr input.
Most uses pass an orientation matrix. All sites are deterministic about
whether a matrix is passed. Make the matrix mandatory for sites that
passed it, and split out a separate version of g3_start_instance_matrix
for the 2 sites which do not provide orientation.