Fully support installing multiple weapons per ship #5991

sturnclaw · 2024-12-05T03:07:02Z

Newly-necromanced from the grave of its 2021 implementation returns the GunManager - a complete replacement of the legacy FixedGuns code. This new implementation:

Supports more than a single weapon per ship per direction
Allows you to separate weapons into groups¹
Handles per-hardpoint gimbal limits
Can track each individual weapon's aiming point and temperature separately¹
Supports rear-facing, side-facing, and any-other-facing weapons¹
Is ready to support visible weapons on ship models²
And can be extended to support additional weapon types (i.e. ballistics / rocket launchers) and even turrets in the future.

I've been talking about this for quite a long time, so I don't think I need to provide a wall of text for the reason this PR was written. This PR represents one of two major changes needed to release the equipment rewrite on the upcoming Pioneertag - the other being the combined fuel-and-hyperdrive rework.

This implementation brings us to feature-parity with the existing FixedGuns system, but feature-enablement work will have to be done in the future to fully harness all of the features of the implementation.

TODO:

Implement serialization code for the GunManager component
Thoroughly test hyperspace travel and save/load code paths

Tagging fluffy/mwerle for code review on what's completed while I'm finishing up the serialization code.

1: The GunManager supports this - the UI and the rest of the code still does not.
2: Since we don't have any weapon models other than a single placeholder, I've stubbed out the necessary placeholders and will fully implement weapon rendering in 2025.

src/Quaternion.h

src/ship/GunManager.h

src/ship/GunManager.cpp

src/ship/GunManager.h

src/ship/GunManager.cpp

mwerle

Going to stop the (nitpick) review for now. (and it also teaches me not to do per-commit reviews.. something that I never really did before, but a colleague of mine swears by! Apologies for some completely unwarranted comments..)

Please check all functions:

whether they are in the correct class
parameter signatures (in general, use references / const refs in preference to pointers and pass-by-value for all non-base types) - I'm guessing a lot of this code is going to be running per-frame and even if not it's just good modern C++.
wrap all compound statements in braces (safe coding practice) - the current PR uses a mix of styles sometimes wrapping single-line compounds in braces, and sometimes not. But let's at least be consistent even if my suggested best-practise is not consistent with the codebase :)

Functionally I guess it works (sorry haven't tried it myself yet); great job! Sounds like space combat is about to get a heck of a lot more deadly!!

Weapon Groups (Design Questions):

Should groups be nameable at the API level, or is this something for a future UI?
Groups appear to be very dynamic at present; a player may like a fixed set of groups, in a fixed order, even if for a particular ship/loadout that doesn't make sense. So the player should be able to keep a group even if it is empty, but have it be disabled. Very useful for certain joysticks which have sliders selecting between multiple set points. Might be more relevant once players can own multiple ships and/or rapidly switch between different loadouts.

bszlrd · 2024-12-05T08:33:25Z

Do you need UI mockups for it? I'd imagine there will be a gun group management interface somewhere. F3 infoscreen perhaps?

src/Quaternion.h

sturnclaw · 2024-12-05T19:08:03Z

@mwerle

Weapon Groups (Design Questions):

Should groups be nameable at the API level, or is this something for a future UI?
Groups appear to be very dynamic at present; a player may like a fixed set of groups, in a fixed order, even if for a particular ship/loadout that doesn't make sense. So the player should be able to keep a group even if it is empty, but have it be disabled. Very useful for certain joysticks which have sliders selecting between multiple set points. Might be more relevant once players can own multiple ships and/or rapidly switch between different loadouts.

I'm unsure if groups are something that will ever receive names, but certainly not at the GunManager level. Per my rationale in another response, the GunManager's group implementation is intended to be very low level and dynamic; what the player interacts with will definitely be augmented at a higher level with additional data to display to the user.
Empty / disabled groups are definitely something I'm open to supporting - that's part of why I kept a number of checks in the code to see if a group is empty, even though default behavior right now is to prune empty groups.
Setting groups is intended to be something the player does after changing weapons, and it's meant to support most configurations of weapons the player would like.

These are very useful concerns that I don't intend to tackle at this juncture yet. The focus of this PR is to get to feature parity with the existing FixedGuns implementation, and I intend to defer explicit support for player-assignable fire control groups to after the upcoming release (there's not enough time to do everything on the TODO list!)

@bszlrd

Do you need UI mockups for it? I'd imagine there will be a gun group management interface somewhere. F3 infoscreen perhaps?

