Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft - Lib Component #810

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Conversation

royfalk
Copy link
Contributor

@royfalk royfalk commented Nov 10, 2023

Lib component decouples the code base into discrete components - drive, life support, reactor, etc.

Code Changes:

Issues:

  • TBD

if (shields_require_power) {
maxshield = 0;
}
PRETTY_ADDU(statcolor + "Minimum time to reach full overthrust speed: #-c",
playerUnit->getMass() * uc.max_ab_speed() / playerUnit->limits.afterburn, 2, "seconds");
//reactor
float avail = (playerUnit->maxEnergyData() * RSconverter - maxshield * VSDM);
float avail = (playerUnit->energy_manager.GetReactorCapacity() * RSconverter - maxshield * VSDM);

Check failure

Code scanning / CodeQL

Multiplication result converted to larger type High

Multiplication result may overflow 'float' before it is converted to 'double'.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably should fix this

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed

@BenjamenMeyer BenjamenMeyer added this to the 0.9.x milestone Nov 11, 2023
Copy link
Member

@BenjamenMeyer BenjamenMeyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one little change and we should be good.

engine/src/cmd/ai/aggressive.cpp Show resolved Hide resolved
Arrested(parent);
}
}*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this was put in for a story line. If so, we should find a different way to implement it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Even if they did, if we run out of reactor before we get to a base, life support goes out and we die (to be implemented). This straight out doesn't make sense on any level.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this is one of those "fence" situations. I would like to ask some of the long-term project members whether they remember what this code was for.

if (shields_require_power) {
maxshield = 0;
}
PRETTY_ADDU(statcolor + "Minimum time to reach full overthrust speed: #-c",
playerUnit->getMass() * uc.max_ab_speed() / playerUnit->limits.afterburn, 2, "seconds");
//reactor
float avail = (playerUnit->maxEnergyData() * RSconverter - maxshield * VSDM);
float avail = (playerUnit->energy_manager.GetReactorCapacity() * RSconverter - maxshield * VSDM);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we probably should fix this

return std::rand();
}

return 1.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably be:

double random_value = std::rand();
if (random_value < 0.2) {
    return random_value;
}
return 1.0;

As it is, if L40 passes then a new random number will be used and it could still be greater than or equal to 0.2, thereby likely defeating the check.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@BenjamenMeyer You are correct, AFAICT.

@royfalk royfalk mentioned this pull request Nov 24, 2023
2 tasks
Copy link
Contributor

@stephengtuggy stephengtuggy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A couple minor things to change, and I haven't finished looking through all of this code in detail yet. But overall, I really like the direction this is headed.

bool damaged = false;
public:
Component();
virtual ~Component() {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice. I see you have the virtual destructor here, for a class that's intended to be inherited. Excellent.

@royfalk royfalk mentioned this pull request Nov 29, 2023
It's semantics, but can no longer upgrade. Rather, can be damaged.
Add unit tests. 
Fix bug in constructor.
Fix bug in operator +=.

Also, rename SPEC to more generic FTL. It's a better term even if it's not accurate.
This is an interim commit. 
Implement shield component fully:
- Add code in base computer to allow sale of component. Also modify shield info in that file.
- Some changes to shields in units.json to support this. Add Type and Facets.


Issues:
- Shields don't work right as power isn't fully implemented. 
- Color of shield component is listed as damaged.

Additionally:
Switch Health from float to Resource<double>
Add pointers to Resource to support DamageableUnit pointers (e.g. hull). 
Fix bug in resource, where an update won't change current value.
Fix subtle bug in basecomputer, where all armor stats reflect 2nd facet only.
Add armor tests
Refactor Components::Load to remove dependency on Unit
Fix bug in Resource class
Fix bug in Hull and implement some missing code
Refactor UnitCsvFactory to remove dependencies
Fix issue in DL::GetPercent. Not sure why it wasn't caught by the compiler before.
Note: hull upgrade was disabled
Fix issues with shields in SPEC and SetAdjustedMaxValue
Remove energy manager
Make Cloak a Component subclass
Implement Reactor, Shield and Armor more fully
Remove Damageable::regenerateShields - now part of shield and more consistent
Implement energy_container and consumer.
Remove redundant files - energy_types, energy_manager and fuel.
void Armor::SaveToCSV(std::map<std::string, std::string>& unit) const {
for(int i=0;i<8;i++) {
unit[armor_facets[i]] = std::to_string(facets[i].health.Value());
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this only CSV? or any format? Could we just name this Save?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All our saves are CSV only. A proper JSON save would be hierarchical.

  • Ship
    • Component (e.g. radar)
      • Damage to specific attribute.

The current system saves everything as one big struct. When you repair or downgrade, it checks the stock attributes. It works but because everything is in one file, you can't do any unit tests.

// Load from units dictionary
virtual void Load(std::string upgrade_key, std::string unit_key);

virtual void SaveToCSV(std::map<std::string, std::string>& unit) const = 0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this doing something specific to CSV files? Or does it work for JSON and other formats too?
E.g could we just call it Save? Or does it need to only apply to CSV files?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All our saves are CSV only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice to change that up. I'd like to change the saved game format to use JSON too, perhaps also use a zip like format similar to ODF. But that's another refactor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since CSV is an implementation detail that could change, it's probably better to describe it more generically. I think Save or Saves is great, personally.

@royfalk
Copy link
Contributor Author

royfalk commented Apr 24, 2024

This PR got out of hand. I'll wait until @BenjamenMeyer and I finish discussing something and then I'll close it.
I'll port the code here in manageable parts.

@stephengtuggy
Copy link
Contributor

This PR got out of hand. I'll wait until @BenjamenMeyer and I finish discussing something and then I'll close it.
I'll port the code here in manageable parts.

Thanks. I was not looking forward to reviewing this. LOL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants