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

Add GrantConditionOnHealth #16224

Merged
merged 2 commits into from Mar 21, 2019

Conversation

@MustaphaTR
Copy link
Member

commented Feb 21, 2019

GrantConditionOnDamageState doesn't allow enough accuracy on when to grant the condition. On Generals Alpha i had this trait to make Battle Bus turn immobile when damaged, which happens at 65/105 health.

@GraionDilach

This comment has been minimized.

Copy link
Contributor

commented Feb 21, 2019

IIRC the map editor Health property is using percentages and not exact values. These should be consistent IMO.

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Feb 21, 2019

Even tho i like consistency, i disagree. The whole point of the trait is giving more control over the conditions, using percentanges would increase the possible points from 5 to 100 but still not enought to properly get 65/105 for example as i said above.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:gcohealth branch 2 times, most recently from c5268f9 to 94273e0 Feb 21, 2019

@abcdefg30
Copy link
Member

left a comment

Looks good to me otherwise.

@MustaphaTR

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

Updated.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:gcohealth branch from 5f8398b to b8f93c0 Mar 15, 2019


if (!granted)
GrantConditionOnValidHealth(self, health.HP);
else if (granted && (info.MinHP > health.HP || maxHP < health.HP))

This comment has been minimized.

Copy link
@Phrohdoh

Phrohdoh Mar 17, 2019

Member

Would it be better to use >= and <= which would make the range values inclusive instead of exclusive?

IMO when writing Min: 120, for example, I'd expect the condition to be granted when the actor's hp is >= 120 <= max instead of > 120.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 17, 2019

Contributor

This else if is for revoking the condition, so using exclusive values is correct, I think.


namespace OpenRA.Mods.Common.Traits
{
[Desc("Applies a condition to the actor at when its health is between 2 specific values.")]

This comment has been minimized.

Copy link
@Phrohdoh

Phrohdoh Mar 17, 2019

Member

The wording on this could be a bit better.

For example:

Applies a condition to this actor when its health is between 2 values

Also see my comment on inclusive vs exclusive range which would change the wording of this.

Either way this trait's docs should mention whether the values are inclusive or exclusive.

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 21, 2019

Contributor

Those trait descs currently aren't really visible anywhere outside the code itself, and the descs of MinHP/MaxHP implicitly mention that they're inclusive.

This comment has been minimized.

Copy link
@Phrohdoh

Phrohdoh Mar 21, 2019

Member

The trait descs are directly visible on the trait wiki page.

You can't "implicitly mention" something...

This comment has been minimized.

Copy link
@reaperrr

reaperrr Mar 21, 2019

Contributor

The trait descs are directly visible on the trait wiki page.

Ok, I guess I missed that.

You can't "implicitly mention" something...

I meant he didn't explicitly use 'inclusive' or 'includes', but "Maximum level (...) at which (...)", but I see your point.

@reaperrr reaperrr merged commit ac7d2e0 into OpenRA:bleed Mar 21, 2019

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:gcohealth branch Mar 21, 2019

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