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

Issue #416: AtB turret generation and base attack scenario fix #417

Merged
merged 12 commits into from
Oct 23, 2017

Conversation

NickAragua
Copy link
Member

@NickAragua NickAragua commented Aug 1, 2017

A somewhat crude implementation of a turret generation mechanism for base attack scenarios (and now usable for any other scenarios in the future which may want turrets). I'm not aware of a way that turrets can be generated from the standard RAT generator, so I chose to imitate the mechanism used to add civilian units instead.

Notes: the AtB rules state that the civilian unit count on base attack missions should be 8, so 8 it is.

Also: This fix is dependent on pull request #415 , as it uses the updated unitGenerator.getUnitRatingName() function.

@neoancient
Copy link
Member

AtB used to generate turrets using a crude hard-coded method based tables provided by Makinus, who wrote the AtB rules. I've requested code review by kipstafoo since he has be reworking the AtB scenario generation code.

@kipstafoo
Copy link
Collaborator

So, two comments:

  • It looks like the turrets have been changed to be part of the civilian force, instead of the defending NPC force. Was this a conscious change? If so, why? I don't see a problem with it, I'm just interested as to why.
  • It looks like the randomGunEmplacement() method is now unnecessary. Is there a reason to keep it in the scenario class?

@NickAragua
Copy link
Member Author

NickAragua commented Aug 29, 2017

The AtB rules state that the "base force" during a base attack gets 8 civilian units and 6 turrets. So, I'm following the text. It doesn't really matter either way, as the pilots are generated at the opfor skill and tech level.

I don't think there is a reason to keep the randomGunEmplacement() method around. I'll get rid of it.

@NickAragua
Copy link
Member Author

I think I'd like to keep the randomGunEmplacement() method around but commented out for historical purposes.

@kipstafoo
Copy link
Collaborator

Ok, I'm good with it being part of the civs. Like I said, it really doesn't matter as long as the turrets target the right 'enemy'.

For the randomGunEmplacement(), let's nuke it please instead of commenting it out. Git maintains history for us, so no sense in muddying the code with unreachable/unused code. I'm a stickler for clean code. ;) And that you for removing the unused imports earlier... that drives me utterly bonkers.

@NickAragua
Copy link
Member Author

No idea why the diff looks so weird, but commented out code removed.

@NickAragua
Copy link
Member Author

Using my amazing powers of prediction (wooooooooo), I see a future where Bloodwolf has created more turret RATs, rendering the original fix obsolete. The new fix accounts for the possibility of many different combinations of turret RATs, just as long as they follow the pattern of "Turret YYYY Q", where YYYY is a year and Q is the single-letter quality level.

@BLOODWOLF333
Copy link
Contributor

K, Yeah when I get around to grinding through the clan turret RATs they will all be in the same format.

@NickAragua NickAragua merged commit ee0695d into MegaMek:master Oct 23, 2017
@NickAragua NickAragua deleted the origin/atb_turret_generation_fix branch November 16, 2017 01:13
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.

4 participants