garrysmod
garrysmod copied to clipboard
Facepunch
Add rounding to Angle and Vector
- Add Angle:GetRound( decimals )
- Add Angle:GetFloor()
- Add Angle:GetCeil()
- Add Vector:GetRound( decimals )
- Add Vector:GetFloor()
- Add Vector:GetCeil()
- Add Vector:GetRotated( rotation ), a brother of Vector:GetNormalized()
These function names sound like they modify the existing object, but instead, they create a new one. And another thing, you should be using the named fields on the objects instead of 1 2 3.
x y z for vectors.
p y r for angles.
GetRotated has already been done in https://github.com/Facepunch/garrysmod/pull/1419
@sannysc I was thinking if I should make them modify the original or return a new object. I agree that such naming is inconsistent with vector/angle normalization/rotation functions naming, though there are not so many functions to talk about consistency. I work with vectors a lot and almost in all cases it's more handy to return a new object. First of all - this way you can do chains like
----------------------------------------------------------------
local gridPos = ent:GetPos():Round()
-- vs --
local gridPos = Vector( ent:GetPos() )
gridPos:Round()
----------------------------------------------------------------
----------------------------------------------------------------
----------------------------------------------------------------
render.DrawSprite( ent:GetPos():Floor(), ... )
-- vs --
local spritePos = ent:GetPos()
spritePos:Floor()
render.DrawSprite( spritePos, ... )
----------------------------------------------------------------
etc.
Of course you can do "return self" in functions which modify the original object, but that leads you to a habit to use such chains with these functions, which in many cases can lead to unintended side affects (modification of objects you were not trying to). In case of GetPos it wouldn't have side affects since GetPos returns a new object which you can modify without being afraid to move the original entity, but take the following example into consideration
local points = { Vector( 1.1, 2.2, 3.3 ), ... }
for i, point in ipairs( points ) do DrawPoint( point:Ceil(), ... )
In this case, ceiling would change contents of the original points table, but it's not intentional. To avoid this, you'd need to explicitly create a new vector, copying the original one, and only then rounding it or whatever. It clutters the code and in my opinion is just unnecessary.
In my opinion, changing the original value and returning it should be a solution in an environment where you have getters returning a copy of object's field for you to work with, but while in GLua we usually work with tables, where using metatables (for creating getters and stuff) is an overkill for most simple tasks, such approach is not applicable.
GetNormalized appeared once not "just because", but because it is handy to use. I want these new functions to be also handy, I don't think creating 6 more functions (like GetRounded, etc.) is necessary for any reason other than following an arguable convention (yeah, maybe micro-optimization in some cases, but it's vice versa for other cases)
About vectors indexing I don't see a reason why I should avoid using numerical indexes for vectors while Garry's Mod deliberately allows it. For angles it's even more fun, you can use 1,2,3/p,y,r/x,y,z. It's handy in some cases, for example
for i = 1, 3 do
if pos[i] > max[i] then max[i] = pos[i] end
if pos[i] < min[i] then min[i] = pos[i] end
end
I have chosen numerical indexes here because they should be a tiny bit faster than string literals (I hope?)
@Kefta My bad, I didn't make sure GetRotated wasn't already proposed. In any case, it's not merged, so I don't think that's much of a deal
Sorry for the long post, but I wanted to explain myself clearly (I doubt I succeeded though lol). If you have arguments against - I can change my mind
I was thinking if I should make them modify the original or return a new object. I agree that such naming is inconsistent with vector/angle normalization/rotation functions naming, though there are not so many functions to talk about consistency. I work with vectors a lot and almost in all cases it's more handy to return a new object.
The behavior doesn't need to be changed. The name is what doesn't make any sense because it doesn't match the behavior. It is much easier to just change the name. You yourself agree that it is inconsistent. It doesn't matter how few functions there are to compare it with. If it's inconsistent, it's inconsistent. That doesn't change whether it is inconsistent with 3 functions or 30 functions.
About vectors indexing I don't see a reason why I should avoid using numerical indexes for vectors while Garry's Mod deliberately allows it. For angles it's even more fun, you can use 1,2,3/p,y,r/x,y,z. It's handy in some cases, for example
You should avoid using them because it doesn't make sense to people who are reading the code. What the hell is [2] in the context of a vector/angle? You might know, I might know, but it isn't clear what is going on because it looks like some random number.
How can we even be sure that those numbers won't change in a future update? They aren't even documented. It's not documented for angles either. There is absolutely no reason to use those numbers over the letters. There is no universe in which [2] is easier to understand to a reader than y when dealing with vectors and angles.
They are functionally the same thing, except one of them is easier for readers to understand. You're trying to optimize something you haven't even measured, and of which, I doubt is more optimal in the first place to any degree worth caring about. Are you intentionally making your code more difficult to understand? Because that's exactly what you're doing.
@sannysc Is it alright like this or should I change it to GetRounded, GetFloored, GetCeiled?
In my opinion, yes, but I don't want to be picky. If the core devs think they should be changed to something else, I'm sure they'll ask.
In fact, methods that modify the original OOP instance should always return *this !
Edit: you can track my complex number and 2D ray trace library for how to do that.
