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

Fix an integer overflow when calculating selection priorities. #17686

Merged
merged 1 commit into from Feb 16, 2020

Conversation

@pchote
Copy link
Member

pchote commented Feb 15, 2020

This PR fixes a regression introduced by #17291 - the actual bug is much older, but the bad behaviour didn't break anything obvious ingame before that PR.

Repro case:

  • Start a skirmish in TD
  • Enable left-click / classic orders
  • Disable the shroud
  • Select one of the starting forces and mouse-over the enemy construction yard.

Before: the attack cursor would disappear when mousing over the middle of the conyard.
After: the attack cursor works over the entire actor.

Closes #17681.

Its a shame that nobody thought to report this earlier, otherwise this could have been included in the hotfix.

@pchote pchote added this to the Next Release milestone Feb 15, 2020
@Mailaender

This comment has been minimized.

Copy link
Member

Mailaender commented Feb 15, 2020

Can you explain why 16 is the correct value here?

@pchote

This comment has been minimized.

Copy link
Member Author

pchote commented Feb 15, 2020

Both pixelDistance and the selection priority are generally going to be less than 100, so anything bigger than 7 would work without issues. Halving from 32 to 16 is arbitrary, but it means that the selection priority could be treated as a short in the future without worrying about accidental gameplay changes.

@reaperrr reaperrr merged commit 41d7a2d into OpenRA:bleed Feb 16, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

3 participants
You can’t perform that action at this time.