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

Remove even more unnecessary fields from various blueprints (bp.Defense) #6171

Open
wants to merge 7 commits into
base: deploy/fafdevelop
Choose a base branch
from

Conversation

Basilisk3
Copy link
Contributor

@Basilisk3 Basilisk3 commented May 11, 2024

Description of the proposed changes

This PR removes the following fields from various blueprints:

AirThreatLevel = 0,
RegenRate = 0,
SubThreatLevel = 0,
EconomyThreatLevel = 0,
SurfaceThreatLevel = 0

Testing done on the proposed changes

All units I tested still functioned normally.

Checklist

Copy link
Member

@Garanas Garanas left a comment

Choose a reason for hiding this comment

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

The problem with removing fields that the game does not use is that external tools (such as websites that visualize the units) may not have safe guards in place to support missing values. In this pull request the regeneration rate would be such a candidate.

Specifically referring to:

Could you check and verify the code by searching for the blueprint field, and see if they properly allow the value to be missing?

A small example for the UnitDB is the fire rate that is missing on the Fire Beetle - it crashes the script.

@Basilisk3
Copy link
Contributor Author

Specifically referring to:

* https://github.com/FAForever/spooky-db

* https://github.com/FAForever/UnitDB

Could you check and verify the code by searching for the blueprint field, and see if they properly allow the value to be missing?

A small example for the UnitDB is the fire rate that is missing on the Fire Beetle - it crashes the script.

I am not familiar with either repo, so correct me if I am wrong but here are my findings:

The UnitDB has this check:
https://github.com/FAForever/UnitDB/blob/bbef3ca4c18145a04c43fc800b152813d8b74d12/www/res/scripts/functions.php#L1761C1-L1764C4

Spooky checks it too:
https://github.com/FAForever/spooky-db/blob/e19cc5f560889f0c175f6f84465590082a038679/app/views/unit.html#L13

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

Successfully merging this pull request may close these issues.

None yet

2 participants