Change xml recording of hiring halls #6019

AaronGullickson · 2025-02-10T19:41:32Z

Currently a WIP. When hiring halls were added, they were set up using an xml syntax that is not consistent with how we want xml to work for planetary systems. They were placed within the overall planetary system with this code:

<hiringHall>
   <start>2694-01-01</start>
   <level>QUESTIONABLE</level>
</hiringHall>

All we really need to know is the value for the variable staticHall which is tracked in planetary systems and then overrides the dynamic determination of hiring halls. Since levels can change dynamically over time, as given by the start field above (and sometimes an end field), this value should just be tracked in the system_events.xml like so:

<event>
  <date>2694-01-01</date>
  <hiringHall>QUESTIONABLE</hiringHall>
</event>

This is consistent with how the XML has been set up to use. Furthermore, these events should be associate with specific planets not the planetary system at large. This PR will make those changes. The first commit changes the underlying XML files. I also have to make some changes on the Java side to read this in properly.

codecov-commenter · 2025-02-10T19:47:58Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.34%. Comparing base (078a894) to head (3f39021).
Report is 19 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff            @@
##             master    #6019   +/-   ##
=========================================
  Coverage     10.33%   10.34%           
- Complexity     6130     6135    +5     
=========================================
  Files          1039     1039           
  Lines        139399   139412   +13     
  Branches      20675    20677    +2     
=========================================
+ Hits          14409    14420   +11     
- Misses       123542   123549    +7     
+ Partials       1448     1443    -5

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

… for NPE

AaronGullickson · 2025-02-10T21:43:59Z

Ok, I have now made the changes on the Java side. Hiring halls are now recorded at the planet level and general in system events rather than the base system (where the default to NONE). Everything is reading in and displaying as expected. This PR is ready for review.

IllianiCBT

It feels really weird doing a code review for the guy who literally wrote mhq. Couple of very minor points of feedback, otherwise this looks great.

The change I suggested regarding using valueOf instead of a switch isn't vital, given how few cases there are in that enum, but I've found it saves a lot of time and headache when working with enums that have large definition counts.

I'm marking this as 'request changes' just because of the legal statement stuff. It's not me being petty, I just know how much work went into fixing the legal statements earlier in 2024 and I really don't want Tex (or anyone else) having to redo it all.

IllianiCBT · 2025-02-11T00:49:04Z

MekHQ/data/universe/planetary_systems/system_events.xml

@@ -55967,6 +55971,10 @@
        <date>3050-01-01</date>
        <population source="noncanon">2049886673</population>
      </event>
+      <event>
+        <date>3057-01-01</date>
+        <hiringHall source="Mercenary FM revised">STANDARD</hiringHall>


I like that we're now tracking source, too. This is a good addition. Effectively resolved any future conversations about why x was rated as y

IllianiCBT · 2025-02-11T00:49:34Z

MekHQ/src/mekhq/adapter/HiringHallAdapter.java

