-
Notifications
You must be signed in to change notification settings - Fork 177
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
Added Scenario & Mission Tracking to Kills, Added Ability to Assign Kills to Scenario and/or Mission #3988
Added Scenario & Mission Tracking to Kills, Added Ability to Assign Kills to Scenario and/or Mission #3988
Conversation
private int missionId; | ||
private int scenarioId; |
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 these should be initialized to 0, see next comment for reasoning.
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.
The primitive 'int' is assigned a value of 0 by default
https://stackoverflow.com/questions/27211713/why-is-an-integer-variable-assigned-null-by-default
![image](https://private-user-images.githubusercontent.com/103902653/322581072-404a30f7-1885-4e7d-ad83-5b828ee0dfb5.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk0MjAzODAsIm5iZiI6MTczOTQyMDA4MCwicGF0aCI6Ii8xMDM5MDI2NTMvMzIyNTgxMDcyLTQwNGEzMGY3LTE4ODUtNGU3ZC1hZDgzLTViODI4ZWUwZGZiNS5wbmc_WC1BbXotQWxnb3JpdGhtPUFXUzQtSE1BQy1TSEEyNTYmWC1BbXotQ3JlZGVudGlhbD1BS0lBVkNPRFlMU0E1M1BRSzRaQSUyRjIwMjUwMjEzJTJGdXMtZWFzdC0xJTJGczMlMkZhd3M0X3JlcXVlc3QmWC1BbXotRGF0ZT0yMDI1MDIxM1QwNDE0NDBaJlgtQW16LUV4cGlyZXM9MzAwJlgtQW16LVNpZ25hdHVyZT1mZTJhNDM0NGVkN2ViYTcwMjlhYjIwMTdiODhmMmQwZjZjMmI0MzUzNjRhOWYzOTdjNGYwOGIwNDI1YzhiZmYwJlgtQW16LVNpZ25lZEhlYWRlcnM9aG9zdCJ9.1EDwwWNQz6X53QLwfuwt2Qyq_YSOZ4C2HasD5uczRAA)
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.
🙈
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.
Don't worry, I only discovered this by complete accident. But I rely on it a lot here, because it allows for easy handling of older saves with null missionID
and scenarioID
values
this.frame = parent; | ||
this.kill = Objects.requireNonNull(kill); | ||
this.date = this.kill.getDate(); | ||
this.missionId = this.kill.getMissionId(); |
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 doesn't look like there is good handling for null values here, which looks like a possibility if the default constructor were used.
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.
In the event the default constructor is used, missionID
and scenarioID
will == 0
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.
Overall looks good, just a minor nitpick about the new member initialization values.
+ Fix #3348: Added Ability to Collapse/Expand Logs, Missions and Kills in Personnel Unit Screen + PR #3970: Reduced Personnel Table Right-Click Menu Clutter + Fix #3981: Removed Unnecessary Error Log + PR #3988: Added Scenario & Mission Tracking to Kills, Added Ability to Assign Kills to Scenario and/or Mission + Fix #3989: Fixed Ship Search Overvaluing Ultra-Green Personnel + PR #3996: Add new player deployment variables to Scenario
Forward
Throughout this PR the term 'Mission' can be read as referring to both Missions and Contracts interchangeably. AutoMedals is the internal name for the in-dev project that will automatically prompt users to optionally assign medals to those employees that have earned them.
Kills
Current Functionality
Three data points are recorded for Kills: Killer, Killed, and Date.
Problem
The in-dev AutoMedals project needs to be able to identify Kills that occurred over the course of a Mission, and those that occurred over the course of a single Scenario.
Solution
I added functions to allow for the Scenario ID and Mission ID numbers to be baked into the stored Kill data. This information is logged and stored, alongside the existing Kill data, in the users' save file.
These IDs are drawn from existing functionality, all I did was hook them into the
Kill
class. That means that any future system that calls this information (such as AutoMedals) will be able to fetch the scenario and mission data just by callinggetScenario()
orgetMission()
.I also added the ability to fetch the internal names of all scenarios on the campaign to the
Campaign
class, (getScenarios()
) as that functionality was missing and I needed for this PR. ThegetScenarios()
function is basically identical to its Mission equivalentgetMissions()
.Testing
I ran a whole bunch of Human v. Princess, and Princess v. Princess scenarios. Somewhere around 20-30 and did not find any instances where kills failed to be assigned a Scenario or Mission ID. This testing was conducted with manually created scenarios, as well as AtB and StratCon scenarios. The testing also accounted for both automatically and manually assigned kills.
Add/Edit Kills
Current Implementation
Currently, the Add/Edit Kill dialog allows you to assign Killer, Killed, and Date. This is true, also, for the Assign Kill dialog.
Problem
This renders a Kill's associated Mission and Scenario IDs completely invisible to users. Furthermore, this means there is no ability for users to assign Missions and/or Scenarios to Kills obtained outside of mm. Such as kills related to RPG-focused campaigns, or any other source of Kills.
Furthermore, as shown in the above image, the dialogs are titled as if they were Scenario dialogs and not Kill.
Solution
I updated the dialogs so that they're a little more visually streamlined. I also renamed the dialogs to 'Edit Kill Entry' and 'Add Kill Entry'.
More importantly, I added the ability to state what Scenario and/or Missions the kill occurred on. This functionality is extended to all future and prior kills. Kills that do not currently have a Scenario or Mission ID will default to the index 0 combobox option, which is blank. Kills can be saved with blank Scenario or Mission IDs, at which point the ID is saved as '0'. This ensures we're not introducing new
null
values, although as ID 0 is not a valid Scenario or Mission ID, this will need to be factored into any future code.There was the possibility of saving the values as
null
, but that would likely break a whole bunch of things so I figured it was best avoided. The only kills which will have a Scenario/Mission ID ofnull
will be those from campaigns originating from prior versions, the frequency of which will decrease with time.When the campaign is saved, any Kills with
null
Mission/Scenario IDs will be assigned IDs of 0.(Assign Kill/Add New Kill dialog)
(Same dialog, but with options selected)
(Edit Kill dialog)
Limitations
Currently, mhq does not track what Mission a Scenario was spawned by. This removes the ability to filter Scenarios based on what Mission was selected. Instead, I sorted the list so that the most recent scenario will always be at the top.
Potentially, this functionality could be added through a follow-up PR, but it's unlikely this is something I'll personally do. At least, I'm not adding it to my current list of projects.
Testing
I have accounted for Missions/Scenarios being deleted, after the kill is assigned an ID tied to that Mission/Scenario. In those events, the system recognizes it's not a valid ID and just treats it as if it were ID 0.
I also accounted for users trying to add kills before a Scenario and/or Mission has been added to the campaign. In these cases users can only select the index 0 option for Scenario and/or Mission, which sets the relevant ID to 0.
Editing a kill that has a null Mission/Scenario ID simply treats it as if the ID were 0.
Recommendation
Once we have a release version that includes this PR, I recommend we advise users to save their campaigns immediately. This will populate all prior Kills with default Mission/Scenario IDs and reduce the amount of
null
IDs the system needs to deal with.I'll make sure to factor into AutoMedal the possibility that it's run before these default values have been assigned, so that will catch whatever left. But saving an existing campaign to the new version, when transferring saves across versions, should probably be encouraged anyway.
Import/Export
Current Implementation
Currently, all kill information is exported and imported across campaigns.
Problem
This means that Scenario/Mission IDs are also transferred across campaigns. This would cause conflicts with the destination campaign, with kills being assigned to the wrong Scenario & Mission.
Solution
When exporting personnel information, kills have Scenario and Mission IDs reset to 0.
Any
null
IDs are also set to 0, as this is functionally identical to saving the campaign.All this means is that the Kill is decoupled from its assigned Scenario and Mission, the Kill still remains in the employees' personnel file and can be manually reassigned, if desired.
While, when exporting, there is the option to export completed Missions, this would still result in Scenario ID conflicts, as well as conflicts with any re-used Mission IDs.
If the Active Mission's ID is ever reused (because it wouldn't have been used in the Destination Campaign), Kills from the Active Mission (in the Origin Campaign) would then be assigned to this new Mission. That functionality is undesirable, so I deemed the best approach was to wipe the slate clean.
Testing
I tested this functionality with campaigns containing Kills both with default (0) Mission/Scenario IDs; non-zero Mission/Scenario IDs, and with
null
Mission/Scenario IDs. None caused issue.Reformatting
Current Implementation
In AddOrEditKillEntryDialog.java all new GUI elements in
initComponents()
are placed at the top of the function.Problems
This forces a lot of scrolling up and down to find what elements are being created and when.
Solution
I moved the element initialization to the top of the relevant block of Swing code.