Refactor: Detach outfit class from weapon #9859

RisingLeaf · 2024-02-24T10:11:38Z

Refactor

This PR addresses the strange stuff described in issue #9852

Summary

Removes inheritance of the outfit class to the weapon class and instead makes weapon a member of outfit.
Also creates a new member in hardpoint pointing to the weapon and a getter for that variable.

I am very open to suggestions, this is a first minimum viable product

Usage examples

makes some changes a lot easier

Testing Done

Flew around installed and uninstalled weapons, no errors encountered

Performance Impact

Nothing I noticed

source/Hardpoint.h

source/Outfit.cpp

source/Ship.cpp

source/ShipInfoDisplay.cpp

source/PrintData.cpp

source/OutfitInfoDisplay.cpp

…ndless-sky into change-weapon-logic

OcelotWalrus

Looks good for me.

source/Armament.cpp

tehhowch

Obligatory context note: I didn't build this, test it, or review the tens of thousands of discussion posts or PRs that have been made since I last had time for this repo.

source/Outfit.h

tehhowch · 2024-03-03T20:43:32Z

source/OutfitInfoDisplay.cpp

 		attributeValues.emplace_back(outfit.Ammo()->DisplayName());
 		attributesHeight += 20;
-		if(outfit.AmmoUsage() != 1)
+		if(weapon.AmmoUsage() != 1)


Having multiple places that need to be checked for the ammo bits feels off.

Consider moving all ammo bits into Outfit, or vice versa. Another option is to introduce a class like AmmoConsumption, which would handle mapping a Weapon / Outfit definition to the ammo it requires (e.g. if an outfit had multiple weapons, or different firing modes that were internally represented as multiple Weapons belonging to a single Outfit).

Introducing something like AmmoConsumption feels a bit too much for this PR, so I moved the ammo pair into weapon completely. But still left an accessor function in outfit to avoid making the code confusing when it comes to an outfit storing ammo or a weapon consuming ammo

tehhowch · 2024-03-03T20:52:04Z

source/ShipInfoDisplay.cpp

-		if(it.first->IsWeapon() && it.first->Reload())
+	{
+		const Weapon &weapon = it.first->GetWeapon();
+		if(it.first->IsWeapon() && weapon.Reload())


Suggested change

if(it.first->IsWeapon() && weapon.Reload())

if(weapon.IsWeapon() && weapon.Reload())

?

Weapon shouldn't have an IsWeapon() function anymore. When I said that IsWeapon should be a function of Outfit, I meant to move it from Weapon to Outfit, not have Outfit call Weapon's function.

Refer to #9859 (comment)

So I thought for consistency I should always use outfit.isweapon
but ig in this situation what you recommend looks better

tehhowch · 2024-03-03T21:03:00Z

source/Weapon.cpp

@@ -101,7 +101,7 @@ void Weapon::LoadWeapon(const DataNode &node)
 		else if(key == "submunition")
 		{
 			submunitions.emplace_back(
-				GameData::Outfits().Get(child.Token(1)),
+				&GameData::Outfits().Get(child.Token(1))->GetWeapon(),


IMO this is not desirable. Previously, we were storing a pointer to the key of a collection, and the choice of collection meant we had particular guarantees about that pointer (always stable, etc) regardless of the collection size. Now we're storing a pointer to an implementation detail (some field of said managed object). Probably it's fine. But it's definitely something to be aware of.

endless-sky/source/Weapon.h

Lines 47 to 50 in 71b66b9

explicit Submunition(const Weapon *weapon, std::size_t count) noexcept

: weapon(weapon), count(count) {};

const Weapon *weapon = nullptr;

I recall there were previous thoughts of registering / standardizing Weapon (i.e. GameData::Weapons()), but I don't have specifics on why we deferred. Likely it was related to the ability of Person ship variants to have their own specialized weapon definitions.

Would it be better to store a pointer to the outfit?

source/Weapon.h

quyykk · 2024-03-26T22:20:01Z

source/Weapon.h

@@ -202,8 +204,9 @@ class Weapon {
 	// Return the ranges at which the weapon's damage dropoff begins and ends.
 	const std::pair<double, double> &DropoffRanges() const;

+private:
+	friend class Outfit;


Do we have to make this a friend?

for setturretturn and ammo

quyykk · 2024-03-26T22:23:33Z

source/Weapon.h

@@ -57,6 +57,8 @@ class Weapon {


 public:
+	Weapon() = default;
+
 	// Load from a "weapon" node, either in an outfit, a ship (explosion), or a hazard.
 	void LoadWeapon(const DataNode &node);
 	bool IsWeapon() const;


Do we need to have Weapon::IsWeapon? Presumably we only really need Outfit::IsWeapon.

we need it for hazards too

source/Outfit.cpp

quyykk · 2024-03-26T22:36:25Z

source/Hardpoint.h

@@ -101,6 +103,7 @@ class Hardpoint {
 private:
 	// The weapon installed in this hardpoint.
 	const Outfit *outfit = nullptr;
+	const Weapon *weapon = nullptr;


Why store the weapon? Any reason why we can't use outfit->GetWeapon() where we need to access it?

I didn't check, but maybe GetOutfit() is unused and not needed in which case you remove outfit instead.

the code just looks a lot cleaner with an extra variable for weapon

worx

fb880b5