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

Remove Negative Damage Weapon Full Health Check #15751

Merged
merged 2 commits into from Nov 21, 2018

Conversation

Projects
None yet
4 participants
@MustaphaTR
Copy link
Member

MustaphaTR commented Oct 29, 2018

The behaviour can be replicated with conditions and currently non-damage warheads on the weapon can prevent this check from properly working.

We talked on irc that some warheads like CreateEffect shouldn't effect the targeting regardless, but i haven't done it yet because couldn't figure how exactly to do it. It can be done in a seperate PR.

I also put an Update rule that notifies about this change, but doesn't actually edit anything.

Show resolved Hide resolved mods/ra/rules/defaults.yaml Outdated

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:remove-hardcoded-heal-health-check branch from bda9d79 to 12a3985 Oct 29, 2018

Show resolved Hide resolved mods/ra/rules/defaults.yaml Outdated
Show resolved Hide resolved mods/ra/rules/defaults.yaml Outdated
Show resolved Hide resolved mods/ts/rules/defaults.yaml Outdated

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:remove-hardcoded-heal-health-check branch from 12a3985 to 76f8227 Oct 31, 2018

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:remove-hardcoded-heal-health-check branch from 76f8227 to 8fdaa14 Oct 31, 2018

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 3, 2018

Also needs a rebase.

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:remove-hardcoded-heal-health-check branch from 8fdaa14 to d65219b Nov 3, 2018

@pchote
Copy link
Member

pchote left a comment

RA mechanics can no longer repair vehicles, and I don't see an immediately obvious reason why.

Show resolved Hide resolved mods/ra/rules/defaults.yaml Outdated

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:remove-hardcoded-heal-health-check branch 2 times, most recently from 7145f66 to 76abe7a Nov 4, 2018

TargetTypes: Repair
GrantConditionOnDamageState@DAMAGED:
Condition: damaged
ValidDamageStates: Light, Medium, Heavy, Critical

This comment has been minimized.

@pchote

pchote Nov 4, 2018

Member

IMO it makes the rules much easier to read if the fields that determine whether a trait is active (RequiresCondition, RequiresPrerequisites, Faction – and in this case ValidDamageStates) go at the top, before fields that define what the trait does when it is active (in this case grant Condition).

I don't think this needs to block merging here, but it is something I would like to start applying to our default mods. We should consider adding a section to our coding standards with this and other conventions (e.g. for the trait @SUFFIXes and condition names) if we can agree on what the standards should be.

@pchote pchote added the PR: Needs +2 label Nov 4, 2018

@abcdefg30
Copy link
Member

abcdefg30 left a comment

Looks good otherwise.

@pchote

This comment has been minimized.

Copy link
Member

pchote commented Nov 17, 2018

Can we get this fixed up and merged?

@MustaphaTR MustaphaTR dismissed stale reviews from abcdefg30 and pchote via 5335aac Nov 17, 2018

@MustaphaTR MustaphaTR force-pushed the MustaphaTR:remove-hardcoded-heal-health-check branch from 76abe7a to 5335aac Nov 17, 2018

@MustaphaTR

This comment has been minimized.

Copy link
Member

MustaphaTR commented Nov 17, 2018

Updated.

@pchote pchote force-pushed the MustaphaTR:remove-hardcoded-heal-health-check branch from 5335aac to 702b166 Nov 19, 2018

@pchote
Copy link
Member

pchote left a comment

There were still, IMO, some issues with the English in the update rule. I have pushed over this with revised wording. LGTM now, but i'd prefer if @abcdefg30 or someone else could confirm a 👍 on the new wording before merging.

get
{
return "Negative-damage are no longer automatically invalid against targets that have full health.\n" +
"Previous behaviour can be restored by enabling a Target trait using GrantConditionOnDamageState.\n" +

This comment has been minimized.

@abcdefg30

abcdefg30 Nov 20, 2018

Member

Targetable trait.

{
get
{
return "Negative-damage are no longer automatically invalid against targets that have full health.\n" +

This comment has been minimized.

@reaperrr

reaperrr Nov 21, 2018

Contributor

This seems to be missing 'weapons' after the 'Negative-damage'.

@reaperrr reaperrr force-pushed the MustaphaTR:remove-hardcoded-heal-health-check branch from 702b166 to af76b62 Nov 21, 2018

@reaperrr

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 21, 2018

Fixed the two text issues me and @abcdefg30 pointed out, the rest looks fine, so lgtm.

@reaperrr reaperrr merged commit 5303257 into OpenRA:bleed Nov 21, 2018

2 checks passed

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

This comment has been minimized.

Copy link
Contributor

reaperrr commented Nov 21, 2018

@MustaphaTR MustaphaTR deleted the MustaphaTR:remove-hardcoded-heal-health-check branch Nov 21, 2018

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