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

Make BaseProvider PausableConditional #14738

Merged
merged 1 commit into from Mar 8, 2018

Conversation

Projects
None yet
6 participants
@MustaphaTR
Copy link
Member

MustaphaTR commented Jan 16, 2018

I wanted to work on replacing ExternalCapturable building lock with conditions, BaseProvider needs to be conditional for that to work, as currently being captured disables the base providing.

Pausing the trait makes the Range Circle red, disabling it complately removes the circle.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:conditional-base-provider branch 2 times, most recently from b1c05d3 to 2f15abd Jan 16, 2018

@@ -90,7 +92,7 @@ public IEnumerable<IRenderable> RangeCircleRenderables(WorldRenderer wr)
self.CenterPosition,
Info.Range,
0,
Color.FromArgb(128, Ready() ? Color.White : Color.Red),
Color.FromArgb(128, Ready() && !IsTraitPaused ? Color.White : Color.Red),

This comment has been minimized.

@pchote

pchote Jan 16, 2018

Member

Shouldn't you be checking IsTraitPaused (and IsTraitDisabled) in Ready?

var target = Target.FromPos(bp.Actor.CenterPosition);
if (target.IsInRange(center, bp.Trait.Info.Range))
return bp.Actor;
var baseProviders = a.TraitsImplementing<BaseProvider>().ToArray();

This comment has been minimized.

@pchote

pchote Jan 16, 2018

Member

Isn't this just reimplementing a slower version of ActorsWithTrait?

This comment has been minimized.

@MustaphaTR

MustaphaTR Jan 16, 2018

Author Member

I'm not sure if i messed something else up or it should be like that, ActorsWithTrait was giving crash to me for multipile traits on the actor. So here finds the actors having trait and get all the traits from them.

Edit: Hmm.. Wait, i'm already using ActorsWithTrait on the Rendering Circle part, i'll try again.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:conditional-base-provider branch from 2f15abd to c82ca99 Jan 16, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Jan 16, 2018

Updated.

@@ -69,7 +68,7 @@ public void BeginCooldown()

public bool Ready()
{
if (building != null && building.Locked)
if (IsTraitPaused || (building != null && building.Locked))

This comment has been minimized.

@pchote

pchote Jan 16, 2018

Member

This should check IsTraitDisabled too.

@@ -154,14 +154,17 @@ public Actor FindBaseProvider(World world, Player p, CPos topLeft)

foreach (var bp in world.ActorsWithTrait<BaseProvider>())
{
if (bp.Trait.IsTraitDisabled || bp.Trait.IsTraitPaused)

This comment has been minimized.

@pchote

pchote Jan 16, 2018

Member

This can be removed. The bp.Trait.Ready() 3 lines down (should) already check both of these.

@@ -239,7 +242,7 @@ public IEnumerable<IRenderable> Render(WorldRenderer wr, World w, ActorInfo ai,
if (!RequiresBaseProvider)
return SpriteRenderable.None;

return w.ActorsWithTrait<BaseProvider>().SelectMany(a => a.Trait.RangeCircleRenderables(wr));
return w.ActorsWithTrait<BaseProvider>().Where(a => !a.Trait.IsTraitDisabled).SelectMany(a => a.Trait.RangeCircleRenderables(wr));

This comment has been minimized.

@pchote

pchote Jan 16, 2018

Member

RangeCircleRenderables already includes a disabled check, so this can stay as it was.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:conditional-base-provider branch from c82ca99 to 3f8fba1 Jan 16, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

MustaphaTR commented Jan 16, 2018

Updated.

@@ -82,6 +81,9 @@ bool ValidRenderPlayer()

public IEnumerable<IRenderable> RangeCircleRenderables(WorldRenderer wr)
{
if (IsTraitDisabled)
yield break;

// Visible to player and allies
if (!ValidRenderPlayer())
yield break;

This comment has been minimized.

@MunWolf

MunWolf Feb 19, 2018

Contributor

Why not add it as an or statement instead of multiple ifs?

This comment has been minimized.

@GraionDilach

GraionDilach Feb 19, 2018

Contributor

This is how our existing conventions of this property is defined usually tbh.

@Mailaender
Copy link
Member

Mailaender left a comment

Good looks good.

@reaperrr reaperrr merged commit c976bb1 into OpenRA:bleed Mar 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:conditional-base-provider branch Mar 8, 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.