Various NPC fixes/enhancements#2433
Merged
Merged
Conversation
Member
hemberger
commented
May 11, 2026
hemberger
commented
- Tweak HTML on the "Hire Trader" page
- Add Race/Last Active columns in the table on the "Manage NPCs" page.
- Alliance leader now gets a message if NPC is dismissed by someone else.
- Alliance Bank Report now identifies NPCs.
- News entries now include alliance membership.
- Fix bug allowing players to hire more than 3 NPCs.
Add a newline between the message and the table. It was too cramped previously.
Call it "Action" like in many other similar tables. This makes the table look a bit less weird when there are NPCs but they are not allowed to be hired.
This is especially useful for notifying the leader when an NPC is fired.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2433 +/- ##
============================================
- Coverage 35.62% 35.60% -0.02%
- Complexity 4314 4319 +5
============================================
Files 132 132
Lines 12898 12905 +7
============================================
Hits 4595 4595
- Misses 8303 8310 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Helpful for deciding which NPC should get the axe.
This helps make it more obvious what the NPC contributions are. Switching to using Player methods to get the display name, rather than manually constructing it. Modified Player::getDisplayName to optionally omit the alignment-based name color. This was needed to recover the original color in the bank report. Could have also added an option to omit the player ID (again, to retain original behavior of the report), but I think it's useful to include the player IDs here for the same reasons we include them almost everywhere.
This is motivated by the fact that hired NPCs leave their alliance after being podded, so it is not obvious to see which alliance they were in at the time they were podded. But I think it may also be a nice way to make the news more informative. The main downside is that it is a bit wordy when the player also has a named ship.
Since Player::joinAlliance only updates the player alliance in PHP, subsequent calls to Player::getAlliancePlayers will not include the new player if the cache has not already been populated. This was the case in HireTrader when loaded from HireTraderProcessor, which would therefore allow hiring NPCs over the limit of 3. The option would only display for a split second for AJAX-enabled users, but it was persistent if AJAX was disabled. Managing state consistency between PHP and the database is challenging, and hopefully this can be fixed with transactions once we fully migrate to InnoDB.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.