godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
90.35k stars 21.06k forks source link

[TRACKER] Refactor math code and add integer vector types #18324

Closed aaronfranke closed 3 years ago

aaronfranke commented 6 years ago

I plan to make separate PRs for each item on the list. Each change (mostly) requires the one before it, but I believe that each change is justified on its own without taking into account future ones (following the from one working state to another working state philosophy).

I won't work on future items until current PRs are merged (since they need past items).

The below discussion may not reflect the above points, as I've edited this post over time.

groud commented 6 years ago

Please wait for a proper PR review of #18096 before doing all this work. I personally believe most of those changes are not desired.

PJB3005 commented 6 years ago

Position2 might be a bit of a handful to write, so Pos2 might be a good idea too.

aaronfranke commented 6 years ago

I had considered Pos2 and I figured it could be ambiguous as well, but then again, what else could Pos mean except for Position? We may end up switching it to Pos2 instead, if it's desired.

Edit: Seems to not be desired.

akien-mga commented 6 years ago

Position2 might be a bit of a handful to write, so Pos2 might be a good idea too.

Given that we changed all the 2.x API from *pos* to *position*, I don't think that would be consistent :)

groud commented 6 years ago

I don't get why you think a "Point" induces an integer positioning. The mathematical definition of a point is basically a position in space. You say "a point on the grid" but "a position on the grid" has exactly the same meaning. I think Point2 and Point2i are understandable and explicit enough.

Regarding other changes, they need reduz's input. I suspect there are reasons for those classes not being exposed to GDscript/c#.

aaronfranke commented 6 years ago

I mean, I could make "Position2i" a typedef for integer points, but here's my thought process:

groud commented 6 years ago

To understand clearly, this is how things are done now:

struct Vector2 {
//...
}
typedef Vector2 Size2;
typedef Vector2 Point2;

struct Point2i {
//...
}
typedef Point2i Size2i;

for me, making things consistent would just be:

struct Vector2 {
//...
}
typedef Vector2 Size2;
typedef Vector2 Point2;

struct Vector2i {
//...
}
typedef Vector2i Size2i;
typedef Vector2i Point2i;

So for me:

Do you have any other suggestions aside from having i at the end? I think it's inelegant.

That's where I disagree. I have no problem with that. There are no mathematical term to define a point/vector/position/size that is defined on an integer space, thus adding a "i" in the end makes more sense than any other solution replacing one of the current Point2/Position2/Vector2 by a integer version. As any of those term can be understood as float or integer, doing so would definitely lead to more confusion.

aaronfranke commented 6 years ago

I'm not saying that a position isn't a vector. I'm saying that integer-math "vectors" lose many of the properties that mathematical vectors have, such as normalization (only possible when aligned to axis) or rotation (only possible with 90-degree multiples). So "Vector2i" could be misleading.

The distinctions are that "position" is more specific than "vector" or "point" which can be ambiguous, and "vector" is never used to describe grid points but "point" is, so "point" for integers seems appropriate if we want to have unique names for each type (I do, you don't).

My proposal would be:

struct Vector2 { 
//... 
} 
typedef Vector2 Size2; 
typedef Vector2 Position2; 

struct Point2 { 
//... 
} 
// typedefs optional, I personally don't want them, "Size2i" defeats purpose of unique names
groud commented 6 years ago

Yeah obviously, you can't do the same thing in the real numbers space than in the integer one.

But this is not the question here, the question is how should we name those objects. For me, using "Point2" (or "Position2") for integers and "Vector2" for floats is even more misleading than having "Vector2" for float and "Vector2i" for integers. As no substantial distinction can be made between point/vector/position/size, the "i" at the end seems a correct solution to distinguish between what is integer-based or not. We could name it something like IntegerVector2, IntVector2 or even Vector2Int, but since it's a commonly used type I think the "i" is probably enough.

aaronfranke commented 6 years ago

Even if we call integer "vectors" Vector2i. I do still think that "Position2" is a better typedef name than "Point2". Positions are called "position" in most other places in the code.

romlok commented 6 years ago

Since it'd not be dealing with a continuous coordinate space, might Discrete2 be an option for the name? Even though "discrete" does not necessarily mean only integers in mathematical terms, Godot primarily targets game developers, not mathematicians. :)


As to the existing use of Point2 and Size2, neither of them are exposed to the GDScript side of things, and developers seem to do fine with positions and sizes being stored as mere Vector2s.

Personally, I don't see a need for Point2 and Size2 to exist as aliases of Vector2 if annotating variables is their only purpose. IMO, If a variable name isn't descriptive enough then the variable name should be made more descriptive. And FWIW, all the variables I've seen where these are used seem to be descriptive enough already!

BBric commented 6 years ago

Pixel or Cell or PairInt for Point2i ?

aaronfranke commented 6 years ago

@romlok @BBric Good suggestions. I like the word Cell, it fits well with the use cases described here https://github.com/godotengine/godot/issues/3286 of using these integer point classes with dictionaries/maps (where floats are unsuitable). If we decided to change Point2i to Cell2 then I would be happy with that change. The big thing is that I like unique names - I dislike "Vector2i" because it looks just like "Vector2".

But I don't like the names Pixel (sounds like something to do with images) or PairInt.

Side note, some other projects use Point to mean integer points, such as MonoGame's Point.cs.

aaronfranke commented 6 years ago

Assuming we go with "Cell" as the integer point/vector/whatever name, how about this?

struct Vector2 { 
//... 
} 
typedef Vector2 Size2; 
typedef Vector2 Point2;  // Or, Position2

struct Cell2 { 
//... 
} 
typedef Cell2 Size2i; 
typedef Cell2 Point2i;  // Or, Position2i
BBric commented 6 years ago

'Position' is good too, it remains geometrical, but maybe humanly slightly more specific, thus better suited for integer coordinates...

aaronfranke commented 6 years ago

It sounds like we're going with "Vector2i" based on Reduz's response here.

Anutrix commented 5 years ago

What's the status now?

aaronfranke commented 5 years ago

@Anutrix https://github.com/godotengine/godot/pull/22205 will be merged after 3.2 is released, and I plan to finish this work by 4.0.