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 collision detection perspective ratios #5913

Merged
merged 2 commits into from
Jan 3, 2018
Merged

Conversation

ChrisLoer
Copy link
Contributor

Fixes issue #5911.

  • Box and circle collision detection were using incorrectly inverted perspective ratios. [(1/(.5 + .5 * y/x) instead of (.5 + .5 * x/y)].
  • Debug collision circles were using the same incorrect ratio (so they matched actual collision detection).
  • Debug collision boxes were right the whole time (this is how we noticed there was a problem).
  • pixelsToTileUnits incorrectly included perspective ratio: it's used for measuring geometry, not text, so "scale" is relevant but "perspective ratio" isn't.

'collision-pitched-lines' exposed the behavior, but it was too complicated a test for us to be able to understand the problem.

I'll try to construct a regression test that clearly exercises the old vs. new behavior, but probably won't get around to it until the new year.

/cc @ansis @anandthakker @jfirebaugh

 - Box and circle collision detection were using incorrectly inverted perspective ratios. [(1/(.5 + .5 * y/x) instead of (.5 + .5 * x/y)].
 - Debug circles were using the same incorrect ratio (so they matched actual collision detection)
 - pixelsToTileUnits incorrectly included perspective ratio: it's used for measuring geometry, not text, so "scale" is relevant but "perspective ratio" isn't.

'collision-pitched-lines' exposed the behavior, but it was too complicated a test for us to be able to understand the problem.
@ChrisLoer
Copy link
Contributor Author

Pushed a regression test that isolates the behavior from all the noise in the other collision render tests that are affected by this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants