Misc. Refactoring of Components #1005

royfalk · 2025-01-21T17:34:09Z

What is this pull request trying to do?
As I'm working on moving hull, armor and shield code into lib_component, additional changes have also been made. I've separated these into another PR, so review will be easier.

The changes are many but very localised.

Move graphicOptions.InWarp to ftl_drive.Enabled(). Also support enable, disable and toggle.
Remove base_computer::showUnitStats. It's been replaced by python code.
Remove two unused parameters in DamageRandSys.
Remove unused parameter in Component::Load and subclasses
Align Cloak with other components.
unit_csv::UnitToMap no longer uses the original unit stats as a starting point.
Rename Component::Percent to Component::PercentOperational. We will have namespace collisions when we implement hull, armor and shield, as there is a Percent function there as well.
Standardise on Component::Load(unit_key) and Component::Upgrade(upgrade_key)
Fix bug in AfterburnerUpgrade::Upgrade - added __upgrades suffix twice.
Disabled Component::Load code related to MasterPartList. This needs a PR on its own to do right.
Deleted Component::unit_key. This collides with parameters and doesn't actually add worth. Components shouldn't know about the ship.
Fix issue in JumpDrive and other some other components, where the constructor switched two parameters around.
Make Resource::Percent return 0 and not -1. The game uses this in cockpit and VDU and checking every cycle is costly.
Resource now checks if it is destroyed (0) before repairing.

Code Changes:

Have the PR Validation Tests been run? See https://github.com/vegastrike/Vega-Strike-Engine-Source/wiki/Pull-Request-Validation

Issues identified:

