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

Added Scenario & Mission Tracking to Kills, Added Ability to Assign Kills to Scenario and/or Mission #3988

Merged
merged 6 commits into from Apr 15, 2024

Conversation

IllianiCBT
Copy link
Collaborator

@IllianiCBT IllianiCBT commented Apr 10, 2024

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.

image

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 calling getScenario() or getMission().

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. The getScenarios() function is basically identical to its Mission equivalent getMissions().

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.

image

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 of null 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)

image

(Same dialog, but with options selected)

image

(Edit Kill dialog)

image

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.

@IllianiCBT IllianiCBT marked this pull request as draft April 10, 2024 01:36
@IllianiCBT IllianiCBT marked this pull request as ready for review April 10, 2024 01:40
@IllianiCBT IllianiCBT self-assigned this Apr 10, 2024
@IllianiCBT IllianiCBT added Awards Personnel Personnel-related Issues GUI labels Apr 10, 2024
Comment on lines +44 to +45
private int missionId;
private int scenarioId;
Copy link
Collaborator

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

Copy link
Collaborator Author

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();
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

Copy link
Collaborator

@Sleet01 Sleet01 left a 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.

@HammerGS HammerGS merged commit 52133a3 into MegaMek:master Apr 15, 2024
4 checks passed
HammerGS added a commit that referenced this pull request Apr 19, 2024
+ 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Awards GUI Personnel Personnel-related Issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants