-
Notifications
You must be signed in to change notification settings - Fork 178
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
Markets Tab and AbstractUnitMarket #2478
Markets Tab and AbstractUnitMarket #2478
Conversation
…s some parts of the contract market changes
Codecov Report
@@ Coverage Diff @@
## master #2478 +/- ##
============================================
- Coverage 10.73% 10.62% -0.12%
- Complexity 3805 3868 +63
============================================
Files 700 721 +21
Lines 97891 99960 +2069
Branches 16307 16405 +98
============================================
+ Hits 10512 10621 +109
- Misses 86001 87954 +1953
- Partials 1378 1385 +7
Continue to review full report at Codecov.
|
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.
Some comments after my first pass.
MekHQ/src/mekhq/campaign/market/unitMarket/UnitMarketOffer.java
Outdated
Show resolved
Hide resolved
MekHQ/src/mekhq/campaign/mission/atb/scenario/PirateFreeForAllBuiltInScenario.java
Outdated
Show resolved
Hide resolved
MekHQ/src/mekhq/campaign/mission/atb/scenario/AllyRescueBuiltInScenario.java
Outdated
Show resolved
Hide resolved
MekHQ/src/mekhq/campaign/mission/atb/scenario/AllyRescueBuiltInScenario.java
Outdated
Show resolved
Hide resolved
Co-authored-by: Christopher Watford <[email protected]>
Co-authored-by: Christopher Watford <[email protected]>
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.
One usability/convenience suggestion, and one other thing that I already forgot what it was. Ah, there it is, philosophical question.
campaign)); | ||
int weightClass; | ||
do { | ||
weightClass = AtBMonthlyUnitMarket.getRandomWeight(campaign, UnitType.MEK, contract.getEmployerFaction()); |
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.
How about just introducing a getRandomWeight() method in there with an upper limit instead to cut down on random loops?
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 would require two methods (minimum limit, maximum limit) for something used three times (twice here [1 min, 1 max], once in PirateFreeForAllBuiltInScenario). I don't think the underlying code increase to handle min/max weights would be worth it, hence why I did it as do-while loops.
case COL_PRICE: | ||
case COL_PERCENT: | ||
case COL_DELIVERY: |
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.
Kind of a moot point, I suppose, but do we really want the player to know ahead of time how long it'll take to get a particular unit delivered?
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 it is like shipping, so... it makes sense to me.
Is there anything else for this? I'd like to start working on phase 2 in the nearish future @NickAragua |
I'm going to call this one good to go, since I'm not too concerned about performance in a couple rare legacy AtB scenarios. |
This PR creates the Markets Tab (following the setup from #2417), abstracts out the Unit Market, makes the Unit Market accessible outside of AtB, redoes the Unit Market Dialog to improve base functionality and simplify the backend, and adds the EntityViewPane and EntityImagePanel components (MegaMek/megamek#2783 merged these into MegaMek).
For reviewers:
Unit Market Dialog was completely rewritten, with it being replaced with a minimal dialog over the primary pane. The previous setup can be ignored.
I previously had this being implemented alongside the AbstractContractMarket, and have carried over the resources and options from there, plus some of the implementation albeit in a commented out state (with TODO statements written for the changes so they are easily searchable). Doing the latter was proving to be too much to do at once, hence the division.