Overdrive assets are missing the "Afterburner_Upgrade" key and are therefore not working when upgraded (Overdrive missing Upgrade_Type field Assets-Masters#55).
BaseComputer does not provide upgrade_view with the current ship status as a parameter. Currently, upgrade_view python script only uses units.json to generate the description. Therefore, any damage to the already installed component cannot be shown (BaseComputer does not provide upgrade_view with the current ship status as a parameter #1006).
Llama.begin shield02 randomly shows damage (Llama.begin shield02 randomly shows damage #1007).
You can buy any shield you want (2/4) for your ship (You can buy any shield you want (2/4) for your ship #1008).
The current upgrade management screen is showing the gaps between the old code and the new. Need to map these out and implement properly in python (Refactor upgrade code and move it to python #1011).
The game doesn't list integrated components. This was OK before, as the engine treated the unit as a single struct and worked on it. Now, we need to list these missing components (The game doesn't list integrated components #1009).
Drive upgrades have not been implemented yet in assets.
The game treats upgrades as cargo. We need to move away from this paradigm (The game treats upgrades as cargo - We need to move away from this paradigm #1010).
The Component class needs to store the information about the upgrade (price, volume, etc.).

- Move graphicOptions.InWarp to ftl_drive.Enabled(). Also support enable, disable and toggle. - Remove base_computer::showUnitStats. It's been replaced by python code. - Remove two unused parameters in DamageRandSys. - Remove unused parameter in Component::Load and subclasses - Align Cloak with other components. - unit_csv::UnitToMap no longer uses the original unit stats as a starting point. - Rename Component::Percent to Component::PercentOperational. We will have namespace collisions when we implement hull, armor and shield, as there is a Percent function there as well. - Standardise on Component::Load(unit_key) and Component::Upgrade(upgrade_key) - Fix bug in AfterburnerUpgrade::Upgrade - added __upgrades suffix twice. - Disabled Component::Load code related to MasterPartList. This needs a PR on its own to do right. - Deleted Component::unit_key. This collides with parameters and doesn't actually add worth. Components shouldn't know about the ship. - Fix issue in JumpDrive and other some other components, where the constructor switched two parameters around. - Make Resource::Percent return 0 and not -1. The game uses this in cockpit and VDU and checking every cycle is costly. - Resource now checks if it is destroyed (0) before repairing.

- Disable mass tests as it is disabled in Component - Disable upgrade name, as it is partially disabled in Component - Adjust Resource tests - new behavior doesn't allow destroyed resource to be repaired.

engine/src/components/afterburner.h

stephengtuggy · 2025-01-21T23:15:26Z

engine/src/components/tests/afterburner_tests.cpp

    upgrade.Load(upgrade_string + upgrades_suffix_string);

    EXPECT_EQ(afterburner.GetMass(), 0.0);
    EXPECT_EQ(afterburner.thrust.MaxValue(), 10.0);
    EXPECT_EQ(afterburner.speed.MaxValue(), 100.0);
    EXPECT_EQ(afterburner.GetConsumption(), 2.0);

-    EXPECT_EQ(upgrade.GetMass(), 5.0);
+    //EXPECT_EQ(upgrade.GetMass(), 5.0);


Why are these being commented out? I don't understand

If you look at Component.cpp, you'll see I've commented out the code to initialize name and mass of the upgrade.
I have a general idea of where I want to go with this. Right now, the game uses cargo to represent upgrades and these (volume, price, name, etc.) are stored as part of the Master Part List (MPL).
Because these are part of Cargo and not Unit Asset, we need to process two separate elements.
I'm not sure how to do that. I'm also not sure I need to.
Right now, upgrade_view takes the key and pulls data directly from the Assets and MPL.

So in other words, we're breaking functionality temporarily until we figure out how this should be done. I am not OK with that. At least not on master. The master branch is supposed to remain playable at all times, right?

I agree here, the master should be playable at all times, so each of the issues identified should be fixed before this PR can be merged.

Saying that, this is a step in the right direction, and so I'll be play testing this branch, and as the fixes come in for the various issues identified I will continue play testing until we are sure that this branch does not introduce any new issues into master.

So in other words, we're breaking functionality temporarily until we figure out how this should be done. I am not OK with that. At least not on master. The master branch is supposed to remain playable at all times, right?

Nope. Right now the game stores a list of Cargo items for each upgrade. I've made provisions in Component when I started development to store this information there.

Commenting it out does not impact game play as this extra copy of Cargo attributes (mass, volume, name, description, etc.) is currently not used.

engine/src/cmd/ai/aggressive.cpp

engine/src/cmd/movable.h

RandomDamage has a chance of destroying a resource, which then cannot be repaired.

Add gitignore

engine/src/components/afterburner_upgrade.h

stephengtuggy

Looks good to me

royfalk requested review from BenjamenMeyer, evertvorster and stephengtuggy January 21, 2025 17:34

royfalk self-assigned this Jan 21, 2025

Fix unit tests

e14986c

- Disable mass tests as it is disabled in Component - Disable upgrade name, as it is partially disabled in Component - Adjust Resource tests - new behavior doesn't allow destroyed resource to be repaired.

stephengtuggy reviewed Jan 21, 2025

View reviewed changes

engine/src/components/afterburner.h Outdated Show resolved Hide resolved

stephengtuggy reviewed Jan 21, 2025

View reviewed changes

engine/src/cmd/ai/aggressive.cpp Show resolved Hide resolved

BenjamenMeyer reviewed Jan 22, 2025

View reviewed changes

engine/src/cmd/movable.h Show resolved Hide resolved

royfalk added 2 commits January 22, 2025 08:49

Fix last remaining unit test

fe4dd49

RandomDamage has a chance of destroying a resource, which then cannot be repaired.

Add override

127e0a8

Add gitignore

stephengtuggy reviewed Jan 22, 2025

View reviewed changes

engine/src/components/afterburner_upgrade.h Outdated Show resolved Hide resolved

royfalk mentioned this pull request Jan 23, 2025

The game doesn't list integrated components #1009

Open

Fix over-aggressive search and replace.

36510b8

stephengtuggy approved these changes Jan 25, 2025

View reviewed changes

royfalk merged commit c9a477f into master Jan 26, 2025
21 checks passed

royfalk deleted the task_armor_3 branch January 26, 2025 17:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Misc. Refactoring of Components #1005

Misc. Refactoring of Components #1005

royfalk commented Jan 21, 2025 •

edited

Loading

stephengtuggy Jan 21, 2025

royfalk Jan 22, 2025

stephengtuggy Jan 22, 2025

evertvorster Jan 23, 2025

royfalk Jan 25, 2025

stephengtuggy Jan 25, 2025

stephengtuggy left a comment

Misc. Refactoring of Components #1005

Misc. Refactoring of Components #1005

Conversation

royfalk commented Jan 21, 2025 • edited Loading

stephengtuggy Jan 21, 2025

Choose a reason for hiding this comment

royfalk Jan 22, 2025

Choose a reason for hiding this comment

stephengtuggy Jan 22, 2025

Choose a reason for hiding this comment

evertvorster Jan 23, 2025

Choose a reason for hiding this comment

royfalk Jan 25, 2025

Choose a reason for hiding this comment

stephengtuggy Jan 25, 2025

Choose a reason for hiding this comment

stephengtuggy left a comment

Choose a reason for hiding this comment

royfalk commented Jan 21, 2025 •

edited

Loading