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

Make Exit Conditional and change RA capture Production Lock back to Exit Lock. #15058

Merged
merged 2 commits into from Oct 8, 2018

Conversation

Projects
None yet
5 participants
@MustaphaTR
Copy link
Member

MustaphaTR commented Apr 15, 2018

I added DisableExitOnCondition to Production as i couldn't get making Exit conditional work, but i guess that would be the correct way to do it. I don't wanna mess with it again rn, but someone else can do it, this hack can be removed. Current changes should do the job for now.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 16, 2018

At the very least this will need an update rule. I don't like the idea of DisableExitOnCondition because that creates more churn and even more update rules when somebody fixes it properly. Better to do it properly the first time.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Apr 28, 2018

Do you have a branch with your attempt at making Exit conditional?

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Apr 29, 2018

No, i think i lost that code. I'm planning to reattempt in soon tbh, but not sure when i can find time for it.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:remove-external-capture-lock branch from d8ab516 to fc9a27b May 3, 2018

@MustaphaTR MustaphaTR changed the title Add DisableExitOnCondition to Production and replace ExternalCapturable lock with conditions. Make Exit Conditional and replace ExternalCapturable lock with conditions. May 3, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented May 3, 2018

Updated.

var building = self.TraitOrDefault<Building>();
if (building != null)
building.Unlock();
if (token != ConditionManager.InvalidConditionToken)

This comment has been minimized.

@abcdefg30

abcdefg30 May 3, 2018

Member

This probably wants a null check as well.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:remove-external-capture-lock branch from fc9a27b to b9140f1 May 3, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 7, 2018

Removing the building lock simplifies my fixes for #15076 and #15123, so adding the dependency tag. If the delay isn't going to hurt modding too badly then I would prefer that we defer this until after the next playtest is branched so that all of the Captures changes can be done in the one cycle.

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented May 7, 2018

I think we want that soon for Shattered Paradise, @ABrandau ?

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented May 7, 2018

This can wait for that IMO. The locomotor shift is already a major change inbetween.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented May 9, 2018

The first commit should be safe enough, but the latter two require update rules and manual changes that overlap strongly with the building lock removal. Merging that will introduce a blocker to remove the rest of the locking code because we really don't want to drag the manual rule changes for that over multiple releases.

@ABrandau

This comment has been minimized.

Copy link
Contributor

ABrandau commented May 9, 2018

Yeah, the removal of the engineer lock would be very nice to have, but we can wait, np.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Sep 30, 2018

I'd like to get #15661 in first, and then this one ASAP afterwards, before #15045.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 7, 2018

Can you please rebase this now?

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:remove-external-capture-lock branch 3 times, most recently from 5d509c1 to 108d6d0 Oct 7, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Oct 7, 2018

Rebased. Second commit is now gone, because it was done in #15661. All this does now is make exit conditional and change the production lock back to exit lock.

@MustaphaTR MustaphaTR changed the title Make Exit Conditional and replace ExternalCapturable lock with conditions. Make Exit Conditional and change RA capture Production Lock back to Exit Lock. Oct 7, 2018

var exit = dest.Info.FirstExitOrDefault(null);
var offset = (exit != null) ? exit.SpawnOffset : WVec.Zero;
var exit = dest.FirstExitOrDefault(null);
var offset = (exit != null) ? exit.Info.SpawnOffset : WVec.Zero;

This comment has been minimized.

@abcdefg30

abcdefg30 Oct 7, 2018

Member

The parenthesis are unnecessary (same for the ones in Aircraft.cs). Not a hard blocker, though.

@pchote pchote force-pushed the MustaphaTR:remove-external-capture-lock branch from 108d6d0 to ea0a815 Oct 8, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 8, 2018

I pushed over this with fixes for @abcdefg30's style nit, and a couple more of my own (RequiresCondition should be listed first, factored out disabled trait checks instead of making the linq queries even messier).

@pchote

pchote approved these changes Oct 8, 2018

@pchote pchote merged commit 9972d69 into OpenRA:bleed Oct 8, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@MustaphaTR MustaphaTR deleted the MustaphaTR:remove-external-capture-lock branch Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment