-
Notifications
You must be signed in to change notification settings - Fork 176
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
Facility implementation #2788
Facility implementation #2788
Conversation
…ty_implementation
…ty_implementation
Codecov Report
@@ Coverage Diff @@
## master #2788 +/- ##
============================================
+ Coverage 10.62% 10.64% +0.02%
- Complexity 3869 3890 +21
============================================
Files 717 717
Lines 100134 100246 +112
Branches 16409 16417 +8
============================================
+ Hits 10635 10668 +33
- Misses 88111 88207 +96
+ Partials 1388 1371 -17
Continue to review full report at Codecov.
|
This pull request introduces 2 alerts when merging fcfdca1 into df5e785 - view on LGTM.com new alerts:
|
…ty_implementation
…ty_implementation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general, just a few code improvement ideas
@@ -1604,6 +1604,9 @@ public static int calculateEffectiveBV(AtBDynamicScenario scenario, Campaign cam | |||
bvBudget += forceBVBudget; | |||
} | |||
} | |||
|
|||
bvBudget += bvBudget * scenario.getEffectivePlayerBVMultiplier(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a Math.round, Math.floor, or Math.ceil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why? It always produces a whole number.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is also an unrounded double to integer cast, and thus may not in future... and causes another CI issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really sure why we care if the bv budget drifts one way or the other by 1 point - we're usually talking thousands or tens of thousands of point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And honestly, how often does java change the behavior of their "double to integer" cast? It's not like we're directly executing CPU instructions here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rarely, and I know it is minor... but it's still a code scanning alert both in LGTM and in GitHub Actions, and I'm trying to keep the first low while reducing the latter.
This pull request introduces 1 alert when merging a4e9daf into eb33d90 - view on LGTM.com new alerts:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For merging into 0.49.3. Will open a PR with suggest code improvements shortly.
This implements supply depots, data centers and early warning systems for StratCon.
Supply depot - allied: +1 support point/week
Supply depot - hostile: +5% to opfor BV budget
Data Center - allied: reveals entire track
Data Center - hostile: increases chances of scenario to spawn by 5% on either the "once a week" roll or the "when you deploy your lance" roll
Early Warning System - reduces arrival delay for reinforcements by 1 turn
I started work on the "Orbital Defense" facility, but I think I have a much cooler idea for it than what I was originally thinking of (think "facility in orbit" rather than "ground based guns"), but that's for a different pull request.