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

Remove legacy building plumbing #15743

Merged
merged 11 commits into from Nov 3, 2018

Conversation

Projects
None yet
2 participants
@pchote
Copy link
Member

pchote commented Oct 27, 2018

Four years in the making, this PR completes the final push to replace our legacy Building plumbing with conditions. This old code hardcoded assumptions about the classic C&C semantics for building placement / capturing / demolition / etc across many traits, and has been superseded by conditions. The last few places that depend on the old plumbing are ported to conditions, and the legacy code is removed.

Fixes #7035.
Fixes #10438.

I recommend reviewing commit-by-commit.

@pchote pchote added this to the Next Release milestone Oct 27, 2018

@pchote pchote force-pushed the pchote:remove-legacy-building-plumbing branch from 6420a31 to f09caec Oct 27, 2018

{
return info.PreventsEjectOnDeath;
if (conditionManager != null && !string.IsNullOrEmpty(info.Condition))

This comment has been minimized.

@abcdefg30

abcdefg30 Oct 27, 2018

Member

Doesn't this need to check if the condition was already granted? (Using conditionToken == ConditionManager.InvalidConditionToken.)

This comment has been minimized.

@pchote

pchote Oct 27, 2018

Member

Previously the answer was no, because the lock prevented multiple demolitions from happening at once. This isn't the case any more, though. I think i'm going to have to do some fundamental work in this trait rather than pushing hacks around even more.

@@ -21,29 +22,47 @@ public class DemolishableInfo : IDemolishableInfo, ITraitInfo
[Desc("If true and this actor has EjectOnDeath, no actor will be spawned.")]
public readonly bool PreventsEjectOnDeath = false;

This comment has been minimized.

@abcdefg30

abcdefg30 Oct 27, 2018

Member

This is not used now.

public override string Description
{
get
{
return "Traits are no longer force-disabled while the WithMakeAnimation trait is active.\n" +
"This affects the With*Animation, With*Overlay, and Gate traits.\n" +
"This affects the With*Animation, With*Overlay, *Production, Attack*,\n" +
"Transforms, Sellable, Gate, ToggleConditionOnOrder, and ConyardChronoReturn traits.\n" +

This comment has been minimized.

@abcdefg30

abcdefg30 Oct 27, 2018

Member

Differences in indentation.

@pchote pchote force-pushed the pchote:remove-legacy-building-plumbing branch 4 times, most recently from 873b250 to 5f83d9c Oct 28, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Oct 28, 2018

Updated. This now also makes Demolishable conditional, and takes another small step towards removing DelayedAction. I have left the DelayedActions in the bridge huts to avoid further expanding the scope of this PR.

@pchote pchote force-pushed the pchote:remove-legacy-building-plumbing branch from 5f83d9c to db4eb77 Nov 3, 2018

@abcdefg30 abcdefg30 merged commit 47a470e into OpenRA:bleed Nov 3, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Member

abcdefg30 commented Nov 3, 2018

@pchote pchote deleted the pchote:remove-legacy-building-plumbing branch Nov 18, 2018

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