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

Consider disabled armaments invalid #13856

Merged
merged 2 commits into from Aug 22, 2017

Conversation

Projects
None yet
3 participants
@reaperrr
Contributor

reaperrr commented Aug 17, 2017

Fixes #13855.

Possibly a candidate for stable, since it's an easy and obvious fix.

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 17, 2017

Contributor

@MustaphaTR Can you confirm that this fixes #13855?

Contributor

reaperrr commented Aug 17, 2017

@MustaphaTR Can you confirm that this fixes #13855?

@MustaphaTR

👍

@reaperrr reaperrr added this to the Next Release milestone Aug 17, 2017

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 18, 2017

Contributor

Updated.

Contributor

reaperrr commented Aug 18, 2017

Updated.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 18, 2017

Member

At the risk of prolonging this PR further... CombatDebugOverlay, AttackGarrisoned, and FlyAttack look like the need similar fixes. Find uses of AttackBase.Armaments to confirm.

It may make more sense to move the check into the definition of getArmaments instead?

Member

pchote commented Aug 18, 2017

At the risk of prolonging this PR further... CombatDebugOverlay, AttackGarrisoned, and FlyAttack look like the need similar fixes. Find uses of AttackBase.Armaments to confirm.

It may make more sense to move the check into the definition of getArmaments instead?

Properly account for disabled Armaments in various places
These places didn't care if an Armament was disabled, which could lead to unexpected behavior.
@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 19, 2017

Contributor

It may make more sense to move the check into the definition of getArmaments instead?

getArmaments currently runs only once at actor creation.
I don't see a simple, performant way to change that, so I've gone for fixing it at the places that use AB.Armaments.

Updated.

Contributor

reaperrr commented Aug 19, 2017

It may make more sense to move the check into the definition of getArmaments instead?

getArmaments currently runs only once at actor creation.
I don't see a simple, performant way to change that, so I've gone for fixing it at the places that use AB.Armaments.

Updated.

@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 19, 2017

Member

getArmaments currently runs only once at actor creation.

For this I was thinking of the .ToArray().Where(Exts.TraitIsEnabled) enumerable caching that we do in several other places. But this can be addressed in a future PR if we want to go that way. For now lets just get the fixes merged.

Member

pchote commented Aug 19, 2017

getArmaments currently runs only once at actor creation.

For this I was thinking of the .ToArray().Where(Exts.TraitIsEnabled) enumerable caching that we do in several other places. But this can be addressed in a future PR if we want to go that way. For now lets just get the fixes merged.

@pchote

pchote approved these changes Aug 22, 2017

@pchote pchote merged commit c75e64a into OpenRA:bleed Aug 22, 2017

2 checks passed

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

@reaperrr reaperrr deleted the reaperrr:fix-ValidWeapons branch Mar 9, 2018

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