garrysmod
garrysmod copied to clipboard
Update the vector Lua extention to include various handy methods.
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.
If modifiers returned *this
the constructor will become:
function meta:GetRotated(ang)
return Vector(self):Rotate(ang)
end
I feel like most of the stuff in here is useless and there are several problems with other stuff:
- 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
? - You still have 2 Vector.Bisect methods
- Your first vector.Bisect sets global variables.
- Shouldn't
GetNegate
beGetNegated
?Normalize
andGetNormlized
-
GetMiddle
never uses its argument, and probably should beGetCenter
or something -
GetResize
also never uses its argument, and should beGetResized
- 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
-
- Thanks, fixed.
- Thanks, fixed also
- Fixed ;)
- The
Middle
must use its argument for obtaining a midpoint. - Thanks, also fixed ;)
SetEx should be added in the engine, not Lua. I requested it here https://github.com/Facepunch/garrysmod-requests/issues/515
I think these should be implemented in the engine for speed.
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).
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. ;)
That is an oversight of the wiki, I will change it. You should still use numerical indexing.
Updated the wiki for the Vector and Angle pages on all possible ways to index them.
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.
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.
Thank you too!
Thoughts @thegrb93 , @fruitwasp is this any good ?
The ToAngle needs to be removed as it will be confused with Vector:Angle
All of the <operation>Unpacked
and Get<Operation>Unpacked
are unnecessary bloat. 90% of this is unnecessary bloat.
~~AngleBetween can be much more efficient, I can provide code for that.~~ Nvm I was reading it wrong
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.
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.
@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:
-
Vector(1,2,3)
This is the regular call -
Vector({1,2,3})
Table-array with 1,2,3 -
Vector({x=1,y=2,z=3})
Table-hash with X,Y,Z -
Vector("1 2 3")
This according to the wiki parses the string to a vector. Notice the space. -
Vector("1,2,3")
What will happen when I give it another delimiter. Notice the comma. :P -
Vector(true,true,true)
This I think should produceVector(1,1,1)
dunno..
I cannot think of any suitable usage converting:
-
vector
fromfunction
, -
vector
fromuserdata
-
vector
from athread
.
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 Thanks!
Requested in https://github.com/Facepunch/garrysmod/pull/1619
@Kefta I see that there is a new wiki. does this mean that the PR #1419 is merged already?
@ 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)
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
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
.