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.
- Change D1X to use D2X rules regarding Vulcan cannon pickup. The D1X
rules were confusing at best, and seem outright wrong in some ways.
- When a Vulcan cannon was picked up, it was treated as containing not
less than VULCAN_WEAPON_AMMO_AMOUNT rounds, regardless of what it
actually contained. D2X respected the actual contained count, even
when running in D1X emulation mode.
- In D1X single player, if the Vulcan cannon was not picked up, then
it could be treated as Vulcan ammo. If at least 1 unit of
ammunition was added to the player, the entire powerup would be
consumed, regardless of how much ammunition remained.
- In D2X single player, if the Vulcan cannon was not picked up, then
ammunition would be taken from it, but the powerup would only be
consumed if all its ammunition was taken.
- In D1X multiplayer, a player who already had a Vulcan cannon could
not get anything from touching a new cannon, and the cannon would
not be changed.
- In D2X multiplayer, a player who already had a Vulcan cannon could
take ammunition from the cannon, but the cannon could not be drained
below VULCAN_AMMO_AMOUNT. If the cannon had VULCAN_AMMO_AMOUNT or
less, then the player could not take ammunition. If the cannon had
more, the player could drain it to that level.
- Replace all that with a simplified version of the D2X rules:
- If the player does not have the cannon, the cannon is picked up
and removed from the mine. The player takes as much of its
ammunition as possible. If the cannon was well stocked, and the
player was nearly full, some ammunition will be lost. This is
unfortunate, but the game has always had this rule, and changing
it would require dropping one or more Vulcan Ammo packs to
represent the untaken ammunition.
- If the player already had that cannon, then the player takes as
much ammunition as the cannon has, while not exceeding the
ammunition cap. Other players, if any, are updated about how much
ammunition remains in the cannon. The cannon remains in the mine.
- Backport to D1X the network message for updating the contained
ammunition in a vulcan cannon. zico added the basic feature in
7684ce92, but only applied it to D2X. With the change to let D1X
multiplayer take ammunition from the cannon, D1X now needs the same
feature.
- Remove the special case to delete an empty cannon. Instead, let the
cannon remain in the mine without ammunition. This allows a player in
single player mode to leave behind a backup cannon, which could be
useful if the player is killed and wishes to rearm before returning to
the death site. Similarly, under the new rule that players in
multiplayer can drain the cannon down to 0 ammunition, this removal
allows the cannon to remain behind for someone else to take, rather
than allowing it to be deleted by a player who already had an instance
of it.
Instead of creating the powerup from a player, then overwriting the
location and velocity of the powerup, and fixing up its segment, create
the powerup directly where it should be, with the intended velocity.
Add periodic calls to send_endlevel_packet in kmatrix if
Control_center_destroyed is false.
Needed because after finishing the last level, kmatrix is called from a
different place where Control_center_destroyed is already false.
It looks like this issue was introduced in Rebirth 0.56 with the
split off of the ipx code (febe5d1). That commit removes the call to
multi_endlevel_poll2 from kmatrix_view, which did the same periodic
sends. (Maybe because in D1 Control_center_destroyed was always true
in kmatrix so it was not needed there.)
Reported-by: snytek <#520>
Fixes: febe5d1 ("Abstracting networking protocols")
clang-13 fails to sufficiently inline this construct, causing it to emit
an unnecessary call to check_null_pointer_conversion on a path where
m_ptr is already guaranteed to be non-nullptr. That call in turn
causes a linker error, since no other site using valptridx<wall> needs
check_null_pointer_conversion instantiated. Rather than provide the
instantiation, rework the logic to avoid the need for the dead call.
Instead of creating the powerup from a player, then overwriting the
location and velocity of the powerup, and fixing up its segment, create
the powerup directly where it should be, with the intended velocity.
Iterating over it returns each side number in turn. This allows
converting many loops of the form:
```
for (int i = 0; i < MAX_SIDES_PER_SEGMENT; ++i)
```
to the compact form:
```
for (const auto i : MAX_SIDES_PER_SEGMENT)
```
The compact form brings the usual benefit of range-based for: delegating
iteration to the compiler prevents the loop body from skipping a step,
and makes clear in the code that this is the case.
Some flags merit a type other than int8_t. Begin moving flags out to
distinct variables with their own type.
Add static_assert checks that the ABI relevant structures do not change.
A level can have a secret exit without having a mission entry describing
where to go when the secret exit is used. Switch from an assertion
failure in that case to a log message.
This triggers a diagnostic from -Wuseless-cast. It appears to be
unnecessary, after tracing down typedefs. It might need to be restored
if some platform uses a definition that is neither the same type nor
implicitly convertible.
clang treats names introduced by structured bindings specially, and
refuses to capture them for a lambda, even when it should be capturing
an rvalue reference. gcc accepts this capture.
Switch to initializing a lambda local variable from the structured
binding in the outer scope, which works with both compilers.
Reported-by: Kreeblah <https://github.com/dxx-rebirth/dxx-rebirth/issues/609>