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

Backward compatibility fix for OOP_GetElementBoundingBox returning vectors post-r13998 #332

Merged
merged 2 commits into from Aug 22, 2018

Conversation

Projects
3 participants
@Addlibs
Contributor

Addlibs commented Aug 22, 2018

After #305 was merged, getBoundingBox OOP method started returning vectors in old code when run on the new client, instead of returning 6 floats as was previously the case.
The backward compatibility check was inverted, as the function returned the 6 legacy floats if min_mta_version was higher than r13999 where it should do so only if it was lower. Additionally, it lacked any warnings or info about this change in behaviour.

This PR fixes MinClientReqCheck which was wrong way round in OOP_GetElementBoundingBox.

It also adds a warning on the server console about modified functionality after 1.5.5-9.13999, consistent with getComponentPosition and getComponentRotation OOP return modifications.

WARNING: resource/scriptfile (Line x) [Client] getBoundingBox will return 6 floats instead of 2 Vector3 because <min_mta_version> Client setting in meta.xml is below 1.5.5-9.13999

New behaviour:
OOP getBoundingBox() method will continue to return 6 floats unless min_mta_version client is set to 1.5.5-9.13999 or higher in the meta.xml, in which case it'll return vectors

@patrikjuvonen patrikjuvonen added the bug label Aug 22, 2018

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

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

@qaisjp qaisjp requested a review from patrikjuvonen Aug 22, 2018

@qaisjp qaisjp self-assigned this Aug 22, 2018

release/v1.5.6 automation moved this from In progress to Ready Aug 22, 2018

@qaisjp

qaisjp approved these changes Aug 22, 2018

lgtm but requesting a review from @patrikjuvonen as he reviewed the previous PR

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Aug 22, 2018

Collaborator

I should've been more explicit on what I reviewed in the previous PR. I only reviewed the part he requested to be commented on, I wasn't reviewing the whole PR. It will take me a while to get to this PR but I'll keep this PR in mind.

Collaborator

patrikjuvonen commented Aug 22, 2018

I should've been more explicit on what I reviewed in the previous PR. I only reviewed the part he requested to be commented on, I wasn't reviewing the whole PR. It will take me a while to get to this PR but I'll keep this PR in mind.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Aug 22, 2018

Member

Oh okay. No problem. In that case am I okay to dismiss your review request?

Member

qaisjp commented Aug 22, 2018

Oh okay. No problem. In that case am I okay to dismiss your review request?

@patrikjuvonen

This comment has been minimized.

Show comment
Hide comment
@patrikjuvonen

patrikjuvonen Aug 22, 2018

Collaborator

Feel free to dismiss my review request. My apologies for any inconvenience.

Collaborator

patrikjuvonen commented Aug 22, 2018

Feel free to dismiss my review request. My apologies for any inconvenience.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Aug 22, 2018

Member

don't worry about it

Member

qaisjp commented Aug 22, 2018

don't worry about it

@qaisjp qaisjp merged commit 2918d95 into multitheftauto:master Aug 22, 2018

3 checks passed

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

release/v1.5.6 automation moved this from Ready to Done Aug 22, 2018

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