-
Notifications
You must be signed in to change notification settings - Fork 16
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
SectorPlayers.inc: change color of protected allies #136
base: main
Are you sure you want to change the base?
Conversation
Move the logic deciding what relational CSS class a player is to a free function to avoid code duplication. No functionality changes.
Currently, players are displayed in sector with the following colors: Enemies (newbie turns=NO): red Enemies (newbie turns=YES): yellow Allies (newbie turns=NO): green Allies (newbie turns=YES): green However, this often causes confusion when an ally is in newbie turns. It can make it difficult to count the number of allies who are able to fight or to determine if an ally has forgotten to leave newbie protection. We change the colors here to make it immediately visible if an ally is in newbie turns. Since you can already click "Examine" to inspect if the ally has newbie turns (it will say "Your target is under newbie protection!"), this change does not modify what information is available to players -- only how it is displayed. Display players with the following new colors: Enemies (newbie turns=NO): red Enemies (newbie turns=YES): yellow Allies (newbie turns=NO): green Allies (newbie turns=YES): yellow <-- changed
@@ -1,3 +1,18 @@ | |||
<?php | |||
function getPlayerOptionClass(&$player, &$other) { |
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.
Could you rename this to something like getDiplomaticStatus
or something and add it to SmrPlayer
as eg $player->getDiplomaticStatus($otherPlayer)
, as it's something that would make sense to live there, and gets this logic out of the template
Change free function `getPlayerOptionClass` in SectorPlayer.inc into a method of class `SmrPlayer` called `getPlayerRelationship`. Also delete `SmrPlayer::{get,set}AttackColour` because these methods were never used and may be confused with `getPlayerRelationship`.
The requested changes have been made. I'm not a huge fan of the name I chose, |
@@ -465,16 +465,17 @@ class SmrPlayer extends AbstractSmrPlayer { | |||
$this->hasChanged=true; | |||
} | |||
|
|||
function getAttackColour() { |
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.
Could you readd these or remove them completely please (I know these functions are not used, but if we're gonna have the db fields/class properties then it would be nice to keep the methods available to make use of, or just get rid of it altogether). For reference these were a 1.5 thing where you could choose the colour of the flashing screen when you get attacked
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.
Sure, do you prefer re-adding them or removing them completely?
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.
We already have one vote for keeping it (and even re-implementing the ability to customize the color), so unless you object I'll keep that code in there.
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.
Keeping it works for me
One additional thought I had: should we make the same color changes to the local map? It might be confusing to have the two be different. |
I don't think the local map has an idea of neutral does it? If it does then it could make sense but I think I avoided it as the amount of db requests it would have to make to check every player in every surrounding sector could be fairly slow |
The local map does in fact check for friendly/enemy/neutral players here. So unless I hear otherwise, I'll go ahead and re-use the logic from the current sector in the local map. |
The code change for the local map won't affect performance. However, this is actually a little more complicated than I expected. In the piece of code I linked above (here), we can see how interlinked the idea of "friendly" and "allied" are. In the Current Sector screen, what you really want to see is "can this trader attack with me?" (because you can see the alliances -- there is no degeneracy between protected allies and protected enemies). However, that degeneracy does exist on the Local Map. So how do we reconcile this? What we want:
So how do we do this, given that we only have 3 colors, but 4 states we want to identify (protected/unprotected ally, protected/unprotected enemy)? Or do we want to argue that the Local Map has reduced information, and you shouldn't be able to identify all 4 states? In that case, I assume we would still want the colors to be consistent with the Current Sector, and that requires a restructuring of the logic in the code I linked. It also raises questions like: Do we want to see the location of all allied traders on the galaxy/local map, or do we want to see the location of all "friendly" (in the new definition, i.e. can attack with you) traders. If the former, then that means you would see both green and yellow icons in any sector (and thus a bit of a change in the code logic). Thoughts? |
I think it's fine for the local map to have less granular info because from a roleplaying sense the info you're getting is via a longer range scan that isn't able to get all of the info (but for allies you can get more info via comms/whatever). Overall I think it's your call though as you know the state of the current game far better than I do |
Is this finished? I know the original way was confising but there was a reason for this. Mainly it was quite a cluster to do it the other way, and there is the fact that allied players show up on Galaxy map, thus having them show up yellow sometimes might be a bit confusing. |
I stopped working on this for the exact reason you pointed out -- I couldn't decide how this should behave on the Galaxy Map. I think before we make any further changes, we should decide on a motivation for how scanners behave. (Are the local map scanners the same as the current sector scanners? Are there long-range scanners, and if so should they be a perk of a particular race? Are ally positions broadcast by a different mechanism, or is it still some form of scanning?) |
Currently, players are displayed in sector with the following colors:
Enemies (newbie turns=NO): red
Enemies (newbie turns=YES): yellow
Allies (newbie turns=NO): green
Allies (newbie turns=YES): green
However, this often causes confusion when an ally is in newbie turns.
It can make it difficult to count the number of allies who are able
to fight or to determine if an ally has forgotten to leave newbie
protection.
We change the colors here to make it immediately visible if an ally
is in newbie turns. Since you can already click "Examine" to inspect
if the ally has newbie turns (it will say "Your target is under newbie
protection!"), this change does not modify what information is
available to players -- only how it is displayed.
Display players with the following new colors:
Enemies (newbie turns=NO): red
Enemies (newbie turns=YES): yellow
Allies (newbie turns=NO): green
Allies (newbie turns=YES): yellow <-- changed