[RFE 4364] Add campaign option to allow duplicate portraits#6092
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #6092 +/- ##
============================================
+ Coverage 11.03% 11.13% +0.09%
- Complexity 6397 6425 +28
============================================
Files 1062 1062
Lines 140222 140441 +219
Branches 20858 20884 +26
============================================
+ Hits 15474 15636 +162
- Misses 123145 123181 +36
- Partials 1603 1624 +21 ☔ View full report in Codecov by Sentry. |
IllianiBird
left a comment
There was a problem hiding this comment.
Looks great. One very minor bit of feedback that I have to mark as 'request changes' because Git doesn't have a 'suggested changes' option. Lemme know if you agree/disagree with my thoughts.
I'm also super keen to hear how you found the process of setting up the campaign option side of things. Something I really tried to do was improve the process so that it was more streamlined than prior CO IIC so looking for feedback now folks are starting to use it.
| Arrays.fill(usePortraitForRole, false); | ||
| usePortraitForRole[PersonnelRole.MEKWARRIOR.ordinal()] = true; | ||
| assignPortraitOnRoleChange = false; | ||
| allowDuplicatePortraits = true; |
There was a problem hiding this comment.
This hooks back into my previous comment - that I accidentally made outside the review, whoops. If we're deciding that we allow duplicate portraits by default, then we might as well have it enabled in the campaign presets.
There was a problem hiding this comment.
Overall it felt like a fairly simple process. The one issue I had was with a bit of an irregular naming - most places in the code follow the convention of either allowDuplicatePortraits or prefixAllowDuplicatePortraits, but the line for creating a new campaign options checkbox requires you to use AllowDuplicatePortraits since it adds a prefix to it under the hood. That minor difference in one place ended up easy to miss, resulting in me putting the allowDuplicatePortraits there like I did everywhere else and then spending some time debugging why it can't find anything.
Implements #4364
When the option "Allow Duplicate Portraits" is enabled, random portrait generator ignores existing personnel and feels okay with potentially using the same portrait. Defaults to true.