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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix vectors, required and optional parameters handling in ColShape.Polygon APIs (client and server) #163

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@4O4
Contributor

4O4 commented Oct 15, 2017

This pull request fixes the initial problem described in https://bugs.mtasa.com/view.php?id=9742 and also some additional problems on client side:

  • makes position and first 3 points to be required parameters (just like the wiki says and just like the server side API works)
  • adds support for Vectors as params in addition to plain numbers

Full test suite is available at:
https://github.com/4O4/mtasa-lua-tests/tree/master/%5Btests%5D/test-colshape-polygon
Obviously it fails miserably without this patch 馃槃

4O4 added some commits Oct 15, 2017

Fix createColPolygon client API required params and Vectors handling
Polygon Colshape must have at least 3 bound points and the position,
so first 4 Vector2D parameters are required. Without this patch it's
possible to create Polygon Colshape with just two or even one vertex
which is:
- totally different behaviour than on server side
- against the specification available in docs / wiki
- kind of useless

From now, the function will return false and no colshape will be created
if required parameters are not passed. Warning will be thrown just like
on server side. It will also accept Vector2 in addition to plain number
params.
Fix createColPolygon server API Vectors handling
It should now handle infinite number of optional Vector params while
still requiring at least 4 2D coords, one of which is the colshape
position and the rest are vertices.

@4O4 4O4 changed the title from Fix vectors, varargs and required parameters handling in ColShape.Polygon APIs (client and server) to Fix vectors, required and optional parameters handling in ColShape.Polygon APIs (client and server) Oct 15, 2017

{
CVector2D vecPoint;
argStream.ReadVector2D ( vecPoint );
vecPointList.push_back ( vecPoint );

This comment has been minimized.

@qaisjp

qaisjp Oct 16, 2017

Member

Why does vecPointList need to exist if its sole purpose is to perform pShape->AddPoint( vecPointList[i] );?

Can't this line be replaced with pShape->AddPoint( vecPoint );?

(This is an honest question. There's probably a very good reason as to why you're doing this!)

@qaisjp

qaisjp Oct 16, 2017

Member

Why does vecPointList need to exist if its sole purpose is to perform pShape->AddPoint( vecPointList[i] );?

Can't this line be replaced with pShape->AddPoint( vecPoint );?

(This is an honest question. There's probably a very good reason as to why you're doing this!)

This comment has been minimized.

@4O4

4O4 Oct 16, 2017

Contributor

It is needed to store first 4 required vectors (position + 3 points),. An alternative would be to declare 4 separate variables. And since the list already exists and must be iterated over anyway, I thought it's reasonable to reuse it by pushing optional points to it too. This way AddPoint is called only in one loop instead of two which is more clean IMHO :) I don't know much about performance aspects here though.

@4O4

4O4 Oct 16, 2017

Contributor

It is needed to store first 4 required vectors (position + 3 points),. An alternative would be to declare 4 separate variables. And since the list already exists and must be iterated over anyway, I thought it's reasonable to reuse it by pushing optional points to it too. This way AddPoint is called only in one loop instead of two which is more clean IMHO :) I don't know much about performance aspects here though.

CScriptArgReader argStream ( luaVM );
for ( uint i = 0; i < 4 || argStream.NextCouldBeNumber (); i++ )
for ( uint i = 0; i < 4; i++ )

This comment has been minimized.

@qaisjp

qaisjp Oct 16, 2017

Member

Why didn't (i < 4) || argStream.NextCouldBeNumber(); work before? It looks like it should work (when clearly it didn't).

Did you test with (i < 4) || argStream.NextIsVector2D(); first?

@qaisjp

qaisjp Oct 16, 2017

Member

Why didn't (i < 4) || argStream.NextCouldBeNumber(); work before? It looks like it should work (when clearly it didn't).

Did you test with (i < 4) || argStream.NextIsVector2D(); first?

This comment has been minimized.

@4O4

4O4 Oct 16, 2017

Contributor

The former did work fine with numbers only, it couldn't work with more than 3 points passed as vectors. The latter worked fine, I indeed tested it first. I guess I then decided to make Client and Server side code look more alike.
I could of course do it by simply using that for loop (with fixed conditon) in Client side code too instead of while 馃槃 But I think it's just a cosmetic thing and the main difference is direction of code sync. Will change this if needed :)

@4O4

4O4 Oct 16, 2017

Contributor

The former did work fine with numbers only, it couldn't work with more than 3 points passed as vectors. The latter worked fine, I indeed tested it first. I guess I then decided to make Client and Server side code look more alike.
I could of course do it by simply using that for loop (with fixed conditon) in Client side code too instead of while 馃槃 But I think it's just a cosmetic thing and the main difference is direction of code sync. Will change this if needed :)

@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 Jan 8, 2018

Contributor

Sooo... what's the status? Any chances for merge? :)

Contributor

4O4 commented Jan 8, 2018

Sooo... what's the status? Any chances for merge? :)

@Necktrox

This comment has been minimized.

Show comment
Hide comment
@Necktrox

Necktrox Jan 8, 2018

Contributor

Somebody has to test it :)

Contributor

Necktrox commented Jan 8, 2018

Somebody has to test it :)

@4O4

This comment has been minimized.

Show comment
Hide comment
@4O4

4O4 Jun 5, 2018

Contributor

Cleanup-closing, I don't like to have PRs hanging forever :) Feel free to reopen it or whatever when the right time comes.

@Necktrox @ccw808 @qaisjp Guys, do you maybe need someone to help you with testing and validating all of these PRs? There are lots of them and they are just getting more stale. Also the longer PR waits, the more difficult merging gets because of arising conflicts.

Contributor

4O4 commented Jun 5, 2018

Cleanup-closing, I don't like to have PRs hanging forever :) Feel free to reopen it or whatever when the right time comes.

@Necktrox @ccw808 @qaisjp Guys, do you maybe need someone to help you with testing and validating all of these PRs? There are lots of them and they are just getting more stale. Also the longer PR waits, the more difficult merging gets because of arising conflicts.

@qaisjp

This comment has been minimized.

Show comment
Hide comment
@qaisjp

qaisjp Aug 29, 2018

Member

To answer your question @4O4, we would definitely appreciate your help reviewing and testing pull requests! 馃檪

Member

qaisjp commented Aug 29, 2018

To answer your question @4O4, we would definitely appreciate your help reviewing and testing pull requests! 馃檪

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