Personality Revamp #5998
Personality Revamp #5998
Conversation
Moved personality-related classes and controllers from the "personnel.randomEvents" package to the "randomEvents.personalities" package. Updated all affected imports and references across various files to align with the new structure. Added a helper method to `PersonalityController` for calculating personality values.
Replaced ordinal-based traits with name-based handling for better readability, removing outdated compatibility logic. Centralized resource management for personality descriptions using properties files, improving maintainability and localization support. Streamlined enum trait methods and added descriptive accessors for consistency.
This update introduced new fields for personality trait description indices and initialized them with random values using `randomInt(3)`. The changes enable more detailed and randomized personality descriptions and extended XML handling for saving and loading these indices. Additionally, personality description property files were updated to support multiple descriptions per trait.
Revised character trait descriptions to use gender-neutral pronouns and phrasing, enhancing inclusivity. This applies to both Aggression and Ambition property files across multiple traits.
Added ".regexp" suffix to resource keys in personality enums to align with expected format. This ensures proper localization lookup and resolves potential inconsistencies in description retrieval.
Replaced existing description keys with `.regexp` counterparts for better consistency and potential adaptability. This change aligns the property structure to a more standardized format across the file.
Revised and expanded intelligence and greed property descriptions to use regex for enhanced flexibility and customization. Corrected terminology inconsistencies, replacing 'Mech with 'Mek across multiple entries to ensure alignment with proper usage.
…rals Adjusted the personality description logic to include proper paragraph formatting. Fixed single-quote escaping in resource files to ensure consistency and prevent errors.
Replaced hardcoded names and pronouns with dynamic placeholders in Social.properties. This improves reusability and allows descriptions to adapt to different characters automatically.
Simplified property key structure by removing ".regexp" suffix in Intelligence and Aggression property files. This ensures consistency and reduces redundancy in key naming while maintaining the same functional content.
Expanded the descriptions for all Personality Quirks, adding nuance and depth through multiple detailed variants. This enhances the context and characterization, creating more immersive and relatable attributes for each quirk.
…descriptions. Updated descriptions in resource files for better narrative depth and clarity, distinguishing between combatant and civilian personas. This improves immersion and contextual storytelling for characters.
Expanded the descriptions of various personality quirks in the `PersonalityQuirk.properties` file. Each quirk now includes nuanced backstories and motivations tailored to both combatant and civilian contexts, enhancing depth and character immersion.
Removed "DEBRIS_SLINGER" from quirks, updated civilian roles to support roles, and added numerous new personality quirks to enhance diversity. Adjusted descriptions to align with the support role context for consistency.
Added campaign context to personality generation and description logic, enabling context-sensitive behavior for factions and roles. Updated related classes and methods to support this refactor, ensuring accurate generation and descriptions based on campaign information.
# Conflicts: # MekHQ/src/mekhq/campaign/personnel/Person.java # MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java
Moved the `getPersonalityValue` method from `CommandRating` to `PersonalityController` to centralize personality-related logic. Updated imports and documentation to align with the refactor. Consolidated copyright year updates across multiple files.
Added test classes for personality-related enums (Greed, Aggression, Social, Intelligence, Ambition, and PersonalityQuirk). These include validation for string parsing, labels, and descriptions to ensure proper functionality.
Replaced redundant calls to PersonalityController with a static import for writeDescription. Ensured consistent usage of writeDescription during role updates and name changes for improved code clarity and maintainability.
IllianiCBT
added
Bug
Personnel
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5998 +/- ##
============================================
+ Coverage 10.92% 11.13% +0.21%
- Complexity 6357 6419 +62
============================================
Files 1059 1062 +3
Lines 140086 140380 +294
Branches 20884 20878 -6
============================================
+ Hits 15301 15632 +331
+ Misses 123192 123128 -64
- Partials 1593 1620 +27 ☔ View full report in Codecov by Sentry. |
Replaced numerical keys with descriptive "lance" keys for Inner Sphere terminology in PersonalityQuirk.properties. This enhances clarity and improves maintainability of resource definitions.
|
This PR has been marked as a Draft as it shouldn't be merged until I return from vacation due to the likelihood of it needing post-launch support. However, this PR is feature complete and ready for review. |
The Personalities.properties file has been deleted as it was no longer in use. This cleanup reduces unnecessary clutter and improves maintainability of the codebase.
Standardized intelligence labels, descriptions, and tiers for clarity and consistency. Expanded descriptions to provide richer characterization for various levels while improving differentiation across the scale.
Updated the copyright header to include 2025, reflecting maintenance or contributions made this year. This change ensures proper representation of the project's timeline and legal accuracy.
This refactor simplifies method signatures by removing the unused `campaign` parameter from personality-related methods in `PersonalityController` and related calls. This enhances clarity and ensures consistency across the application.
|
Taking this off draft, it's now ready to be reviewed and merged. |
Introduced a chance for re-educated personnel to gain personality quirks. Made personality quirk generation configurable via the static `PERSONALITY_QUIRK_CHANCE` field for better control. Also exposed `generatePersonalityQuirk` for broader use within the campaign logic.
Ensure the first trait description is appended without unnecessary `<p>` tags to prevent spacing issues. Adjust the loop to handle subsequent traits starting from the second entry. This improves the clarity and formatting of personality descriptions.
Changed the modifier of PERSONALITY_QUIRK_CHANCE to "final" to ensure its value remains unaltered throughout the codebase. This improves code clarity and enforces immutability for this constant.
Corrected a placeholder error in the 'INDIFFERENT.description.1' property, replacing an incorrect reference to {4} with {0} for consistency and accuracy. This ensures proper formatting and coherent descriptor functionality.
| return aggressionDescriptionIndex; | ||
| } | ||
|
|
||
| public void setAggressionDescriptionIndex(final int aggressionDescriptionIndex) { |
There was a problem hiding this comment.
I suggest adding Javadocs for these even though they're technically a trivial getters and setters. I worry that the consumers of this API might not know that this should be an integer between 0 and 3 or bigger ranges if they change in the future.
There was a problem hiding this comment.
Actually I have a better idea. I'll clamp them and add a static variable so that it's not possible to input invalid entries. While also killing a few magic numbers. Thanks for the inspiration!
| @@ -2616,65 +2695,29 @@ public static Person generateInstanceFromXML(Node wn, Campaign c, Version versio | |||
| } else if (wn2.getNodeName().equalsIgnoreCase("eduEducationTime")) { | |||
| retVal.eduEducationTime = Integer.parseInt(wn2.getTextContent()); | |||
| } else if (wn2.getNodeName().equalsIgnoreCase("aggression")) { | |||
| try { | |||
There was a problem hiding this comment.
Just confirming that these compatibility handlers should be removed since I don't have the full context
There was a problem hiding this comment.
Yeah, I rewrote the parser so they're no longer necessary. Now it will just accept both ordinal and name without issue.
|
|
||
| public class PersonalityController { | ||
| public final static int PERSONALITY_QUIRK_CHANCE = 10; |
There was a problem hiding this comment.
Is it possible to add tests for the PersonalityController class? It looks like there is a lot of logic involving a good bit of randomness here so I think it would be good to have test coverage for it if we don't. Spies will let you control the randomInt() calls to return values you want, if that's any help.
There was a problem hiding this comment.
Ugh, I really don't wanna write tests for code that wasn't designed for testing. But I guess it wouldn't hurt.
| * @param person the person to calculate the personality value for | ||
| * @return the total personality value of the person in the campaign | ||
| */ | ||
| public static int getPersonalityValue(Campaign campaign, Person person) { |
There was a problem hiding this comment.
Is there any way this could be adjusted to work without taking an entire campaign object? It only seems to care about two specific campaign options right? Could we pass those as booleans instead? Just looking for ways to reduce coupling/dependencies on Campaigns.
| import static org.mockito.Mockito.mock; | ||
|
|
||
| public class GreedTest { | ||
| @Test |
There was a problem hiding this comment.
Check out @ParameterizedTest--you could basically condense most of these into a single test case I think with CsvSource or MethodSource. I have an open PR that does this (AtBContract tests) if you need help
|
|
||
| public class AmbitionTest { | ||
| @Test | ||
| public void testFromString_ValidStatus() { |
There was a problem hiding this comment.
Recommending parameterization
| import static org.mockito.Mockito.mock; | ||
|
|
||
| public class AggressionTest { | ||
| @Test |
There was a problem hiding this comment.
Recommending parameterization
| import static mekhq.campaign.personnel.enums.GenderDescriptors.HIS_HER_THEIR; | ||
| import static mekhq.utilities.MHQInternationalization.getFormattedTextAt; | ||
|
|
||
| public enum Aggression { |
There was a problem hiding this comment.
Do you think a class-level docstring is warranted here?
| * <p>The label is determined based on the resource bundle for the application, | ||
| * utilizing the enum name combined with a specific key suffix to fetch the | ||
| * relevant localized string.</p> | ||
| * |
There was a problem hiding this comment.
Suggest documenting the Person parameter in this docstring
| * | ||
| * @return the description associated with this enumeration value | ||
| */ | ||
| public String getDescription(Person person) { |
There was a problem hiding this comment.
Could it make sense to pass in the index, first name, and gender instead of a whole person to reduce the coupling with that class? Doesn't seem like a major concern here but the fewer things you have to mock in tests the better usually.
|
These are all good feedback. I'll pop this back on draft till I have time to refactor the personality controller for testing. |
# Conflicts: # MekHQ/src/mekhq/campaign/rating/CamOpsReputation/CommandRating.java # MekHQ/src/mekhq/gui/adapter/PersonnelTableMouseAdapter.java # MekHQ/src/mekhq/gui/dialog/ResolveScenarioWizardDialog.java
Introduced modular methods and constants to simplify personality generation and enhance extensibility. Added new fallback logic, improved trait description localization with gender and name support, and clamped indexes for defined ranges to ensure consistency. Refactored enum traits to include metadata for better trait management, improving maintainability and readability.
Updated to use `PersonalityController.getPersonalityValue` with explicit parameters for clarity and to align with campaign options. This improves modularity and ensures better maintainability of the personality evaluation logic.
Renamed `writeDescription` to `writePersonalityDescription` for better clarity and consistency. Updated related method calls and tests accordingly. Also changed `generateAndApplyPersonalityQuirk` to public for broader usability.
|
Did some refactoring to personality controller. However, it's not going to be easy to test because of it's a utility class not initialized so it can't be easily spy'd or for the random elements to be overridden by creating an override class in the test suite. Somebody more learned on unit tests might be able to figure it, but I'm not there yet. I also chose not to implement parameterization testing at this time as I want to be able to look into that properly and I really need to move onto post-50.03 bug support, so at this time the test suite passes and I don't want to take the time to rewrite it. I did, however, update javadocs and do some additional polish and safeguards so I don't anticipate any issues. Flipping this to live: once more with feeling. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| Person person = new Person(campaign); | ||
|
|
||
| for (Aggression trait : Aggression.values()) { | ||
| for (int i = 0; i < MAXIMUM_VARIATIONS; i++) { |
Check warning
Code scanning / CodeQL
Constant loop condition Warning
| Person person = new Person(campaign); | ||
|
|
||
| for (Aggression trait : Aggression.values()) { | ||
| for (int i = 0; i < MAXIMUM_VARIATIONS; i++) { |
Check warning
Code scanning / CodeQL
Useless comparison test Warning
| Person person = new Person(campaign); | ||
|
|
||
| for (Ambition trait : Ambition.values()) { | ||
| for (int i = 0; i < MAXIMUM_VARIATIONS; i++) { |
Check warning
Code scanning / CodeQL
Constant loop condition Warning
| Person person = new Person(campaign); | ||
|
|
||
| for (Ambition trait : Ambition.values()) { | ||
| for (int i = 0; i < MAXIMUM_VARIATIONS; i++) { |
Check warning
Code scanning / CodeQL
Useless comparison test Warning
| Person person = new Person(campaign); | ||
|
|
||
| for (Greed trait : Greed.values()) { | ||
| for (int i = 0; i < MAXIMUM_VARIATIONS; i++) { |
Check warning
Code scanning / CodeQL
Constant loop condition Warning
| public void testGetDescription_notInvalid_Combatant() { | ||
| Faction originFaction = Factions.getInstance().getFaction("MERC"); | ||
| for (PersonalityQuirk trait : PersonalityQuirk.values()) { | ||
| for (int i = 0; i < 3; i++) { |
Check warning
Code scanning / CodeQL
Useless comparison test Warning
| public void testGetDescription_notInvalid_Support() { | ||
| Faction originFaction = Factions.getInstance().getFaction("MERC"); | ||
| for (PersonalityQuirk trait : PersonalityQuirk.values()) { | ||
| for (int i = 0; i < 3; i++) { |
Check warning
Code scanning / CodeQL
Constant loop condition Warning
| public void testGetDescription_notInvalid_Support() { | ||
| Faction originFaction = Factions.getInstance().getFaction("MERC"); | ||
| for (PersonalityQuirk trait : PersonalityQuirk.values()) { | ||
| for (int i = 0; i < 3; i++) { |
Check warning
Code scanning / CodeQL
Useless comparison test Warning
| Person person = new Person(campaign); | ||
|
|
||
| for (Social trait : Social.values()) { | ||
| for (int i = 0; i < MAXIMUM_VARIATIONS; i++) { |
Check warning
Code scanning / CodeQL
Constant loop condition Warning
| Person person = new Person(campaign); | ||
|
|
||
| for (Social trait : Social.values()) { | ||
| for (int i = 0; i < MAXIMUM_VARIATIONS; i++) { |
Check warning
Code scanning / CodeQL
Useless comparison test Warning
getPersonalityValuemethod out of Reputation class and into Personality Controller, as it made more sense for it to live there.Taking everything into consideration we have gone from 120 individual personality characteristics to 360. Intelligence has gone from 24 to 350. And personality quirks have gone from 217 quirks (with one description each) to 229 individual quirks and 1,374 descriptions.
And for anyone wondering what the difference looks like, we go from this...
To this...
Fix #5715