@@ -0,0 +1,35 @@
+/*
+ * Copyright (c) 2019-2022 - The MegaMek Team. All Rights Reserved.


Should this read 2025, or has this class just been copied from somewhere else?

Yep, copy pasta error. Fixed now.

IllianiCBT · 2025-02-11T00:52:19Z

MekHQ/src/mekhq/campaign/universe/enums/HiringHallLevel.java

+    GREAT;
+
+    public static HiringHallLevel parseHiringHallLevel(String val) {
+        return switch (val.toLowerCase()) {


If we convert the string to all uppercase I believe we'll be able to use HiringHallLevel.valueOf(val) inside a try-catch. We then we can add a logger message saying 'hey this failed to parse, using 'NONE' inside the catch that will return NONE. This will mean that in the event more enums are added (for whatever reason) we don't need to maintain this method.

Excellent idea and it works perfectly. As I work on the bigger YAML conversion project, I should be able to use this approach to clean some other enum parse methods up that do this as well.

IllianiCBT · 2025-02-11T00:56:46Z

MekHQ/src/mekhq/campaign/universe/Planet.java

+        if (getPopulation(when) == null || getPopulation(when) == 0) {
+            return HiringHallLevel.NONE;
+        }
+        for (Faction faction : getFactionSet(when)) {


When we use getFactionSet() is it possible for the method to return null in the event that no faction is present (for example, empty systems / dead planets)? I'm not 100% if it can, or not, and if it does return null whether the loop will just fail gracefully.

This might not be a valid concern, so ignore this entire message if it isn't.

The code starting on 748 is actually not new, but rather got ported over from PlanetarySystem to Planet so it hasn't been breaking anything yet. I am pretty sure that the factions object is set to an empty set so can never be null.

IllianiCBT · 2025-02-11T01:00:46Z

MekHQ/src/mekhq/campaign/universe/Planet.java

-import mekhq.adapter.PressureAdapter;
-import mekhq.adapter.SocioIndustrialDataAdapter;
-import mekhq.adapter.StringListAdapter;
+import mekhq.adapter.*;


GitHub won't let me leave this comment any higher. This might seem incredibly petty, but don't forget to update the license statements to 2025 whenever you edit a class. Tex put in a lot of effort to bring all our legal stuff up to date, so we're trying to be really vigilant to make sure that work doesn't have to be redone in 5 years.

I went through an updated copyrights on all of the touched files.

IllianiCBT · 2025-02-11T01:01:49Z

MekHQ/src/mekhq/gui/view/PlanetViewPanel.java

@@ -336,8 +336,7 @@ private JPanel getPlanetPanel(Planet planet) {
        gbcLabel.gridy = infoRow;
        panel.add(lblHiringHall, gbcLabel);
        JLabel textHiringHall = new JLabel(StringUtils.capitalize(


I can't believe I've gone this long without knowing this utility existed. This will make my life a lot easier!

IllianiCBT

Thanks for handling the copyrights :)

AaronGullickson · 2025-02-11T22:40:14Z

Hi @IllianiCBT. I think I addressed all your concerns. Let me know if it looks good now.

Change xml recording of hiring halls

a540a30

AaronGullickson added 7 commits February 10, 2025 12:35

Fix xml error on closing tag

ba4fb6b

Update xml planet loading for new location of hiring hall information

1f481f3

Display hiring hall on actual planet, not for planetary system

49998c2

Add convenience isNone method to HiringHallLevel class

5041b6e

Add back in code for dynamically generating hiring halls, with checks…

45ea317

… for NPE

Remove AtB limitations on map adornments for Great hiring halls

3a72e9b

Merge branch 'master' into fix-hiring-halls

b44bde6

AaronGullickson marked this pull request as ready for review February 10, 2025 21:42

AaronGullickson requested review from IllianiCBT and Algebro7 February 10, 2025 21:44

AaronGullickson self-assigned this Feb 10, 2025

AaronGullickson added the Data Hammertime label Feb 10, 2025

AaronGullickson requested a review from HammerGS February 10, 2025 21:44

IllianiCBT requested changes Feb 11, 2025

View reviewed changes

AaronGullickson added 2 commits February 11, 2025 14:15

Correct copyright error

dc3910a

Change copyright on other touched files

314b17c

IllianiCBT approved these changes Feb 11, 2025

View reviewed changes

Change parse method for HiringHallLevel for less maintenance

3f39021

HammerGS merged commit f8838e8 into MegaMek:master Feb 11, 2025
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change xml recording of hiring halls #6019

Change xml recording of hiring halls #6019

AaronGullickson commented Feb 10, 2025 •

edited

Loading

codecov-commenter commented Feb 10, 2025 •

edited

Loading

AaronGullickson commented Feb 10, 2025

IllianiCBT left a comment

IllianiCBT Feb 11, 2025

IllianiCBT Feb 11, 2025

AaronGullickson Feb 11, 2025

IllianiCBT Feb 11, 2025

AaronGullickson Feb 11, 2025

IllianiCBT Feb 11, 2025

AaronGullickson Feb 11, 2025

IllianiCBT Feb 11, 2025

AaronGullickson Feb 11, 2025

IllianiCBT Feb 11, 2025

IllianiCBT left a comment

AaronGullickson commented Feb 11, 2025

		@@ -0,0 +1,35 @@
		/*
		* Copyright (c) 2019-2022 - The MegaMek Team. All Rights Reserved.

Change xml recording of hiring halls #6019

Change xml recording of hiring halls #6019

Conversation

AaronGullickson commented Feb 10, 2025 • edited Loading

codecov-commenter commented Feb 10, 2025 • edited Loading

Codecov Report

AaronGullickson commented Feb 10, 2025

IllianiCBT left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IllianiCBT left a comment

Choose a reason for hiding this comment

AaronGullickson commented Feb 11, 2025

AaronGullickson commented Feb 10, 2025 •

edited

Loading

codecov-commenter commented Feb 10, 2025 •

edited

Loading