Skip to content
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

Implement new, explicit player stances for owners, allies and spectators. #15160

Closed
wants to merge 1 commit into from

Conversation

GraionDilach
Copy link
Contributor

@GraionDilach GraionDilach commented May 20, 2018

This PR is aimed towards Next+1, it's heavily WIP, it's opened this early to ensure necessary discussion. and to made sure that it gets merged early in a release cycle.

This PR aims to resolve a long-standing issue with stances can't distinguishing between owners and allies.

At this point i only tested the first 5 minutes in the RA mod but I'm fixing issues on the get-go.

Related to #10892, #11093, #11102 and probably a lot more.

@pchote
Copy link
Member

pchote commented May 20, 2018

As mentioned in IRC it would make the logic a lot cleaner if we could rename StrictAlly back to Ally and then either introduce an explicit OwnerOrAlly stance, or, better, explicitly reference Stance.Owner | Stance.Ally in code / Owner, Ally in yaml.

The update rule should be relatively straightforward, refer to DefineSoundDefaults for an example where we list field on traits to look for, but then instead of maintaining a list of things to report for manual updates you just need to parse the .Value<Stance>(), update the value, and then replace it on the node.

You can define the following helper in UpdateExtensions to make that last step easier:

public static void ReplaceValue(this MiniYamlNode node, object value)
{
	node.Value.Value = FieldSaver.FormatValue(value);
}


return false;
}

public static bool AppearsHostileTo(this Actor self, Actor toActor)
{
var stance = toActor.Owner.Stances[self.Owner];
if (stance == Stance.Ally)
if ((stance & Stance.Ally) != 0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stance.HasFlag(Stance.Ally)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't work on composite flag values.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stance.HasStance(Stance.Ally)

@GraionDilach
Copy link
Contributor Author

Do note that I am also adding the spectator stance within this PR as well.

I will look into reconsidering Ally to be allies only but that increases a lot of complexity, something I'm not very fond of at this point. My longterm goal with this is to shift all if (player == RenderPlayer) checks to stance lookups anyway.

@pchote pchote added this to the Next + 1 milestone May 21, 2018
@pchote pchote modified the milestones: Next + 1, Future Jun 30, 2018
@pchote
Copy link
Member

pchote commented Jun 30, 2018

Moving to the Future milestone so we don't lose this.

@GraionDilach
Copy link
Contributor Author

I'll probably get to return to this during the weekend, however without actual progress on the reviewing queue I don't see the point of trying to finish this - it would be just wasted effort considering that this'll grow up to be a high-risk high-reward PR ultimately with all the upgrade rules and trait default updates it'll bring to - and my other PRs already are roadblocks on my personal tasklist - #15014 in particular.

@ghost
Copy link

ghost commented Sep 2, 2018

:(

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants