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

Adjust IsAtGroundLevel fixing EjectOnDeath #13892

Merged
merged 1 commit into from Aug 26, 2017

Conversation

Projects
None yet
6 participants
@abcdefg30
Member

abcdefg30 commented Aug 23, 2017

Fixes EjectOnDeath ejecting pilots even when at ground level. The aircraft actor is already dead the time EjectOnDeath runs and thus self.IsAtGroundLevel was always false.

I didn't notice any side-effects from removing the (unnecessary) IsDead check from IsAtGroundLevel.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Aug 23, 2017

Contributor

What is the bug here again?

Contributor

GraionDilach commented Aug 23, 2017

What is the bug here again?

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 24, 2017

Contributor

What is the bug here again?

That this https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.Mods.Common/Traits/EjectOnDeath.cs#L69 would always return true even at ground level, because the actor with EjectOnDeath is obviously already dead at this point (this whole stuff is triggered via Killed, afterall).

Contributor

reaperrr commented Aug 24, 2017

What is the bug here again?

That this https://github.com/OpenRA/OpenRA/blob/bleed/OpenRA.Mods.Common/Traits/EjectOnDeath.cs#L69 would always return true even at ground level, because the actor with EjectOnDeath is obviously already dead at this point (this whole stuff is triggered via Killed, afterall).

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Aug 24, 2017

Contributor

Ah, okay, it still doesn't affect the possibility of ejecting pilots on the ground, carry on.

Contributor

GraionDilach commented Aug 24, 2017

Ah, okay, it still doesn't affect the possibility of ejecting pilots on the ground, carry on.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 24, 2017

Member

On that topic: EjectInAir and EjectOnGround have been made redundant by conditions, and should be removed (but not in this PR).

Member

pchote commented Aug 24, 2017

On that topic: EjectInAir and EjectOnGround have been made redundant by conditions, and should be removed (but not in this PR).

@Phrohdoh

This comment has been minimized.

Show comment
Hide comment
@Phrohdoh

Phrohdoh Aug 25, 2017

Member

@pchote You stated on IRC that removing this check was dangerous. Is that no longer the case?

Member

Phrohdoh commented Aug 25, 2017

@pchote You stated on IRC that removing this check was dangerous. Is that no longer the case?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 25, 2017

Member

Indeed. Calling Location or CenterPosition used to potentially do a trait lookup, but that is now done unconditionally during actor creation, which makes it safe.

Member

pchote commented Aug 25, 2017

Indeed. Calling Location or CenterPosition used to potentially do a trait lookup, but that is now done unconditionally during actor creation, which makes it safe.

@chrisforbes chrisforbes merged commit d6d8cf0 into OpenRA:bleed Aug 26, 2017

2 checks passed

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

@abcdefg30 abcdefg30 deleted the abcdefg30:ejectOnGround branch Sep 11, 2017

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