Added Population Check to Education Module #4281
Conversation
…on EducationController - Included a check for system depopulation, implying academy destruction in EducationController - Modified checkForAcademyDestruction to take into account system's population depletion
IllianiCBT
changed the title
The option is to enable the message that tells users why there is no academy in the relevant system. Otherwise the academies will just be missing with no explanation as to why. |
|
I just spotted the cause of confusion: |
…lict` in campaign options
I guess in that case, I'm even more convinced that we don't really need it to be an option. |
|
Would you be able to advise why you don't think the option is needed? I think it would be helpful if I better knew the direction you were coming from :) |
|
I think a screenshot of the affected area may be helpful to understand what we're talking about exactly. Right now, to me, it looks like you're adding a campaign config option to control the display of invalid locations of academies in a menu where academies are being assigned to a person? What's the scenario where you'd want to be able to say that a person is attending a blown up academy? |
|
The campaign option is part of this range of choices. Each option determines which academies will still be displayed when certain ineligibility criteria are met. Instead of simply disabling the 'show ineligible academies' option, which would hide all academies the individual is ineligible for, this option allows for more nuanced control. When in use, academies will be shown like this: These academies cannot be interacted with, but they inform the user why the academy can't be selected. However, the user may decide that they don't care, in which case the option can be disabled. But perhaps they do care, or maybe they only care if the system is depopulated. By removing the campaign option, we take away that choice from the user. Moreover, it could potentially lead to false bug reports with users claiming that academies aren't displaying. Or, worse, that academies aren't showing when they should, but the user assumes it's because their personnel are ineligible. I introduced these options to prevent situations like this, where it's difficult for the user to easily see what academies they can send personnel to: But also to avoid situations like this, where no academies are visible, but the user has no way to tell why: I hope this clarifies things. I understand it's not always easy to envision how things will appear in-game when all you have is the GitHub changes list. :) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4281 +/- ##
=========================================
Coverage 10.29% 10.29%
- Complexity 5816 5819 +3
=========================================
Files 925 925
Lines 125882 125917 +35
Branches 18628 18637 +9
=========================================
+ Hits 12954 12959 +5
- Misses 111656 111685 +29
- Partials 1272 1273 +1 ☔ View full report in Codecov by Sentry. |
|
Fair enough. I'm not really a giant fan of adding an option for everything as it tends to increase code complexity; I'm also not convinced that adding an extra option buried in campaign settings somewhere will help with user complaints. But since the work is done already, we may as well leave it in. |
|
I see your point. I know you just approved this, but I’m going to go ahead and reduce the options down to just ‘display ineligible academies’. I think having all these campaign options solves a problem that doesn’t exist. |
- Removed configuration options for different types of eligibility conflicts in the education module - Simplified eligibility check to only rely on the "showIneligibleAcademies" setting without considering specific conflicts - Cleaned up associated UI options and settings serialization
# Conflicts: # MekHQ/src/mekhq/gui/panes/CampaignOptionsPane.java
This PR adds a check to ensure a system is populated before allowing personnel to attend academies there.
Additionally, I added a check for total system population depletion after enrolling (as part of the new week checks). This ensures more accurate dates for the destruction of prestigious academies when that destruction results from system depopulation (assuming we have accurate data). If accurate data isn't available, mhq will default to the academy's static destruction year (defined in the academy XML) as a fallback.
This also allows extending academy destruction events to local academies, which wasn't possible previously.
Marking this as a bug, as personnel being able to attend academies with 0 system population is unintended behavior.