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

Add RevealOnDeath trait #12999

Merged
merged 5 commits into from May 1, 2017

Conversation

Projects
None yet
7 participants
@reaperrr
Contributor

reaperrr commented Mar 19, 2017

...and use it in D2k to reveal las actor position for 4 seconds.

Note: RevealOnFire still needs to stay on sandworm, because swallowing doesn't trigger regular death.

Closes #7916.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Mar 19, 2017

Contributor

Updated.

Contributor

reaperrr commented Mar 19, 2017

Updated.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Apr 1, 2017

Contributor

Could this have a damagetype setting?

Contributor

GraionDilach commented Apr 1, 2017

Could this have a damagetype setting?

@penev92

This comment has been minimized.

Show comment
Hide comment
@penev92

penev92 Apr 2, 2017

Member

I have made a case against arbitrary default values on traits many times in the past. I may have fallen out of the loop and things may be done this way now (again), but I'd still like to raise my concern about this practice. Just my two cents, seeing as I am sadly no longer an active member of the team.

Member

penev92 commented Apr 2, 2017

I have made a case against arbitrary default values on traits many times in the past. I may have fallen out of the loop and things may be done this way now (again), but I'd still like to raise my concern about this practice. Just my two cents, seeing as I am sadly no longer an active member of the team.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Apr 2, 2017

Contributor

@penev92 Arbitrary default values are a problem if they rely on a value based on other actors - Aircraft's RepairableBuildings, for example - but when they work with every possible mod (due to being a global setup), then I don't see where is the problem.

Contributor

GraionDilach commented Apr 2, 2017

@penev92 Arbitrary default values are a problem if they rely on a value based on other actors - Aircraft's RepairableBuildings, for example - but when they work with every possible mod (due to being a global setup), then I don't see where is the problem.

@SoScared

This comment has been minimized.

Show comment
Hide comment
@SoScared

SoScared Apr 4, 2017

Member

Very exciting! Would this be able to affect the position of aircraft crashes as well?

*edit: I seem to be able to produce RevealOnDeath with an airborne YAK's last position but not with the crash site position(YAK Husk?).

Member

SoScared commented Apr 4, 2017

Very exciting! Would this be able to affect the position of aircraft crashes as well?

*edit: I seem to be able to produce RevealOnDeath with an airborne YAK's last position but not with the crash site position(YAK Husk?).

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Apr 12, 2017

Contributor

but not with the crash site position(YAK Husk?).

pinging @pchote: The FallToEarth activity uses Dispose instead of Kill. I think it's too risky to count a Dispose as 'Death' in my trait, so my question is, would it cause any trouble to change FallToEarth activity to use Kill instead?

Contributor

reaperrr commented Apr 12, 2017

but not with the crash site position(YAK Husk?).

pinging @pchote: The FallToEarth activity uses Dispose instead of Kill. I think it's too risky to count a Dispose as 'Death' in my trait, so my question is, would it cause any trouble to change FallToEarth activity to use Kill instead?

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 12, 2017

Member

As long as the husk doesn't have UpdatesPlayerStatistics or any other traits that implement INotifyKilled it should (but no guarantees) be fine. This would also let us fix the TS jumpjet death animation.

Member

pchote commented Apr 12, 2017

As long as the husk doesn't have UpdatesPlayerStatistics or any other traits that implement INotifyKilled it should (but no guarantees) be fine. This would also let us fix the TS jumpjet death animation.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Apr 14, 2017

Contributor

Updated, added a commit that makes FallToEarth kill the actor instead of disposing it, allowing aircraft husks to use RevealOnDeath.

Contributor

reaperrr commented Apr 14, 2017

Updated, added a commit that makes FallToEarth kill the actor instead of disposing it, allowing aircraft husks to use RevealOnDeath.

@GraionDilach

This comment has been minimized.

Show comment
Hide comment
@GraionDilach

GraionDilach Apr 17, 2017

Contributor

I still request a DamageType setting on this, but 👍 after that.

Contributor

GraionDilach commented Apr 17, 2017

I still request a DamageType setting on this, but 👍 after that.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Apr 18, 2017

Contributor

Looks good, but I agree that adding a check for DamageTypes might be useful.

Contributor

obrakmann commented Apr 18, 2017

Looks good, but I agree that adding a check for DamageTypes might be useful.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Apr 24, 2017

Contributor

Updated.
Travis didn't run, but I guess you can make check/test to compensate for that.

Contributor

reaperrr commented Apr 24, 2017

Updated.
Travis didn't run, but I guess you can make check/test to compensate for that.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Apr 29, 2017

Member

Why not include this on buildings or carryalls?

Member

pchote commented Apr 29, 2017

Why not include this on buildings or carryalls?

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Apr 30, 2017

Contributor

Why not include this on buildings or carryalls?

Updated.

Contributor

reaperrr commented Apr 30, 2017

Why not include this on buildings or carryalls?

Updated.

@GraionDilach

👍

@pchote

pchote approved these changes May 1, 2017 edited

LGTM 👍. Just one minor issue then we can merge:

Show outdated Hide outdated mods/d2k/rules/aircraft.yaml
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr May 1, 2017

Contributor

Updated.

Contributor

reaperrr commented May 1, 2017

Updated.

@pchote pchote merged commit 7a7b668 into OpenRA:bleed May 1, 2017

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@reaperrr reaperrr deleted the reaperrr:RevealOnDeath branch Jul 23, 2017

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