Rename Stances to Relationships in the yaml api #18678
Conversation
abcdefg30
force-pushed
the
relation2
branch
5 times, most recently
from
a5c5ad5
to
d9ae9f5
Compare
| [Desc("What diplomatic stances can this actor disguise as.")] | ||
| public readonly PlayerRelationship ValidStances = PlayerRelationship.Ally | PlayerRelationship.Neutral | PlayerRelationship.Enemy; | ||
| [Desc("Player relationships the owner of the disguise target needs.")] | ||
| public readonly PlayerRelationship ValidRelationships = PlayerRelationship.Ally | PlayerRelationship.Neutral | PlayerRelationship.Enemy; |
There was a problem hiding this comment.
ValidOwnerRelationships / ValidPlayerRelationships I think would be better.
I can't think of any reasons to leave out the Player part when it is the most important (especially since docs aren't necessarily front-and-center when reading/writing MiniYaml).
There was a problem hiding this comment.
The value is a PlayerRelationship, which imo makes it clear that we're talking about players here. (I'm actually not sure which other relationships there might be?)
There was a problem hiding this comment.
I agree that the "*Relationships" naming may not be ideal, but i'm not immediately sure of a better suggestion.
Using Player or Owner does seem clearer, but probably means using multiple different conventions e.g. ValidTargetOwners: Ally, Neutral here and in many other places, but then something like CreatesShroud.ShroudPlayers: Enemy in others. Is this better or worse?
There was a problem hiding this comment.
Is this better or worse?
IMO better, but I'd keep the generic wording: ValidActorOwners for the trait in question and others, and perhaps AffectedActorOwners or simply AffectedPlayers for remaining traits CreatesShroud, InfiltrateForDecoration, VoiceAnnouncement, WithTextDecoration, WithRangeCircle, WithDecoration, WithBuildingRepairDecoration, AppearsOnRadar, TooltipDescription, RevealsShroud.
IMO the term "valid" is not really fitting for the second group because the event in question will happen anyway (provided any relation is defined), the question is only to whom it happens while for the first group of traits any event can only happen when the actor owner is in the right condition (indirectly true for ProximityExternalCondition and GrantExternalConditionPower since the condition is granted anyway, however the actor owner relation condition decides if any further event develops).
not sure
I don't know if introducing this separation is desirable though from the coder point of view.
EDIT: Well actually the relationship must be valid for the second group too...
|
Rebased. |
|
FWIW my comment wasn't asking for changes, as IMO what we have here is okay. It was asking to make sure that we are all happy with that and don't have better ideas first. |
Follow-up to #18677.