I'd always love UI mockups, you know this. 😄 A UI for fire control groups is something I don't plan to implement until after Pioneertag, but I was likewise thinking about sticking it on the ship info screen. Feel free to make a mockup if you have inspiration - if the improbable happens and I have extra free time I might be able to squeeze it in before mid-January feature freeze.

sturnclaw · 2024-12-05T21:19:47Z

Serialization is implemented (ugly thing that it is), and I've addressed most or all of the review feedback received so far. Please continue to review (and test!) - this branch should be feature-complete at this point and just needs a more strenuous test regimen than I've given it so far.

src/ship/GunManager.h

mwerle · 2024-12-06T03:27:45Z

src/ShipAICmd.cpp

+		float max_fire_dist = stats.range;
 		if (max_fire_dist > 4000) max_fire_dist = 4000;


Suggested change

float max_fire_dist = stats.range;

if (max_fire_dist > 4000) max_fire_dist = 4000;

float max_fire_dist = std::min(stats.range, 4000.0f);

(It took me a few seconds to figure out why the local variable was needed in the first place; this makes it marginally more obvious IMHO)

I know this is not part of this PR, but why are we clamping a weapon range to some arbitrary maximum range here instead of just trusting the weapon stats?

Thanks!

(jokingly) You've dipped your toe into a very dangerous pool - few are brave enough to touch the Ship AI and the those that do tend to be devoured by The Singularity and are never seen again...

In more seriousness, your question likely can only be answered by a long-ago developer - the AI is positively ancient code and hasn't had a proper maintainer in many years. I don't have enough of a math background to make full sense of the flight code, so I've not had reason to touch it before now.

I'll include the above change in the PR, though I'm not sure if I want to address AI weapon handling in this sprint or defer it to another time.

Ah, fair, it's just for AI ships? (I should probably look at the filename lol) So player-ship-weapons are not affected by this clamping; gotcha.

Interesting, so flying 4km's from a target and taking pot-shots at distance guarantees a safe fight.. this requires further testing! (Perhaps it was deemed too difficult/unfair for AI ships to have perfect aim from so far away..) Then again, AI should have a skill modifier, which adds a skill-scaled amount of jitter to the aiming. (No idea whether or not it does, just saying)

- Remove erroneous single-vector constructor, add constructor which computes rotation to a normal vector from some unit vector - Add helper methods to construct a quaternion from up/forward directions

- Simply aliases to the underlying sigc::connection for early cleanup

- Responsible for managing all "permanent" weapons (i.e. not missiles) attached to a ship. - Stores all associated data about a ship's hull configuration and mounted weapons directly, due to a lack of support for serialization of LuaSharedObject. - Each weapon is assigned to a single group, which is responsible for sending fire commands to that weapon and managing targeting. - Originally written (but not merged) in 2021. - Future expansion is intended to support more flexible damage types, explosive projectiles, and dumbfire rocket launchers. - This system is intended to handle turrets in the future, via extensions to the weapon mounting system to support multiple traverse options. GunManager: add FiredThisFrame/StoppedThisFrame - Supports our extremely hacky sound code system - Ideally a "better" solution would be found at some point, but this is adequate for the time being GunManager: add GetGroupLeadPos - Temporary accessor that sums together the lead positions of all weapons in the group to support indicator rendering GunManager: add GetGroupTemperatureState - Temporary API to query the average temperature of all weapons in the group relative to their overheat threshold

- Ported from the FixedGuns implementation. - Thanks to azieba for writing the solution in the first place. - Uses an exact solution to the question of when the projectile will get there, and then an approximation of the ship's acceleration over that time. - Provides quite a bit better probability-of-hit for ballistic weapons

- Kept as a separate commit for porting to a future body inspector tool for the PerfInfo window

- Weapon equipment now uses the GunManager API (with a shim to continue using the existing weapon data) - The GunManager has its weapon mount information populated from Lua on construction or ship type change

- Improve documentation on several functions and types - Add boolean return value to AddWeaponMount - Convert internal functions to take state values by reference rather than pointer - Make defaults for WeaponData/WeaponState less specific

- Ugly but functional, as all C++ serialization code tends to be.

- Because the player variable is cached at file scope, it becomes orphaned on end game. - Reset it during cleanup phase so it can be populated with the correct Game.player object during the next game.

- The StringName values are borrowed rather than copied, which saves a small fixed cost to increment the refcount

sturnclaw added New feature C++ code Lua API labels Dec 5, 2024

sturnclaw requested review from mwerle and fluffyfreak December 5, 2024 03:07