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

Fix Mobile not sanitizing target location of Move orders #13715

Merged
merged 1 commit into from Aug 13, 2017

Conversation

Projects
None yet
5 participants
@obrakmann
Contributor

obrakmann commented Jul 24, 2017

Fixes #13707.

In #13528, the call to Map.Clamp got lost along the way.

@obrakmann obrakmann added this to the Next Release milestone Jul 24, 2017

@rob-v

one question for confirmation and some notes:
in release/playtest, Move/Ctrl/Alt Moves work with target outside map, but A-Move not.
probably not caused by this PR, but there is difference in target point, see next pictures.

if (!Info.MoveIntoShroud && !self.Owner.Shroud.IsExplored(order.TargetLocation))
var loc = self.World.Map.Clamp(order.TargetLocation);
if (!Info.MoveIntoShroud && !self.Owner.Shroud.IsExplored(loc))

This comment has been minimized.

@rob-v

rob-v Aug 5, 2017

Contributor

note: in release IsExplored is checked with TargetLocation, not after Clamp

@rob-v

rob-v Aug 5, 2017

Contributor

note: in release IsExplored is checked with TargetLocation, not after Clamp

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Aug 5, 2017

Contributor

release:
image
playtest:
image

Contributor

rob-v commented Aug 5, 2017

release:
image
playtest:
image

@reaperrr

This comment has been minimized.

Show comment
Hide comment
@reaperrr

reaperrr Aug 5, 2017

Contributor

Might be a missing a CanEnterCell or domain check.

Contributor

reaperrr commented Aug 5, 2017

Might be a missing a CanEnterCell or domain check.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Aug 12, 2017

Contributor

for me 👍 . I wanted only to note/confirm if it is ok:

  1. IsExplored(order.TargetLocation) before vs IsExplored(loc) now (loc after Clamp)
  2. different target point
    @abcdefg30 What should be fixed according to you?
    I ping @pchote as this is related to his PRs.
Contributor

rob-v commented Aug 12, 2017

for me 👍 . I wanted only to note/confirm if it is ok:

  1. IsExplored(order.TargetLocation) before vs IsExplored(loc) now (loc after Clamp)
  2. different target point
    @abcdefg30 What should be fixed according to you?
    I ping @pchote as this is related to his PRs.
@pchote

This comment has been minimized.

Show comment
Hide comment
@pchote

pchote Aug 12, 2017

Member

@rob-v: I'm not sure about your "different target point" screenshots: these are different parts of the map, so there are many reasons unrelated to these changes which could cause the behaviour differences. If the behaviour is different for the same target locations then we indeed have a problem.

Member

pchote commented Aug 12, 2017

@rob-v: I'm not sure about your "different target point" screenshots: these are different parts of the map, so there are many reasons unrelated to these changes which could cause the behaviour differences. If the behaviour is different for the same target locations then we indeed have a problem.

@rob-v

This comment has been minimized.

Show comment
Hide comment
@rob-v

rob-v Aug 12, 2017

Contributor

different corners, but the same map and similar - you click outside map. I can retest with the same corner if preferred. iirc it happened for same part.

Contributor

rob-v commented Aug 12, 2017

different corners, but the same map and similar - you click outside map. I can retest with the same corner if preferred. iirc it happened for same part.

@abcdefg30

This comment has been minimized.

Show comment
Hide comment
@abcdefg30

abcdefg30 Aug 13, 2017

Member

@abcdefg30 What should be fixed according to you?

I was referring to your findings in #13715 (comment).

This also needs a rebase.

Member

abcdefg30 commented Aug 13, 2017

@abcdefg30 What should be fixed according to you?

I was referring to your findings in #13715 (comment).

This also needs a rebase.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Aug 13, 2017

Contributor

@rob-v is right, there is a difference there between last release and playtest/bleed. I'm not sure if it's related to this part of the code, though.

Contributor

obrakmann commented Aug 13, 2017

@rob-v is right, there is a difference there between last release and playtest/bleed. I'm not sure if it's related to this part of the code, though.

@obrakmann

This comment has been minimized.

Show comment
Hide comment
@obrakmann

obrakmann Aug 13, 2017

Contributor

Actually, after trying to bisect this, there isn't a difference after all.

Also: rebased.

Contributor

obrakmann commented Aug 13, 2017

Actually, after trying to bisect this, there isn't a difference after all.

Also: rebased.

@pchote

pchote approved these changes Aug 13, 2017

@pchote pchote merged commit 8f26b4e into OpenRA:bleed Aug 13, 2017

2 checks passed

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

@obrakmann obrakmann deleted the obrakmann:fix13707_fix-unsanitized-move-order-locations branch Aug 13, 2017

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