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

FreeActor now supports to be triggered by condition. #15043

Merged
merged 1 commit into from Jun 16, 2018

Conversation

Projects
None yet
6 participants
@IceReaper
Copy link
Contributor

IceReaper commented Apr 11, 2018

This allows a condition to hold back the FreeActor spawn, till the condition is met.

@IceReaper IceReaper force-pushed the IceReaper:FreeActorINotifyBuildComplete branch 2 times, most recently from 13641d7 to 75321fe Apr 11, 2018

@MunWolf

This comment has been minimized.

Copy link
Contributor

MunWolf commented Apr 12, 2018

See #15045 first comment by Pchote same reason applies.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

GraionDilach commented Apr 12, 2018

I presume we should shift the actual actor spawning within this (and FreeActorWithDelivery) into ConditionalTrait.TraitEnabled in that case.

@MunWolf

This comment has been minimized.

Copy link
Contributor

MunWolf commented Apr 12, 2018

EDIT: Had a look at it, making this a conditional trait would be easy enough.

@IceReaper IceReaper force-pushed the IceReaper:FreeActorINotifyBuildComplete branch from 75321fe to b1461f0 Apr 12, 2018

@IceReaper IceReaper changed the title FreeActor now waits for the building to be completely build. FreeActor now supports to be triggered by condition. Apr 12, 2018


protected override void TraitEnabled(Actor self)
{
GrantFreeActor(self);

This comment has been minimized.

@MustaphaTR

MustaphaTR Apr 15, 2018

Member

Didn't test, but i think with current code it would trigger every time trait gets enabled. I guess we would want that to fire only once.

Edit: Or add a boolean to set if it will fire once or every time when enabled.

This comment has been minimized.

@GraionDilach

GraionDilach Apr 15, 2018

Contributor

Yes and I don't see that as an issue, moreso as a feature. If someone wants this only to be enabled once then said one should never disable the trait. There's no issue here.

This comment has been minimized.

@pchote

pchote Apr 16, 2018

Member

Edit: Or add a boolean to set if it will fire once or every time when enabled.

Good idea. Can we do this?

This comment has been minimized.

@IceReaper

IceReaper Apr 16, 2018

Author Contributor

Sure, will do!

@IceReaper IceReaper force-pushed the IceReaper:FreeActorINotifyBuildComplete branch from b1461f0 to 54046a1 Apr 17, 2018

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

👍

new FacingInit(info.Facing),
});

allowSpawn = info.AllowRespawn;

This comment has been minimized.

@MunWolf

MunWolf May 4, 2018

Contributor

This should probably be set outside the AddFrameEndTask, since technically we can still have multiples if it is granted multiple times in the same frame.

Not sure if the implementation for the granting only allows it once a frame but we probably should not depend on implementation here and just move it out.

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Good spotting, agreed.

init.Self.World.AddFrameEndTask(w =>
this.info = info;

if (!IsTraitDisabled)

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

The condition code is guaranteed to call TraitEnabled, even for traits that start enabled. This check and call can be removed.

{
public FreeActor(ActorInitializer init, FreeActorInfo info)
FreeActorInfo info;

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

The info is exposed via the ConditionalTrait superclass (as Info). This and the assignment in the constructor can be removed.

GrantFreeActor(self);
}

void GrantFreeActor(Actor self)

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

After removing the call from the constructor this can be merged directly into the TraitEnabled.

new FacingInit(info.Facing),
});

allowSpawn = info.AllowRespawn;

This comment has been minimized.

@pchote

pchote May 21, 2018

Member

Good spotting, agreed.

@IceReaper IceReaper force-pushed the IceReaper:FreeActorINotifyBuildComplete branch from 54046a1 to 1896326 Jun 15, 2018

@GraionDilach
Copy link
Contributor

GraionDilach left a comment

I still feel AllowRespawn is redundant and confusing, but whatever, LGTM.

Code updated.

@abcdefg30 abcdefg30 merged commit fc82c24 into OpenRA:bleed Jun 16, 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 Jun 16, 2018

@IceReaper IceReaper deleted the IceReaper:FreeActorINotifyBuildComplete branch Jul 12, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.