-
Notifications
You must be signed in to change notification settings - Fork 291
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
Check if Edge is enabled in Game Options before using it #5855
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5855 +/- ##
============================================
- Coverage 29.05% 28.98% -0.08%
+ Complexity 13911 13909 -2
============================================
Files 2507 2512 +5
Lines 266366 267229 +863
Branches 47621 47780 +159
============================================
+ Hits 77402 77462 +60
- Misses 185025 185817 +792
- Partials 3939 3950 +11 ☔ View full report in Codecov by Sentry. |
megamek/src/megamek/common/Mech.java
Outdated
if (game.getOptions().booleanOption(OptionsConstants.EDGE) | ||
&& (getCrew().hasEdgeRemaining() |
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 looks like we just have a bunch of this conditional repeated throughout Mech.java (and its inheritors), just with different boolean options.
Would it be viable to add, e.g.
public boolean shouldUseEdge(String option) {
return (
game.getOptions().booleanOption(OptionsConstants.EDGE)
&& getCrew().hasEdgeRemaining()
&& getCrew().getOptions().booleanOption(option)
);
or something? Then each check becomes something like this:
case 2:
if (shouldUseEdge(OptionsConstants.EDGE_WHEN_TAC)
&& !game.getOptions().booleanOption(OptionsConstants.ADVCOMBAT_NO_TAC))
Which is somewhat cleaner when spread across all these cases.
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.
Would it make sense to implement that method in the Entity
base class all the various Mech and Aero units extend? Looks like it has a crew member so I think it should have all the information it needs, but I'm not sure if any other objects extend Entity
that shouldn't have this method.
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.
I think that's acceptable; the downside is you would also have to check that getCrew()
doesn't return null
prior to calling .hasEdgeRemaining()
or .getOptions()
, because while an Entity subclass like Mech must have crew, there's no guarantee that a bare Entity will.
@@ -1282,7 +1290,8 @@ && getCrew().getOptions().booleanOption( | |||
case 11: | |||
return new HitData(Mech.LOC_LARM, true); | |||
case 12: | |||
if (getCrew().hasEdgeRemaining() | |||
if (game.getOptions().booleanOption(OptionsConstants.EDGE) | |||
&& getCrew().hasEdgeRemaining() | |||
&& getCrew().getOptions().booleanOption( | |||
"edge_when_headhit")) { |
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.
We should probably take this opportunity to replace all of these with EDGE_WHEN_HEADHIT
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.
Good call, done in 62ec6e8
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.
Functionally looks good, could use a little cleanup.
When determining whether to roll edge on crits, the game only checked whether crews had Edge points remaining and not whether Edge was actually enabled. This led to weird situations where pilots who had edge points assigned would still roll edge even though it was disabled (as described in #5587).
This PR adds an additional check to see if Edge is enabled before checking whether the crew has edge points. I wish there was a cleaner way to do this than at each call site but it would've required more substantial refactoring which I was afraid to do. I looked at implementing it in the
Crew
object but it doesn't extendEntity
and thus does not have access togame
.I tested in a game with all Mechs and they stopped rolling Edge when it was disabled, and rolled Edge when it was enabled, as expected.
Closes #5587