SPA Cross-Country #4118
SPA Cross-Country #4118
Conversation
Codecov ReportBase: 22.97% // Head: 22.97% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## master #4118 +/- ##
============================================
- Coverage 22.97% 22.97% -0.01%
Complexity 4824 4824
============================================
Files 2282 2282
Lines 250115 250126 +11
Branches 46366 46382 +16
============================================
+ Hits 57459 57460 +1
- Misses 191204 191214 +10
Partials 1452 1452
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
|
I've started a new branch to add in the isXXX calls so this will get cleaned up after it is merged along with the original terrain code and possibly some other areas. |
|
Is there anything else I need to fix so this can get approved? |
|
Conflict fixed |
| @@ -386,29 +388,52 @@ public int movementCost(Entity e) { | |||
| } else { | |||
| mp = 1; | |||
| } | |||
|
|
|||
| if (isCrossCountry) { | |||
There was a problem hiding this comment.
Sorry to chime in.
This only checks the movement mode. WHEELED is also used by QuadVees in wheeled mode. A vehicle pilot probably shouldn't command a qv but MM doesn't prevent this. I wonder if e.isVehicle() should be added to the crossCountry boolean definiton.
Also, I agree with Windchild that repeating mp *= 2 really calls for OR-ing the two conditions or it looks like an error.
There was a problem hiding this comment.
The first one for woods was a bug that was fixed. I can probably try to do the if/else or if/else if better, but to cram it all in a single if would make it look rather confusing imo.
As for quadvee. A check for isVee will get added just to make sure it ignores any weirdness like that.
There was a problem hiding this comment.
I agree these are usually ugly to read with all the ((()()((()((()))). But having the same result multiple times screams error or legacy issue. Maybe a solution is to add a
final boolean allowedHoverWheeled = ((moveMode == EntityMovementMode.HOVER) || (moveMode == EntityMovementMode.TRACKED)) && (level == 6);
and then if (allowedHoverWheeled || (moveMode == EntityMovementMode.WHEELED)) {
This is still shorter than the if else and readable enough I guess. What do you say?
There was a problem hiding this comment.
It could work, I will split the changes into multiple commits for review to make it easier to revert if it doesn't look right and you decide against it. Quadvee will be first then the others.
| if ((e.hasAbility(OptionsConstants.INFANTRY_FOOT_CAV) | ||
| && (moveMode == EntityMovementMode.INF_LEG))) { | ||
| mp -= 1; | ||
| } | ||
| return Math.max(0, mp); | ||
| case Terrains.WOODS: | ||
| mp = level; | ||
| if (isCrossCountry) { | ||
| if ((level == 1) && ((moveMode == EntityMovementMode.HOVER) || (moveMode == EntityMovementMode.WHEELED))) { |
There was a problem hiding this comment.
Same as the other, OR-ing this looks even easier as level can't be 0
There was a problem hiding this comment.
But it can be greater than one, and certain vehicles can't travel is certain woods levels. Woods have 3 levels, 1 is light woods, 2 is heavy woods, and 3 is ultra heavy woods. Hover and Wheeled vehicles cannot enter woods at all while tracked vehicles can only enter light woods.
There was a problem hiding this comment.
Hmm I feel we're talking past each other here.
With crossCountry, what this checks is:
if Level 1 and h/w --> mp2
-also- if Level > 1 --> mp2
That is exactly the same as if (Level 1 and h/w) -or- Level > 1 --> mp2
Which is the same as: if level > 1 -or- h/w --> mp2 (assuming level must be > 0)
(I have not thought about the rules, only the code as it is)
| @@ -494,12 +526,24 @@ public int movementCost(Entity e) { | |||
| } else { | |||
| mp = 1; | |||
| } | |||
|
|
|||
| if (isCrossCountry) { | |||
| if (((moveMode == EntityMovementMode.HOVER) || (moveMode == EntityMovementMode.TRACKED)) | |||
This should keep QuadVees from getting lumped into this.
|
Changes done. |
With the exception of water movement this SPA is completed. Waiting on ruling for how tanks in depth 1 water should be treated. I'm thinking they should have the same LOS and targeting rules as amphibious tanks, but you never know sometimes.