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

Prevent unowned monsters from attacking adequately stealthed ships #1582

Merged
merged 1 commit into from May 24, 2017

Conversation

Projects
5 participants
@dbenage-cx
Member

dbenage-cx commented May 22, 2017

Unowned monsters are currently able to attack stealthed ships.

Test save with empire ship(65 stealth) vs unowned juggernaut (system: Pawn ⍺)
and unowned juggernaut vs empire ship(65 stealth) and owned monster(5 stealth) (system: Thrax β)
save-20170522_124712.zip

@dbenage-cx dbenage-cx added this to the v0.4.8 milestone May 22, 2017

@Dilvish-fo

This comment has been minimized.

Show comment
Hide comment
@Dilvish-fo

Dilvish-fo May 22, 2017

Member

From a quick look at the code, without testing, it looks like a good fix, but the commit message seems misleading to me. From the message it sounds like you are making stealthed ships absolutely unattackable by monsters. I would suggest changing the commit message to
Fix unowned monster detection check so that they are not always able to attack stealthed targets regardless of the monster's detection.

Member

Dilvish-fo commented May 22, 2017

From a quick look at the code, without testing, it looks like a good fix, but the commit message seems misleading to me. From the message it sounds like you are making stealthed ships absolutely unattackable by monsters. I would suggest changing the commit message to
Fix unowned monster detection check so that they are not always able to attack stealthed targets regardless of the monster's detection.

Fix unowned monster detection check during combat
Given an object that is not currently visible to an attacking unowned
monster, ObjectAttackableByEmpire will return true due to the
differing meaning of the empire id ALL_EMPIRES.

Resolve such cases by limiting use of ObjectAttackableByEmpire to
when attacking empire id is not ALL_EMPIRES.
@MatGB

This comment has been minimized.

Show comment
Hide comment
@MatGB

MatGB May 22, 2017

Member

Loaded the savegame and I can't see that the monster has ever attacked the stealthed ship, I get "Unknown empire cannot target Shelyak" each turn, which is how it should be, I've never noticed a monster attacking a ship it wasn't meant to be able to see and we did a lot of work rebalancing monstrous detection last year so that you could stealth past some monsters, I've definitely never had a guardian attack a stealthed ship and I'm fairly sure I'd have noticed a moving one.

Also, as I can't read the code properly, Dilvish is correct that the commit message is misleading, when I got the initial email I was very worried as some monsters (specifically the experimentor critters) need to be able to target high stealth ships and are balanced accordingly (the numbers aren't quite right but they're in the right ballpark).

Member

MatGB commented May 22, 2017

Loaded the savegame and I can't see that the monster has ever attacked the stealthed ship, I get "Unknown empire cannot target Shelyak" each turn, which is how it should be, I've never noticed a monster attacking a ship it wasn't meant to be able to see and we did a lot of work rebalancing monstrous detection last year so that you could stealth past some monsters, I've definitely never had a guardian attack a stealthed ship and I'm fairly sure I'd have noticed a moving one.

Also, as I can't read the code properly, Dilvish is correct that the commit message is misleading, when I got the initial email I was very worried as some monsters (specifically the experimentor critters) need to be able to target high stealth ships and are balanced accordingly (the numbers aren't quite right but they're in the right ballpark).

@dbenage-cx

This comment has been minimized.

Show comment
Hide comment
@dbenage-cx

dbenage-cx May 22, 2017

Member

@Dilvish-fo TYVM, as the suggested line is a bit over the recommended 50, I've reworded the summary and provided a more detailed explanation. Please let me know if it could be improved further.

@MatGB For me, this has been exceedingly common recently, not limited to this one case. Here are some logs portions of first bout to compare: master - PR
With master (same save game, empire ship has 65 stealth):
screenshot_20170522_123025

Member

dbenage-cx commented May 22, 2017

@Dilvish-fo TYVM, as the suggested line is a bit over the recommended 50, I've reworded the summary and provided a more detailed explanation. Please let me know if it could be improved further.

@MatGB For me, this has been exceedingly common recently, not limited to this one case. Here are some logs portions of first bout to compare: master - PR
With master (same save game, empire ship has 65 stealth):
screenshot_20170522_123025

@dbenage-cx dbenage-cx changed the title from Prevent unowned monsters from attacking stealthed ships to Prevent unowned monsters from attacking adequately stealthed ships May 22, 2017

@geoffthemedio

This comment has been minimized.

Show comment
Hide comment
@geoffthemedio

geoffthemedio May 24, 2017

Member

looks reasonable...

Member

geoffthemedio commented May 24, 2017

looks reasonable...

@geoffthemedio geoffthemedio merged commit 047ca69 into freeorion:master May 24, 2017

2 checks passed

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

@dbenage-cx dbenage-cx deleted the dbenage-cx:fix_monster_combat_stealth_enemy branch May 29, 2017

@Vezzra Vezzra added this to Proposed in 0.4.7.1 Bugfix Release Aug 13, 2017

@Vezzra Vezzra modified the milestones: 0.4.7.1 Bugfix Release, v0.4.8 Aug 20, 2017

@Vezzra Vezzra moved this from Proposed to Accepted in 0.4.7.1 Bugfix Release Aug 20, 2017

@Vezzra

This comment has been minimized.

Show comment
Hide comment
@Vezzra

Vezzra Aug 20, 2017

Member

@dbenage-cx, this PR has been proposed and accepted for inclusion into the 0.4.7.1 bugfix release, but when I try to cherry pick b95e34b into the release branch, I get conflicts I can't resolve (because I can't figure out what's wrong).

Can you do the cherry picking? The release branch has already been (re)created, so you can go ahead.

Member

Vezzra commented Aug 20, 2017

@dbenage-cx, this PR has been proposed and accepted for inclusion into the 0.4.7.1 bugfix release, but when I try to cherry pick b95e34b into the release branch, I get conflicts I can't resolve (because I can't figure out what's wrong).

Can you do the cherry picking? The release branch has already been (re)created, so you can go ahead.

@Vezzra

This comment has been minimized.

Show comment
Hide comment
@Vezzra

Vezzra Aug 20, 2017

Member

@dbenage-cx, nevermind, upon further investigation I realized this PR fixes a bug introduced post 0.4.7, so it does not apply for 0.4.7.1 anyway.

Member

Vezzra commented Aug 20, 2017

@dbenage-cx, nevermind, upon further investigation I realized this PR fixes a bug introduced post 0.4.7, so it does not apply for 0.4.7.1 anyway.

@Vezzra Vezzra modified the milestones: v0.4.8, 0.4.7.1 Bugfix Release Aug 20, 2017

@Vezzra Vezzra moved this from Accepted to Rejected in 0.4.7.1 Bugfix Release Aug 20, 2017

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