Descent 2: Vertigo level 10 has invalid data in its control center
triggers. Sanitize invalid data at load to avoid problems in
memory-poisoning builds.
`oldsegnum` is `const`, so the address computed from it cannot change.
Lift the computation of `auto &children` out of the loop.
Reduce the scope of `biggest_val`, since it does not need to outlive the
loop.
Introduce a helper to obtain both the magnitude of a vector and the
corresponding normalized vector. Use it to capture both values as const
when possible.
gcc-11.3.0 issues a -Wmaybe-uninitialized warning for `exit_side`.
Reorder the code to eliminate this warning. Also, fix a potential
out-of-bounds read if `matt_find_connect_side` returned `side_none`.
Before, this would lead to a read of `Side_opposite[side_none]` before
the test that `entry_side` is not `side_none`. Now, if
`matt_find_connect_side` returns `side_none`, the read of
`Side_opposite` is skipped and logic goes directly to finding a proper
exit side.
gcc-11.3.0 with -Og may issue a -Wmaybe-uninitialized warning for `w1`
because it fails to prove that every path sets the variable. Reorder
the logic to use a local lambda to ensure that every path assigns a
value.
`Multi_is_guided` is only enabled in one place. One code path set up
Guided_missile[] for the local player, and a different path handled
network players. Remove `Multi_is_guided` and rearrange such that a
single site handles both local and network players.
When the host exits the game, guests switch the game window to be
non-visible, and raise a dialog box stating `Host has left the game!`.
Prior to this change, the guests would then busy loop until the dialog
was dismissed. With this change, the game will sleep for each idle
event, minimizing CPU use during the period waiting for the player to
acknowledge the dialog box.
Using `sav_objnum` caused the player to be restored into the object slot
of the pre-load level, in the object table of the post-load level. This
was harmless when the slots were the same, and wrong when they were not.
It became noticeably wrong when more player data was moved into the
object. After those changes, the player's data was stored into the
correct post-load slot, but the ship was assigned to the pre-load slot.
Remove the incorrect slot switch, and add an assert that the object
being overwritten is a player ghost. With the incorrect switch present,
this assert will catch most (but not all)[1] cases of the incorrect
restore. With the incorrect switch removed, this assert succeeds.
[1] In the unlikely case that the post-load slot was a player start, but
for a different player, the assert could succeed despite the slot
mismatch bug. In the common case, the post-load slot would not be a
start of a different player; it would either be the correct player slot,
in which case `sav_objnum` was harmless, or it would be some other
object, such as a robot.
If the guest is slow to respond, the host would enter a busy loop
polling for the guests to be ready. Mitigate the CPU load by sleeping
for 50ms when guests are not yet ready.
Remove the multi_do_fire branches that check the message type. Delegate
those checks to the caller, so that multi_do_fire can work with the
common initial sequence of the three messages.
Descent 2 secret levels allow a player to rescue hostages that are not
counted in `total_hostages`, so a player can exit with more hostages
saved than were in the mine. Change the scoring logic not to penalize a
player for saving these unaccounted hostages.
In non-memory-poison builds, the zero initialization of the new object
will suffice to cover this. In memory-poison builds, the new object
will be reset to a poison value, so the member must be given a
reasonable value here.
Convert the RAIIsocket to a simple SOCKET before passing it to FD_SET.
Otherwise, the build fails with:
```
similar/main/net_udp.cpp: In function 'int {anonymous}::udp_general_packet_ready(dcx::{anonymous}::RAIIsocket&)':
similar/main/net_udp.cpp:969:2: error: use of deleted function 'bool dcx::{anonymous}::RAIIsocket::operator==(T) const [with T = long long unsigned int]'
969 | FD_SET(sock, &set);
| ^~~~~~
similar/main/net_udp.cpp:493:29: note: declared here
493 | template <typename T> bool operator==(T) const = delete;
| ^~~~~~~~
```
- Hopefully fixes issues with poor quality resampling when using SDL_AudioCVT
- Converts 11025 Hz or 22050 Hz samples to the default 44100 Hz outputs
- Uses high order brick wall filter like Sound Blaster 16
The kill messages have different lengths and conditional processing
based on whether the message is MULTI_KILL_CLIENT or MULTI_KILL_HOST.
Split the two into separate functions to simplify the implementation of
each.
Compiler error messages are generally better when reporting a misuse
that fails a requires() versus reporting a misuse that fails a
std::enable_if. In some cases, this also makes the code clearer, and
avoids the need for dummy template parameters as a place to invoke
std::enable_if.
The caller has the player object, and can provide the orientation matrix
from it. Use that instead of letting multi_send_fire recompute the
object in order to get the matrix.
When the player fires a generic secondary, the choice of proximity bomb
versus super-mine is never read. When the player specifically drops a
bomb, only that choice needs to be read. Pass the chosen weapon as an
argument, and avoid computing the choice on the path where it is never
read.
Two paths are always local. One path is always remote. Split the
handler function to remove the locality test and rely on the caller's
knowledge of whether the affected player is local.
Resolve the object to a vmobjptridx once, and use it as needed. Replace
some uses of ConsoleObject with the local player, since those should be
the same, and local player is already computed.
Only one caller passes a value other than `object_none` for
`is_bomb_objnum`. That one caller uses an equivalent predicate to the
one that `multi_send_fire` uses for deciding whether to use
`MULTI_FIRE_BOMB`. Therefore, if `is_bomb_objnum != object_none` is
true, the weapon is guaranteed to be a proximity bomb or smart mine, and
`multi_send_fire` can use `MULTI_FIRE_BOMB` without retesting the object
id.
Commit 81dd23b151 ("Only charge player for weapons that fire successfully")
added an early return when `objnum == object_none`, so if execution
reaches the call to `multi_send_fire`, then the object is guaranteed to
exist. Access the track_goal without rechecking whether the object
exists.
Instead of sliding the entire queue down one element at a time, move the
non-expired elements in one step, and wipe all the trailing elements
in a single pass.
The only values for any given player are true/false, so use a bitset
instead of a byte per player. Some implementations may use a uint64_t
internally, negating the space savings. Even so, this version is
an improvement for requiring the use of playernum_t as an index.
The function had one caller, which always passed `0`. Inline that.
Next, observe that `sent` is only ever modified `if (!(Game_mode &
GM_NETWORK))`. As a multiplayer function, `multi_send_robot_frame`
should never be called in single-player, so this test should always be
false. Delete it, and observe that `sent` is now `const`.
Remove unused return value of `multi_send_robot_frame`.
Instead of passing a truth value and letting
multi_send_robot_position_sub pick which of two priority values to use,
let the callers pass the priority directly. There were two callers.
One used a fixed truth value, and the other can easily use a ternary.
Instead of passing a bare `int` named `secret_flag`, define it as an
`enum class : uint8_t` to name the two special values.
Rework the passing of this value, to deal with some confusing
inconsistencies when reading the code.
Before this change:
In D1:
- Multiplayer will always go to the secret level, regardless of which
exit door the player used.
In D2:
- Flying through a D1 secret exit in multiplayer shows the on-HUD error
"Secret Level Teleporter disabled in multiplayer!", and does not exit
the level. This is at best confusing, and at worst dangerous, since
D1 secret exits are only available during the countdown, so the player
has little time to realize that the normal exit must be used instead.
- Like D1, multiplayer will request to go to the secret level regardless
of the exit used. Unlike D1, the caller ignores the flag and always
advances to the next regular level.
After this change:
- No observable differences for the player in-game. The questionable D2
secret exit handling for D1 is retained.
- The code makes clearer that secret exits do not work in D2
multiplayer, by way of `#if defined(DXX_BUILD_DESCENT_I)` guarding the
existence of the parameter and all updates to it.
All callers except one want the `lite` version of the request. Switch
from a boolean that is selected inside the function to a reference that
is defaulted to `lite`. Override that argument to the heavy version in
the one caller that needs it.
Using `netplayer_info` in `UDP_sequence_packet` defined many unused
fields. Replace this with per-message types that carry only the
required fields. Make these fields `const` where possible.
Some callers create an explosion with damage forced to zero. Others
create an explosion with damage non-zero. Only the non-zero case needs
to examine nearby objects. Split object_create_explosion_sub so that
the zero-damage case can skip that logic, and the arguments required to
implement it.
Remove the definition of FQ_CHECK_OBJS and all uses of it. Add a new
fvi_query member d_level_unique_object_state *LevelUniqueObjectState.
If object checking is enabled, pass &LevelUniqueObjectState in that
member. If object checking is disabled, pass nullptr in that member.
Change fvi_sub to use this member to decide whether to perform object
checking.
If FQ_CHECK_OBJS is used, a valid object is required. Copy the
icobjptridx_t from fvi_query into a local vcobjptridx_t to force a
validation, then use that validated copy for later work.
- Make all members constant, and pass an anonymous temporary fvi_query
to find_vector_intersection.
- Change `p0`/`p1` to `const vms_vector &`, since the positions are
mandatory. Callers can no longer pass `nullptr` or an uninitialized
value here.
- Change `thisobjnum` to `icobjptridx_t`. Calls to fvi_sub built an
objptridx at need, so moving it to the caller allows it to be
constructed once per find_vector_intersection call.
- Move `flags` and `rad` out of fvi_query, since calls to fvi_sub may
use other values than the ones in fvi_query. This prepares for
passing fvi_query to fvi_sub.
Ship wiggle is normally enabled for compatibility with the original
game. However, during development, it can become a nuisance, so add an
option to allow disabling it.
This started out as underline, but one of the special drawing modes uses
it to draw an underline-width block across the entire vertical height of
the character.
- Inline MAX_FCD_CACHE into the definition of Fcd_cache, then rely on
Fcd_cache.size() later.
- Make Fcd_index unsigned. As an index, a negative number is invalid.
- Change flush_fcd_cache to update Last_fcd_flush_time, rather than
requiring callers to do it. Most callers did not update this.
- Change add_to_fcd_cache to validate and reset Fcd_index before use,
rather than resetting after increment. This allows Fcd_index to
become invalid after a write, but it will be reset to a valid value
before it is used again.
- Fix original game bug in add_to_fcd_cache that caused the cache
expiration path to clear the wrong cache entry.
Embedding it in d_level_shared_robot_info_state is reasonable from a
relational perspective, but interferes with referencing
d_robot_info_array when only a forward declaration of
d_level_shared_robot_info_state is available.
C++20 deprecates `array_name[A,B]`. For this use case, `A` is needed
for its side effects, but is not intended as an index operation. Add
parentheses to satisfy the deprecation warning, without changing the
meaning of the code.
If the song originates in an m3u playlist, no temporary buffer is
needed. Avoid allocating and initializing one, and instead pass the
pointer directly from the CMLevelMusicPath field.
- Skip reading unused alpha channel in TGA screenshot path. The alpha
channel is discarded, so skip even retrieving it from OpenGL.
- Perform the red/blue swap in place, instead of copying into an
additional buffer.
gamedata_read_tbl is only called if (D1 || (D2 && EDITOR)). Exclude
defining it for (D2 && !EDITOR). From there, also exclude defining or
reading the alias list, which is only written by a function
called by gamedata_read_tbl.
Reported-by: heftig <https://github.com/dxx-rebirth/dxx-rebirth/issues/642>
splitpath_t is designed for MS-DOS paths, even though Rebirth now runs
on many platforms that never used DOS conventions. Most of the members
of splitpath_t are unused on all platforms. Remove them, and switch to
returning an initialized version of the structure.
Reported-by: ziplantil <https://github.com/dxx-rebirth/dxx-rebirth/issues/637>
Fixes: 8faed77f5f ("Properly record the event of reset_rear_view() while switching levels to make it work right when rewinding as well; Properly record Countdown seconds for each newdemo frame instead of second change to get display showing up right while playback and still preserving backwards compability; Suspend Game_wind when playing endlevel movie while demo playback")
This loop is run once before the `if()` test, then again at the top of
both the true and false branches of the `if`. Remove the runs inside
the conditional, since they should produce the same result as the run
that occurs before the conditional.
The existing handling always wrote to slot 0, but did so in a convoluted
way. The variable names suggest this was not intended, but no one has
ever reported it as broken, and some otherwise valid data files may
depend on this quirk.
D1 and D2 used logically equivalent code, but it was not the same text,
so the unification project split this with a `#ifdef`. Since the code
does the same thing in both games, combine it into a non-guarded
statement.
Two tests in series examined the object for a type of OBJ_ROBOT. Fold
the test together.
The next test also examines ->type. Move that to an else since it
cannot be true if the first test was true.
The segment list is written to `hit_data`, but `hit_data` goes out of
scope before the segment list is read back. Skip collecting it, since
it was effectively write-only on this path.
Commit 1a9fba804d made an incorrect simplification. It observed that
thief robots are not boss robots and vice versa, and from that changed:
```
if (is_thief) { drop_stolen_items(); }
if (is_boss) { start_boss_death_sequence(); }
else if (death_roll) { start_robot_death_sequence(); }
else { regular_death(); }
```
to
```
if (is_thief) { drop_stolen_items(); }
else if (is_boss) { start_boss_death_sequence(); }
else if (death_roll) { start_robot_death_sequence(); }
else { regular_death(); }
```
This was incorrect, because although a thief is not a boss, a thief does
need to proceed through the other non-boss if tests. This error caused
a thief not to explode on death, and instead continue to fly around and
absorb weapon fire.
Fix the logic error by removing the incorrect `else`, so that a thief
can be tested for is_boss, get false, and then proceed through the
non-boss death logic.
Fixes: 1a9fba804d ("Avoid repeated valptridx dereferences in multibot.cpp")
gcc-12 warns that the snprintf to initialize `tempname` may be
truncated. The variable is never used after it is initialized, so
delete the initialization and declaration.
Players are not robots, but the show-path cheat tried to pretend they
are, and triggered a BUG warning[1] when looking up robot information
from the player object. Fix this by passing in the robot_info when the
caller is providing a robot, and a named `nullptr` value (as
`create_path_unused_robot_info`) when the caller is providing a
non-robot object.
[1] As below, but repeated many times
```
similar/main/ai.cpp:1905: BUG: object 0x555555858550 has type 4, expected 2
similar/main/ai.cpp:1974: BUG: object 0x555555858550 has type 4, expected 2
```
Zero the entire array, rather than adjusting the loop to account for the
count of segments in the level. The array may not need to be zeroed at
all, but this is cheap and guarantees consistent results.
Use `object &` instead of `vmobjptr_t`. This should generate equivalent
code, but produce smaller debug information and may require less
inlining by the compiler.
Every caller passes `1`, so remove the parameter and always use `1`.
For backward compatibility with the previous network protocol, continue
to send a `1` in the network message.
- Write a more specific message on failure
- Defer writing a success message until `spit_powerup` succeeds
- Defer the sound too
- Rewrite the success message to tell how much ammo remains
Commit 8c8b7419b6 defined Shift-F5 and Shift-F6 to adjust the VR
handling. However, Shift-F5 and Shift-F6 already had a meaning: drop
primary weapon / drop secondary weapon. The ordering of the logic
causes the new VR handling to suppress the old drop handling, thus
preventing players from dropping weapons. Disable the VR meaning for
these keys so that drop-weapon works again, since F1 documents those
keys as drop-weapon.
Fixes: 8c8b7419b6 ("Improved conditionals for stereo vs non-stereo modes.")
v29_trigger and v30_trigger define a field `time`. v29_trigger never
initializes it. v30_trigger initializes it from the uninitialized
v29_trigger in legacy mode, and from a file field otherwise. No program
logic ever reads this member, so remove it.
Commit 7f2df64649 converted `mission_menu` to inherit from `listbox`,
but introduced an off-by-one bug in the handling of subdirectories.
`listbox` must be told how many strings it is given. Subdirectories
have one extra string, to represent the `listbox_go_up` element. The
count passed to `listbox` incorrectly failed to adjust for the generated
go-up element, causing listbox not to show the last string in the list.
Change the logic to adjust the count to include the extra element.
Fixes: 7f2df64649 ("Make mission_menu inherit from listbox")
Reported-by: AlumiuN <https://github.com/dxx-rebirth/dxx-rebirth/issues/629>
The contents of the output buffer are undefined if PHYSFSX_getRealPath
fails, so mark the function as [[nodiscard]] and modify all callers to
check that the function succeeded.
Rework the error paths to return path-specific status codes so that the
caller can report exactly which step caused an HMP file to be rejected.
On error, print this reason numerically and, if the reason was a PhysFS
error, also print the PhysFS error code numerically and symbolically.
openable_door_on_near_path should return 0 for no door and any non-zero
value for door-found. Commit 9fdf6005df changed the logic to return 0
for no door, and the side number value for door-found. This is wrong,
since WLEFT has integer value 0, so the caller will interpret a return
of door-found, side=WLEFT as no-door-found.
Fixes: 9fdf6005df ("Convert ai_static::GOALSIDE to sidenum_t")
If two or more events are delivered in the same loop, the previous
implementation would count joystick motion multiple times. Fix this by
moving the joystick interpretation to occur once, after all the events
have been processed.
Lists of these objects are unrolled by a template parameter pack
regardless of whether they are type compatible, so keeping the types
compatible does not improve code size. Store more precise types in the
structure, and avoid needing to store the constant values into a
structure at runtime.
- Use std::integral_constant instead of a static function that returns
the value
- Remove unused protocol_family
- Replace the enum with a typedef for the one type that the enum was
used to define
If exactly one object will always be needed, use an overload that
returns the object id. Otherwise, use an overload that only returns
whether at least one object was created. This simplifies callers that
always request exactly one object.
If at least two robots would be dropped, and a drop failed, then
object_create_robot_egg would report failure to the caller. Callers
that check the return code treat any failure as total failure, but that
is not guaranteed to be true. If the game successfully dropped one
robot and failed when dropping a second, then the caller would receive a
status of failure.
Fix this by returning a status of whether at least one object was
created.
