Fixed #7472 so that cargo is only shown on allied vehicles #7486

Merged
merged 1 commit into from Feb 21, 2015

Conversation

Projects
None yet
5 participants
@pevers
Contributor

pevers commented Feb 16, 2015

Fixed issue #7472 so that only allied vehicle pips are shown. Did the check for EffectiveOwner to show the pips when a vehicle is disguised as ally.

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Feb 16, 2015

Member

The automatic build failed because of

OpenRA.Game/Traits/SelectionDecorations.cs:L86: [SA1000] The spacing around the keyword 'if' is invalid.

Run the utility command --check-code-style to make sure there aren't any issues after fixing this and push again ;)

Member

penev92 commented Feb 16, 2015

The automatic build failed because of

OpenRA.Game/Traits/SelectionDecorations.cs:L86: [SA1000] The spacing around the keyword 'if' is invalid.

Run the utility command --check-code-style to make sure there aren't any issues after fixing this and push again ;)

@@ -78,6 +78,14 @@ IEnumerable<IRenderable> DrawPips(WorldRenderer wr, Actor self, int2 basePositio
if (!pipSources.Any())
yield break;
+ // Don't draw pips if this is the enemy vehicle
+ var isAlly = self.Owner.IsAlliedWith(self.World.LocalPlayer)

This comment has been minimized.

@pchote

pchote Feb 16, 2015

Member

This looks like it will break the ability to view decorations if the "disable shroud" cheat is activated (which actually does a lot more than just disable shroud).

@pchote

pchote Feb 16, 2015

Member

This looks like it will break the ability to view decorations if the "disable shroud" cheat is activated (which actually does a lot more than just disable shroud).

@penev92 penev92 modified the milestone: Next release Feb 19, 2015

@@ -78,6 +78,13 @@ IEnumerable<IRenderable> DrawPips(WorldRenderer wr, Actor self, int2 basePositio
if (!pipSources.Any())
yield break;
+ var isAlly = self.Owner.IsAlliedWith(self.World.LocalPlayer)

This comment has been minimized.

@pchote

pchote Feb 19, 2015

Member

Sorry, I wasn't really clear with my previous comment: These checks need to be against self.World.RenderPlayer in order for it to behave correctly with replays and the disable shroud cheat.

@pchote

pchote Feb 19, 2015

Member

Sorry, I wasn't really clear with my previous comment: These checks need to be against self.World.RenderPlayer in order for it to behave correctly with replays and the disable shroud cheat.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Feb 19, 2015

Member

The real bug is at line 41, which does an AND check instead of an OR. Fixing that should be sufficient to fix the issue without any new code.

Member

pchote commented Feb 19, 2015

The real bug is at line 41, which does an AND check instead of an OR. Fixing that should be sufficient to fix the issue without any new code.

@Phrohdoh

This comment has been minimized.

Show comment
Hide comment
@Phrohdoh

Phrohdoh Feb 19, 2015

Member

These commits will need to be squashed.

Did the check for EffectiveOwner to show the pips when a vehicle is disguised as ally.

Very nice idea there!

A bit off-topic: We could implement a FakePips for this scenario (in another PR).

Member

Phrohdoh commented Feb 19, 2015

These commits will need to be squashed.

Did the check for EffectiveOwner to show the pips when a vehicle is disguised as ally.

Very nice idea there!

A bit off-topic: We could implement a FakePips for this scenario (in another PR).

@pevers

This comment has been minimized.

Show comment
Hide comment
@pevers

pevers Feb 19, 2015

Contributor

@Phrohdoh FakePips is a good idea I think!

Contributor

pevers commented Feb 19, 2015

@Phrohdoh FakePips is a good idea I think!

Fixed #7472 so that cargo is only shown on allied vehicles
fixed spacing

pips are shown on every team when shroud is disabled

removed comment

Remove unwanted comment explaining how ally-detection works

fixed the real problem in RenderAfterWorld
@pevers

This comment has been minimized.

Show comment
Hide comment
@pevers

pevers Feb 19, 2015

Contributor

I have squashed the commits.

Contributor

pevers commented Feb 19, 2015

I have squashed the commits.

@@ -136,4 +136,4 @@ IEnumerable<IRenderable> DrawTags(WorldRenderer wr, Actor self, int2 basePositio
}
}
}
-}

This comment has been minimized.

@obrakmann

obrakmann Feb 20, 2015

Contributor

Please try to avoid committing those whitespace changes.

@obrakmann

obrakmann Feb 20, 2015

Contributor

Please try to avoid committing those whitespace changes.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Feb 20, 2015

Contributor

This fixed it, 👍

Contributor

obrakmann commented Feb 20, 2015

This fixed it, 👍

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Feb 21, 2015

Member

Looks nice ingame 👍

On an unrelated topic, I just got my dog to enter an enemy APC...

Member

penev92 commented Feb 21, 2015

Looks nice ingame 👍

On an unrelated topic, I just got my dog to enter an enemy APC...

penev92 added a commit that referenced this pull request Feb 21, 2015

Merge pull request #7486 from delftswa2014/bugfix/apcbibs
Fixed #7472 so that cargo is only shown on allied vehicles

@penev92 penev92 merged commit 20ca9b2 into OpenRA:bleed Feb 21, 2015

2 checks passed

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

@jabbink jabbink deleted the delftswa2014:bugfix/apcbibs branch Apr 9, 2015

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