godotengine / godot-proposals

Godot Improvement Proposals (GIPs)
MIT License
1.13k stars 69 forks source link

Add PackedVector2iArray and PackedVector3iArray #5486

Open alazifk opened 2 years ago

alazifk commented 2 years ago

Describe the project you are working on

I would like to create a GDScript-function in C++ that takes an array of Vector2i.

Describe the problem or limitation you are having in your project

E.g. void foo(const Vector<Vector2i>& points); will throw an error.

In type_array.h only

MAKE_TYPED_ARRAY_INFO(Vector<Vector2>, Variant::PACKED_VECTOR2_ARRAY)
MAKE_TYPED_ARRAY_INFO(Vector<Vector3>, Variant::PACKED_VECTOR3_ARRAY)

are defined, but there is nothing analogous for Vector2i and Vector3i. I know very little C++ so I might be mistaken but it seems this simply has not been implemented yet.

Describe the feature / enhancement and how it helps to overcome the problem or limitation

It would allow to have GDScript-functions take an array of Vector2i/Vector3i.

Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams

void foo(const Vector<Vector2i>& points); void foo(const Vector<Vector3i>& points);

If this enhancement will not be used often, can it be worked around with a few lines of script?

You could use Vector with floats, but that is not as nice.

Is there a reason why this should be core and not an add-on in the asset library?

It has to be.

Calinou commented 2 years ago

Adding new Variant type has a performance cost, so it should be carefully considered. It needs to be used often enough within the engine to be worth doing.

You could use Vector with floats, but that is not as nice.

An alternative is to use a PackedInt32Array (or PackedInt64Array) with a defined stride in the method's documentation (2 for Vector2i, 3 for Vector3i, 4 for Vector4i).

alazifk commented 2 years ago

My specific use case was to to pass directions. But since there are not that many bools or flags will do. For lists of points I prefer PackedIntArrays anyways, because they have less overhead. So I do not consider the change worth it if any performance costs are involved.

MisterMX commented 2 years ago

Can this be reopened because there are actual cases in core where these types are needed (see https://github.com/godotengine/godot/issues/66541)?

@Calinou can you pin-point to the place that causes performance implications? Because I don't see anything.

Calinou commented 2 years ago

can you pin-point to the place that causes performance implications? Because I don't see anything.

Variant is full of switch statements that iterate over all possible types, so adding more types makes those switch statements slower by definition. See also https://github.com/godotengine/godot-proposals/issues/629 (which ended up being adopted as there were enough use cases in core for it to make sense). However, proposals like https://github.com/godotengine/godot-proposals/issues/5397 are unlikely to be accepted in comparison because there are few concrete use cases in core.

See also https://github.com/godotengine/godot/pull/36436 which is where Vector2i and Vector3i were implemented.

Mickeon commented 2 years ago

Where would PackedVector2iArray and PackedVector3iArray be used in, other than in the case made for the proposal? It feels very situational, I'm not too sure about this.

Chaosus commented 2 years ago

If introducing new types is not desired, maybe it makes sense to use an Array class instead (just need to add a note to the documentation to make clear that it will be filled with Vector2i variables).

Mickeon commented 2 years ago

I think it would also make sense to natively support typed arrays with core methods. There's some that return Arrays of Dictionaries that as of now merely describe they do in the documentation, so in these cases an Array of Vector2i would also make sense, I suppose, especially if access speed is no concern.

kleonc commented 1 year ago

I think it would also make sense to natively support typed arrays with core methods.

It's already supported, check out e.g. https://github.com/godotengine/godot/pull/63959 or the latest docs.

Mickeon commented 1 year ago

Alright then, I do see this proposal as being very unnecessary for now, yeah...

MisterMX commented 1 year ago

I didn't know about TypedArray yet. Thanks @Chaosus for the hint.

I opened https://github.com/godotengine/godot/pull/66655 that would remove the necessity of this extension.

acgc99 commented 1 year ago

I recently requested this feature in the discussion #5713 , but @Calinou notified me that this is already tracked here. Since someone may think this proposal is unnecessary, I expose my case where it would be useful.

I have an 2D hexagonal grid (it is a 3D game, but the grid is 2D), that means that the hexagons positions are Vector2 (Vector3 in fact, but one component is the same for all). As the page of Red Blob Games suggests, using other system of coordinates than the cartesian one is better for algorithms. So I implemented the axial coordinates, and now I have a set of Vector2is. This also prevents floating point errors with the cartesian coordinates when performing operations with the positions. Now I have functions that return an Array[Vector2i], but it would be nicer to have a PackedVector2iArray.

So they might also be useful for other coordinate systems rather than cartesian.

MajorMcDoom commented 5 months ago

I recently requested this feature in the discussion #5713 , but @Calinou notified me that this is already tracked here. Since someone may think this proposal is unnecessary, I expose my case where it would be useful.

I have an 2D hexagonal grid (it is a 3D game, but the grid is 2D), that means that the hexagons positions are Vector2 (Vector3 in fact, but one component is the same for all). As the page of Red Blob Games suggests, using other system of coordinates than the cartesian one is better for algorithms. So I implemented the axial coordinates, and now I have a set of Vector2is. This also prevents floating point errors with the cartesian coordinates when performing operations with the positions. Now I have functions that return an Array[Vector2i], but it would be nicer to have a PackedVector2iArray.

So they might also be useful for other coordinate systems rather than cartesian.

I also have very similar cases:

It is quite common for me, but I totally understand if the underlying performance implications outweigh that. It's nothing I can't work around.