Erroneous change of SPEC capacitor value when weapon capacitor is sold

[ghost]

When a normal weapon capacitor is sold the required energy for a jump and the warp storage of the spec capacitor also changes. This makes jumping the ship impossible without upgrading to a better capacitor.

Steps to reproduce:

• start new campaign
• buy jump drive
• check ship stats
• notice warp capacitor storage and jumpnode travel cost
• sell normal weapon capacitor, check ship stats again
• cost and storage for spec capacitor has changed

[stephengtuggy]

I have encountered this issue on almost every campaign. Thanks for identifying specifically under what conditions it occurs.

[LifWirser]

If by "weapon capacitor" do you mean "capacitor" (because it affects more than weapons like sheilds thrust etc, Having An understanding of capacitor theroy/application, it is possible for the spec capacitor and the regular capaacitor to interact, However for the game probably should not (except for a " shady mechanic " tweak that could enhance battle power FWIW) SUGGEST: new audit sub topic "ship_components" for this issue

[Echoes91]

Actually the warp capacitor value changes when selling any upgrade for the first time (try selling armor/shields and check). Only the initial value of Llama.begin seems inconsistent:

• Warp capacity starts at 270833 MJ with capacitor I installed (166666 MJ)
• When any upgrade is sold, capacity drops to 187500 MJ (or to 20833 MJ if SPEC capacitor was sold)
• From now on, warp capacity is consistent: 20833 MJ + SPEC capacitor value

[nabaco]

That certainly sounds like a bug. I will try to reproduce it, and see if I can get any logs related to the issue.

[ghost]

I finally had some time to revisit this issue and I think I have found the bug. There is definetly a programmatic issue but there is also a huge issue with the units.csv file. I would like to hear your opinions in this matter, especially from the ship designers. IIRC @Loki1950 was quite involved with ship design.

I will try to explain the problem with the initial Llama as example. It applies to all other ships as well. First, let us take a look at this excerpt from units.csv which displays a few of the affected values:

Ship         Warp_Capacitor Primary_Capacitor Outsystem_Jump_Cost Radar_Range Cargo

Llama.begin  325            200               200                 300000000   {skyscope1;...}{capacitor02;...}{add_spec_capacitor01;...}
Llama.blank   25                              300                 100000

Notice the difference in the values between the *.blank and the *.begin unit. The initial value for the primary capacitor is 20 000 MJ in this case and the radar range is 300 000 km (the values for warp energy have an odd conversion factor). If you sell some equipment then all those values are recomputed by this function:

bool BaseComputer::sellUpgrade( const EventCommandId &command, Control *control )
{
...
                playerUnit->SellCargo( item->content, quantity, _Universe->AccessCockpit()->credits, sold, baseUnit );
                UnitUtil::RecomputeUnitUpgrades( playerUnit );
                refresh();
...
}

At some point RecomputeUnitUpgrades() will call a function named makeFinalBlankUpgrade(), which will upgrade your Llama.begin based on the values of the Llama.blank template! The value for Warp_Capacitor drops to 25, Outsystem_Jump_Cost rises to 300, Primary_Capacitor plummets to 0 and Radar_Range is now only 100 km. Afterwards those values are recalculated based on the equipment that you currently have, i.e., you have a Skyscope1 so your new Radar_Range is 30 000 km, you have a capacitor2 so your new Primary_Capacitor value is 20 000 MJ (seems unchanged in this example but that is a coincidence). The value Outsystem_Jump_Cost cannot be influenced by any equipment so it stays at 300. At this point you cannot jump your ship anymore because your warp capacitor cannot store the required energy. This happens for all ships when you sell equipment, whether you notice it or not.

In order to have consistent values I can offer two different solutions, each of which in a different branch in my cloned repository.

Branch bugfix_spec_cap: The values are recomputed, i.e., RecomputeUnitUpgrades() is called, when a unit is created. This way the unit will have the values based on the *.blank model from the beginning, adjusted by the unit's equipment. This approach, however, eliminates the distinct features of the different models. There is no justification to buy/sell them at different prices.

Branch bugfix_spec_cap_no-blank: In this approach the values are also recomputed at unit creation, however, the function makeFinalBlankUpgrade() has been adjusted to base the upgrade on the actual model, e.g., your initial Llama.begin model will be upgraded based on the Llama.begin template. So all your constant values like Outsystem_Jump_Cost stay the same. Your equipment influences a variable value only if it yields a better value, i.e., your equipment will never cause a downgrade. This obviously leaves more interesting possibilities for ship designers to construct different ship models with distinctive features. It will make the *.blank templates almost obsolete, only interesting for ship designers as orientation.

No matter which approach is used, they both will require corrections of the ship stats in units.csv, e.g., the first fix will require for all ship models to have some equipment added in order to be usable. The no-blank approach must correct all non-sensical values like Radar_Range. Allmost all ships have maximum Radar_Range of 300 000 km and thus cannot be influenced by any available equipment.

At this point I would like to suggest that you try out both proposed solutions and get familiar with the differences. It is probably best if you start a new game and hack yourself some more credits, so you can buy a few ships and see how those are affected by each solution.

My preference is on the no-blank solution. I will make a PR once we settle on which solution to use.

[Loki1950]

Thx for taking this one up but your terminology is off there no such thing as a weapons capacitor it is the capacitor that goes with the reactor it should match the size of the reactor the SPEC capacitor is just for the jump drive and SPEC drive the .blank templates are not used for any ship that is bought they are used in the generation of NPC ships ie: all the ships but the players you will notice that if it's not in master_part_list.csv you can not buy it.Have a look at the terminal output you will notice a lot of errors in ship generation not sure on why but your solution will not work as you intend.BTW I was not involved in ship creation just the forum admin and testing and fielding compile time questions.

[ghost]

Thx for taking this one up but your terminology is off there no such thing as a weapons capacitor it is the capacitor ...

Yes, I hade noticed it after I created the issue some time ago. Since it discharged most notably when the weapons were fired I referred to it as "weapons" capacitor, it is just capacitor or primary capacitor in the units.csv file. The SPEC capacitor is the warp capacitor in the units.csv file.

... your solution will not work as you intend.

I do not know how it was supposed to work when the buy/sell mechanism was implemented, all I can say is that this

the .blank templates are not used for any ship that is bought

does not apply. Currently, whenever an equipment part (not a weapon) is sold, the stats of the player's ship will be recalculated based on the corresponding *.blank template. It is hard coded, as explained in my previous post. This leads to the behaviour, that the ship stats that the player sees when buying a new ship, allmost all of them change when a single piece of equipment is sold, i.e., those stats are not relyable and appear volatile. This is most notably for both capacitors but also for other stats like the radar range. E.g., the range will drop from 300 000 km to 100 km for the Progeny. Both my solutions address that problem in a different manner. In both solutions the ship stats, that are displayed to the player, are stable and will not change when an unrelated piece of equipment is sold.

BTW I was not involved in ship creation just the forum admin and testing and fielding compile time questions.

Thanks for clearing that up.

[evertvorster]

I prefer the first option, where if you don't have a radar, your range should be 0. No reactor? You're not taking off. So, when you are buying a new ship, it will be blank and you need to buy the parts to make it go.

Ideally a .milspec or .highbornspec add the fancy expensive equipment on a blank hull. The .milspec and other variants do also modify the top speed and turning speed, which does make a good argument for buying those hulls.

This part of the game does feel a little clunky. It would have been awesome if there was some design documents for this part of the game.

[ghost]

Ideally a .milspec or .highbornspec add the fancy expensive equipment on a blank hull. The .milspec and other variants do also modify the top speed and turning speed, which does make a good argument for buying those hulls.

It would also be a good idea if the *.milspec would provide some "fancy" stuff that cannot otherwise be obtained. It is reasonable to assume that a *.milspec will have classified technology that is not open to the public market. Not the topic of this issue but just leaving this here as an idea for later.

This part of the game does feel a little clunky.

Fully agree.

It would have been awesome if there was some design documents for this part of the game.

I have searched and could not find any. So anyone with any insights of the design process of a ship/unit, please share. Creating design documentation maybe a good topic for another issue.

[evertvorster]

I have searched and could not find any. So anyone with any insights of the design process of a ship/unit, please share. Creating design documentation maybe a good topic for another issue.

Allow me to respectfully disagree with you here. The best time to write design docs is before we fiddle with the code, as we have to decide what it is that we would like the code to do.

Right now is a good time to just type up a one page document on how you would like to see this stuff working first, and let us vote on it. You can also add in the .milspec stuff. When testing time comes, we'll know what to test for as well. A lot of the struggles we have with this codebase is due to lacking documentation. We can ask Benjamin where a good place would be to store the documentation that you do produce.

I have the units.csv editor working on my machine, so I can make requested changes to the units.csv if you need them.

Something to think about while you are designing away... We would like to lessen the dependency on a units.csv over time, so the less we need to specify in there, the better.

By this I mean if we can go zeroing out radar ranges for a hull and just let it be dictated by the equipment we put on the craft that would be better.

[Loki1950]

@royfalk was working on a refactoring of the unit class but had to stop because he forgot that the main way we use the class is though the Python interface and most of his work was not working as he wanted either IIRC the design of the unit class was by @danielrh our founder.

[BenjamenMeyer]

@evertvorster fully agree on the documentation side. Opened up https://github.com/vegastrike/Vega-Strike-Engine-Source/issues/317 for that part of the discussion. Certainly start taking notes now while we figure that story out and come up with our official way of doing it.

[ghost]

We would like to lessen the dependency on a units.csv over time, so the less we need to specify in there, the better.

Yes, I had noticed that a lot of values are redundant and ambiguous. However, if we go with the *.blank solution, then a lot of NPC ships will be created with just the hull. A lot of ships do not specify any equipment and rely on the redundant, ambiguous values to be functional. So the units.csv will also need a lot of work, which leads me to ...

I have the units.csv editor working on my machine, so I can make requested changes to the units.csv if you need them.

Where is that editor and how do I use it? I know there are some tools provided in engine/src/objconv but I did not have the time to familiarize myself with them. A quickstart guide would be much appreciated. Thanks.

[evertvorster]

@crts-crome, the tool is available in the Github repo for vegastrike/Tools/modtools/UnitConverter. There is a user manual in the "doc" subdirectory of that tool.

I am quite happy to help you with the drudgery work, it's been a couple of weeks since I looked at the Units.csv. In all honesty, with a good enough description I could probably knock out a BASH script that does the replacements in Units.csv without touching the units.csv tools.

As a start, I suppose we have to figure out which fields of the units.csv file to ignore, and where the ships need equipment fitted so as to keep the same specifications.

For instance, if we decide that radar range is provided solely by the radar, each vessel needs to have a radar fitted with a range similar to what is specified in the units.csv.

Which reminds me.. is everybody happy with the direction we are intending to go in? Does it make more sense to have equipment provide the specs they provide, and each ship is a sum of it's parts.

What attributes does the hull provide?

Will these changes affect save games?

In my opinion, any change that brings the game forward in terms of realism and robustness is a good one.

We were discussing some far off in the future solution where we can get rid of the units.csv file completely, by having the information for each type of object stored with the object, and the game discovering objects by reading the directory structure of the game, and building the Universe from that. However, that's open heart surgery at this point.... so the less we rely on units.csv, the closer we get to the point where something like that might be easy.

[ghost]

@evertvorster Thanks for the modtools hint. Until now I have been displaying the units.csv file in libreOffice. I got the UnitConverter running. I noticed an issue, though. If you add an upgrade, lets say capacitor03, then it will put it in the "Upgrades" column:

Upgrades        | Cargo
{capacitor03;;} |

If the "Cargo" column is empty then it will not display a capacitor in your inventory. It will nonetheless use the capacitor value defined by the "Uprades" column, as can be seen in the ship's info display.

If you define a capacitor in the "Cargo" column

Upgrades        | Cargo
{capacitor03;;} | ...{capacitor04;upgrades/Capacitors/Standard;200.000000;1;4.000000;4.000000;1.000000;1.000000;;false;true}...

then capacitor04 will be displayed in your inventory but its value will be ignored. It will still use the value from the "Upgrade" column. From what I can tell so far is that the capacitor value is determined in the following order (priority descending):

• check for capacitor in "Upgrades" column
• check for capacitor in "Cargo" column; this is the only one that is displayed in the ship's inventory
• check the value defined in "Primary_Capacitor" column

I have also been thinking about the "Radar_Range" values. The all have the max value of 300 000 km. This brought up the idea that changing that value might affect the AI for NPC units. What if the "Radar_Range" affects an NPC's action radius? What about the armor values? If the NPC unit finds itself with a lower armor value, will it just run away while it would have otherwise attacked? I am not saying that the values do affect the AI but it is something that should be checked.

The main reason I favour the no-blank solution over the .blank solution - at least as a temporary solution - is because I cannot assess how many other issues may unfold in the wake of the .blank solution. It feels like pulling on a loose thread. I was hoping that there might be a few people around who actually designed ships and could share some insight how they determined all those values. Going with the blank solution may push the release date for Release version 0.6 further back since it has a greater risk for unseen issues.

So even if the long term goal is to build from the hull up, the no-blank solution is a good temporary measure which minimizes the risk of unravelling that infamous thread.

[Loki1950]

Most of the values come from hard physics ie: mass,volume,centre of mass,thrust and hard point coordinates you are forgetting that the no .blank solution means no other ships are created that means no ships but the players' kinda lonely no bounty missions no pirates no other ships trading the .blank template are integral to the whole design of having a populated universe.

[BenjamenMeyer]

I do agree that the equipment should carry the values and the vehicle should be a sum of it's parts. This is probably necessary before we can change formats to make things easier to work with too.

As to the no blank...I'll have to read the discussion closer. I'm sure there's a good solution to maintain that while also maintaining the sum of parts aspect.

[BenjamenMeyer]

To be clear - this issue will not effect 0.6.x. It would only apply to 0.7.x (master) and later.

As to the blank issue - if they're informing the NPC players, then...

1. the Blank entries are essentially defaults?
2. we should probably have a base default, and then some explicit NPC specifications

Per 2 - default can specify the hull frame, and a default set of components. NPCs can take the default and apply some upgrades. We could even mark some upgrades as non-buyable if we want to reserve them for NPCs.

My guess is to implement some of this we'll likely be post 0.8.x.

[evertvorster]

@Loki1950 To break this into tiny parts, let's just talk about radar ranges for now. A hull without a radar should not have a radar range, right? Currently, the .blank specifies a radar range, but not a radar. This works fine for NPC, but causes trouble when you buy that hull and sell the radar. In the spirit of re-use, let's assume that a .blank or .stock or whatever we want to call a base model specifies a basic radar rather than a radar range.

All the NPC still have the radar ranges, but now we don't have the problem of a radar range being present without a radar.

To make this sort of thing work, it appears that we will have to change the code that reads the radar ranges, as well as modify the units.csv to give everybody a radar.

I suspect that this process would have to be repeated for every bit of equipment assumed to be necessary for NPC to function like they are functioning now.

Apart from solving the weird you-can't-jump-anymore bug when you do an equipment upgrade, it will make the game slightly simpler to understand, as the special case for NPC goes away. I'm all about simplicity.

[evertvorster]

@crts-crome Very definitely, if you are flying around with a bunch of radars in crates in your cargo hold, they would be not working. I would think that the game engine is doing the right thing here.

For upgrades, these should not show as cargo, they will be part of the ship, using up the upgrade space rather than cargo space. As it is assumed to be installed, the item should have an effect on your vessel's attributes.

[evertvorster]

@Loki1950 Thinking about this some more, we can modify the units.csv first, 0 out all the radar ranges and give the NPC craft radars. Then, we'll go and modify the code to completely ignore the radar range, and just calculate it from the units installed on the vessel.

If this works, then we can move on to the next thing.

An empty field in a .csv file is not too terrible of a thing, so once we have removed this bit of weirdness we can stop and concentrate on the next bug or improvement that is within reach.

[Loki1950]

As @BenjamenMeyer mentioned this will be for 0.7x as we want to avoid any Python changes ATM have to understand exactly how the units class works then apply that knowledge to this bug.

[BenjamenMeyer]

@evertvorster per https://github.com/vegastrike/Vega-Strike-Engine-Source/issues/13#issuecomment-683249502 we should also add a chunk of code to validate that any given vessel is valid at load, and if not generate a warning or error.

Would it be possible to do a validity check for all NPC vessel types at game load? If necessary we could do a special game mode (f.e --validated-vessels) that only does the check if it takes too long. This would give us some confidence that when we change/update something we did so properly.

[evertvorster]

Ah, thanks for bringing this one under my attention again.

Maybe my thinking is a little simplistic here, but can't we move to a model where the NPC ships also have actual equipment providing the attributes of their craft?

One effect of this model would be that there is a chance of equipment flying around after a craft is destroyed that can be tractored in, which could be nice.

It would also be nice to be able to simplify the units.csv, or move to something even more simple and robust.

I'll go and document my thoughts in #317