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

0009742: add support for Vec2s on createColPolygon client-side and allow passing in more Vec2s server-side #217

Merged
merged 2 commits into from Jul 5, 2018

Conversation

Projects
4 participants
@patrikjuvonen
Collaborator

patrikjuvonen commented Jul 3, 2018

Mantis Bug Tracker issue:
9742

Co-contributors:
@4O4 (PR)

I was fixing someone's Lua script and that's how I encountered this bug. I searched for the issue on Mantis Bug Tracker and then I found a link to @4O4's old PR which I reviewed. My patch is slightly different. I read the comments @qaisjp had written. I did not directly copy any code from his PR, but I do want to give him credits obviously for delivering a patch before me.

Function:
createColPolygon

Summary:

  • Passing in Vec2s client-side was not working;
  • Passing in Vec2s server-side was limited to 4;
  • Neither of which threw errors if not all 3 required points were given (due to it reading argStream wrong);
  • Pretty small and simple fix, easy to test.

Testing:
While I could provide my own tests, I found that @4O4's tests do the same thing. All tests passed and making complicated polygon colshapes is working.

patrikjuvonen added some commits Jul 3, 2018

@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 Jul 4, 2018

Contributor

There was a nice PR-merge streak in this project recently so I really do hope this PR won't get stuck like the one I created 😄

Contributor

4O4 commented Jul 4, 2018

There was a nice PR-merge streak in this project recently so I really do hope this PR won't get stuck like the one I created 😄

@saml1er

This comment has been minimized.

Show comment
Hide comment
@saml1er

saml1er Jul 4, 2018

Member

Good job! There's not much code to review, if it works properly, I'll be happy to merge.

Member

saml1er commented Jul 4, 2018

Good job! There's not much code to review, if it works properly, I'll be happy to merge.

@qaisjp

qaisjp approved these changes Jul 5, 2018

@qaisjp qaisjp merged commit a24b3ab into multitheftauto:master Jul 5, 2018

2 checks passed

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

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Jul 5, 2018

Member

Thank you both @4O4 and @patrikjuvonen. Good work.

Member

qaisjp commented Jul 5, 2018

Thank you both @4O4 and @patrikjuvonen. Good work.

@patrikjuvonen patrikjuvonen deleted the patrikjuvonen:issue-9742 branch Jul 5, 2018

@patrikjuvonen patrikjuvonen added the bug label Aug 7, 2018

@patrikjuvonen patrikjuvonen added this to In progress in release/v1.5.6 via automation Aug 7, 2018

@patrikjuvonen patrikjuvonen added this to the 1.5.6 milestone Aug 7, 2018

@patrikjuvonen patrikjuvonen moved this from In progress to Done in release/v1.5.6 Aug 7, 2018

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