garrysmod icon indicating copy to clipboard operation
garrysmod copied to clipboard

Update the vector Lua extention to include various handy methods.

Open dvdvideo1234 opened this issue 6 years ago • 26 comments

dvdvideo1234 avatar Oct 10 '17 12:10 dvdvideo1234

I would remove the nil check line, it will just lead to people not knowing when they sent in a bad value. Also, you can skip the Vector.Set line by using the Vector's copy-constructor. Ex. local v = Vector(self)

Lastly, you should be using tabs for codebase consistency, not spaces.

Kefta avatar Oct 10 '17 16:10 Kefta

If modifiers returned *this the constructor will become:

function meta:GetRotated(ang)
	return Vector(self):Rotate(ang)
end

dvdvideo1234 avatar Oct 11 '18 08:10 dvdvideo1234

I feel like most of the stuff in here is useless and there are several problems with other stuff:

  1. Do we really need stuff like ToTable, ToAngle (this one specially confusing because the name might make someone think its transforming a normalized/direction vector into an angle, which it does not), GetSub/Mul/Add/Div? GetZero? Middle?
  2. You still have 2 Vector.Bisect methods
  3. Your first vector.Bisect sets global variables.
  4. Shouldn't GetNegate be GetNegated? Normalize and GetNormlized
  5. GetMiddle never uses its argument, and probably should be GetCenter or something
  6. GetResize also never uses its argument, and should be GetResized

robotboy655 avatar Jun 04 '19 19:06 robotboy655

  1. The idea is to change the data structure, but if you insist, I will remove them ;)
    • Get(Sub / Mul / Add / Div / Zero / Middle) Must return copy of added, subbed, dived ... vector
  2. Thanks, fixed.
  3. Thanks, fixed also
  4. Fixed ;)
  5. The Middle must use its argument for obtaining a midpoint.
  6. Thanks, also fixed ;)

dvdvideo1234 avatar Jul 11 '19 12:07 dvdvideo1234

SetEx should be added in the engine, not Lua. I requested it here https://github.com/Facepunch/garrysmod-requests/issues/515

Kefta avatar Sep 17 '19 09:09 Kefta

I think these should be implemented in the engine for speed.

Kefta avatar Oct 04 '19 12:10 Kefta

You should utilise vector:Unpack where you index x, y, and z to be more efficient. Also, using number indexing is slightly faster than string indexing (vec[1] vs vec.x).

Kefta avatar Oct 23 '19 12:10 Kefta

You should utilise vector:Unpack

Last time I tested at home Unpack was not pushed to production. I will try today as well.

Also, using number indexing is slightly faster than string indexing (vec[1] vs vec.x)

Yes, this is true. The official wiki says we must use X, Y, Z. It is always better to stick to the documentation. ;)

dvdvideo1234 avatar Oct 24 '19 06:10 dvdvideo1234

That is an oversight of the wiki, I will change it. You should still use numerical indexing.

Kefta avatar Oct 24 '19 09:10 Kefta

Updated the wiki for the Vector and Angle pages on all possible ways to index them.

Kefta avatar Oct 24 '19 10:10 Kefta

Looking good. Last thing to clean up is the style (spaces around parentheses and brackets, C-styled logical operators) to match the codebase, and to change the name of the PR as this is no longer one method.

Kefta avatar Oct 25 '19 15:10 Kefta

I think this is finally in a good place. There's some additional optimisations that can be done to save on Vector objects in Get* alternatives instead of just calling the original functions with a copy, but those can be done post-merge. Thanks for staying patient with all the corrections.

Kefta avatar Oct 28 '19 08:10 Kefta

Thank you too!

dvdvideo1234 avatar Oct 28 '19 08:10 dvdvideo1234

Thoughts @thegrb93 , @fruitwasp is this any good ?

dvdvideo1234 avatar Oct 29 '19 15:10 dvdvideo1234

The ToAngle needs to be removed as it will be confused with Vector:Angle

thegrb93 avatar Oct 29 '19 15:10 thegrb93

All of the <operation>Unpacked and Get<Operation>Unpacked are unnecessary bloat. 90% of this is unnecessary bloat.

thegrb93 avatar Oct 29 '19 15:10 thegrb93

~~AngleBetween can be much more efficient, I can provide code for that.~~ Nvm I was reading it wrong

thegrb93 avatar Oct 29 '19 15:10 thegrb93

I think <operation>Unpacked should be added in the engine - it would be more efficient than creating a vector object simply to perform an operation on all three digits, and even more efficient than setting each component manually. They could even be made into optional args to be able to do two component operations in one function call instead of four (two __newindex, two __index). I'll add them to my request on https://github.com/Facepunch/garrysmod-requests/issues/1442.

Now looking at it, ToAngle should be removed not for the confusion with Angle, but rather the uselessness of the operation: I can think of no cases where you would go from a Vector to Angle unless it were to convert an impulse force, but that would require performing operations on the components pre-conversion anyway.

Kefta avatar Oct 29 '19 18:10 Kefta

The APIs with Unpacked will work very nice for me, as I will remove like 100+ lines of code from my addon just for using these. Sometimes a copy of the vector of the executed operation is needed, and for this Get<Operation>Unpacked is used.

dvdvideo1234 avatar Oct 29 '19 19:10 dvdvideo1234

@Kefta @thegrb93,

Is it better for data conversion to just run this. It relies on the object factory to create an instance from any type. I see the conversion from a string is not needed. But I wonder, what is the difference between:

  1. Vector(1,2,3) This is the regular call
  2. Vector({1,2,3}) Table-array with 1,2,3
  3. Vector({x=1,y=2,z=3}) Table-hash with X,Y,Z
  4. Vector("1 2 3") This according to the wiki parses the string to a vector. Notice the space.
  5. Vector("1,2,3") What will happen when I give it another delimiter. Notice the comma. :P
  6. Vector(true,true,true) This I think should produce Vector(1,1,1) dunno..

I cannot think of any suitable usage converting:

  1. vector from function,
  2. vector from userdata
  3. vector from a thread.

dvdvideo1234 avatar Nov 21 '19 15:11 dvdvideo1234

2 doesn't work but can be achieved with Vector(unpack({1, 2, 3})). 3, 5, and 6 don't work. You shouldn't adjust your functions at all for that behaviour - let the Vector constructor handle all type coercions. This PR is fine in its current state.

Kefta avatar Nov 21 '19 16:11 Kefta

@Kefta Thanks!

Requested in https://github.com/Facepunch/garrysmod/pull/1619

dvdvideo1234 avatar Nov 29 '19 09:11 dvdvideo1234

@Kefta I see that there is a new wiki. does this mean that the PR #1419 is merged already?

dvdvideo1234 avatar Feb 17 '20 13:02 dvdvideo1234

@ Kefta I see that there is a new wiki. does this mean that the PR #1419 is merged already?

No, I have added some methods that were requested here (Your yourself rated the last comment on that issue too)

robotboy655 avatar Feb 17 '20 16:02 robotboy655

What is the point of having GetAdd, GetSub, GetMul and GetDiv when we already have __add, __sub, __mul and __div? The special metamethods do the same things and must be more efficient.

CornerPin avatar Jan 20 '21 14:01 CornerPin

@CornerPin

These are here for language richness. I can admit, they do the same stuff like the other ones, but I've never measured it. The richest one language is, the better as you can so the same thing by utilizing different lines of code. They are here for language richness mostly, for the users who prefer local c = a:GetAdd(b) more than local c = a + b.

dvdvideo1234 avatar Jan 22 '21 15:01 dvdvideo1234