Open Vlad-Zumer opened 5 months ago
These should be separate proposals as they are very different topics
For the second part of the proposal, it's not really accurate to say the Packed*Array
classes are missing any methods, they're not and never were designed to be:
Array
Their purpose is primarily to serve as convenient data transfer units between the engine internals and scripting, to avoid costly conversions every time the engine core needs to do something with data passed from scripting, or provide the same data to scripting. On the c++ side Array
is of limited usefulness when it comes to handle data in most cases, but that's the interop type with scripting. That's why the packed array types exist, they exist to provide a useful interface so that we can get the best of both worlds:
Vector<T>
), without doing costly conversions every time we want to have a bound methodThe Packed*Array
types are designed not primarily for heavy use in scripting, but instead to serve as a cost effective way to transfer data in a way that works for both c++ and scripting
Tl;dr;
If you want general purpose functionality, use Array
, if you want an interop type with low cost of transfer to the engine internals, use Packed*Array
These should be separate proposals as they are very different topics
If this refers to "⚪ Issue 1: Typed arrays and type coercion" that was only there to who how I ended up hitting the PackedStringArray
limitation. That problem already has an issue assigned to it marked as a bug.
For the second part of the proposal, it's not really accurate to say the
Packed*Array
classes are missing any methods, they're not and never were designed to be (...)
I understand the design behind the Packed Arrays
, but I don't understand if this section is supposed to be pushing back on this proposal.
PS: Does the formatting of the proposal text make it seem like there are 2 issues and 2 proposals?
There are two very different topics here, with separate implementation processes, type coercion is separate from adding functions, so should be a separate proposal to discuss and implement, the two implementations would almost certainly be two different PRs due to it being completely different parts of the engine
One is the way the methods are bound, they would likely be bound in core/variant/variant_call.cpp
, the coercion would be part of the GDScript module instead, and is a bug, not a proposal
There's also an additional issue with this proposal that I forgot about when first responding: C#
Adding these methods would further increase the lack of parity with C# as Packed*Array
doesn't exist in C#, see here, this is already a source of confusion there and this would make that (and writing tutorials etc.) even worse
It would also (depending on how it's implemented) make working with Modules and GDExtension more difficult, as Packed*Array
is different in Modules and GDExtension due to the former just being a Vector<T>
and the latter being a dedicated interop type, and we aim to make it possible to write code that works both in a module and as GDExtension with minimal modification or #ifdef
etc.
Vector<T>
for example already has sort_custom
but it doesn't take a Callable
but a custom sort class, this can't be exposed to scripting, so it would cause issues with handling that parity for example
Further most of these methods (with the exception of the pop
methods, back
, front
, and pick_random
methos) couldn't be implemented on Vector<T>
itself, because you couldn't have it take a Callable
because it isn't guaranteed to be able to pass the argument. So it'd have to be implemented externally. So it would be messy and complicated. This is why Vector
doesn't have any Callable
based methods in it
@AThousandShips thank you for helping to explain the underlying rationale so clearly!
I am primarily a web developer and recently I've been learning Godot for hobby gamedev purposes (from Unity previously). I think the Web platform's TypedArray presents an interesting comparison here. There are some important limitations with types like Uint8Array, Float32Array, etc in order to preserve their performance benefits (e.g. no "push" or "pop" methods in order to avoid unnecessarily reallocating the underlying buffer). These typed arrays are even more important now that tech like WebAssembly is supported, making it easier and more efficient to share data across contexts.
So with that in mind:
System.Array
of specific types; this means that C# authors using Godot can leverage convenience methods from System.Linq.Enumerable
(e.g. Where()
in place of filter()
, Select()
in place of map()
, Aggregate()
in place of reduce()
)all
, any
, filter
, map
, reduce
back
, bsearch_custom
, erase
, front
, max
, min
, pick_random
, pop_at
, pop_back
, pop_front
, push_back
, push_front
, shuffle
, sort_custom
map
, where the return type should match the strict-typed array, it would likely be preferable to fill in 0
(or equivalent initial value) for cases where the provided callback does not return the expected type instead of throwing errorsfilter
, script authors may see positive performance benefits from built-in method implementations. Without the convenience method, the simplest alternative implementation would be to iterate backwards over the array and call remove_at
for each item that doesn't pass the filter condition. However, this is most likely less efficient than resizing the result array once at the end of the operation. See example usage below.# this loop
for idx in range(file_names.size()-1, 0, -1):
if file_names[idx].ends_with(".import"):
file_names.remove_at(idx)
# could be simpler and more efficient as a one-liner
file_names.filter(func(s): return !s.ends_with(".import"))
# this loop is arguably better, but more verbose
var filtered_file_names = PackedStringArray()
var accepted = 0
filtered_file_names.resize(file_names.size())
for idx in file_names.size():
if !file_names[idx].ends_with(".import"):
filtered_file_names[accepted] = file_names[idx]
accepted += 1
filtered_file_names.resize(accepted)
I recognize that the inclusion/exclusion of certain methods from packed arrays is a matter of opinion and priorities, and I'm not claiming to represent anything other than my personal opinions here.
I'm still quite new to Godot and my C++ is rusty, but I'm happy to help contribute as best I can. I appreciate the discussion!
There are two very different topics here, with separate implementation processes, type coercion is separate from adding functions, so should be a separate proposal to discuss and implement, the two implementations would almost certainly be two different PRs due to it being completely different parts of the engine
Ok, so I was misunderstood here. This proposal is only for adding missing methods on the Packed Arrays
. The type coercion bit was just meant to explain how I ended up needing these methods on the PackedStringArray
. I will reformat the proposal to alleviate this. Thank you!
One is the way the methods are bound, they would likely be bound in
core/variant/variant_call.cpp
, the coercion would be part of the GDScript module instead, and is a bug, not a proposal
This has already been accepted as a bug in godot@#91048, and godot@#92215 or so it seems, so I call it bug.
Also there seems to already be a more in-depth proposal for "fixing" typed arrays in GDScript
, see #7364.
It's true that they don't exist but, with the exception of modifying the array, you can get the same functionality between Packed*Array
in GDScript
and System.Array
in C#
(in the example below int[]
).
I can see how this may make the implementation of this proposal harder, nevertheless I'm positive this isn't the only place where these 2 differ.
Can you expand on this, please? Why couldn't a Callable
take the argument? Also what argument specifically? Are you considering the return value of the Callable
?
The way I see it right now, the Implementation difficulty : 1
can be implemented directly on the Vector<T>
, and the rest (because they need access to Callable
and Variant
) in an extensions file that is used to create the bindings for GDScript
. I'm not sure how much there is a need for these methods in Modules
and GDExtensions
, as, AFAIK you are using C++ to write both and such would have access to inner Vector<T>
and the std::vector<T>
, for which (the latter) the stl
already has these mostly implemented (C++20
).
you can get the same functionality between
Yes, that's what I said, you can but you have to change your code, that's exactly the issue I'm talking about
I can see how this may make the implementation of this proposal harder, nevertheless I'm positive this isn't the only place where these 2 differ.
So let's not make it harder shall we?
Why couldn't a Callable take the argument
Because you can't convert everything to a Variant
, what if you make a Vector<Pair<Object *, Ref<Resource>>>
? How would you encode that to a Variant
? How about Vector<Vector<Object *>>
?
Again this is just part of why this change would involve a lot of fragile glue code that would:
Packed*Array
aren't available on Vector
The reason this is relevant is because Packed*Array
isn't the same as Vector<T>
in GDExtension as I already mentioned, so if you're writing code that can be used both as a module and as an extension you're unable to handle that difference,
Yes, that's what I said, you can but you have to change your code, that's exactly the issue I'm talking about
You don't really have to change the code because, like you said, Packed Arrays
don't exist in C#
. I don't think that having to use the language features that are not a 1-to-1 mapping is problematic, or a good example of lack of parity between GDScript
and C#
.
1) Why not require that the inputs and outputs of the Callables
, even in C++
be Variants
. What I am proposing is that the Callables
from the Packed Arrays
methods would take in Variants
and output Variants
as well.
2) There are already methods on normal arrays that take in Callables
which need to have Variant
parameters and return either another Variant
or a specific type (see examples).
Func Name | Return | Callable Signature |
---|---|---|
Array.all |
bool |
(Variant) -> bool |
Array.map |
Array |
(Variant) -> Variant |
Array.sort_custom |
void |
(Variant, Variant) -> bool |
Array.reduce |
Variant |
(Variant, Variant) -> Variant |
I tested some of these with Callables
that do not respect the required signature and some amount of type checking is already being done (when using typed Callables
). Why can't this be done for Packed Arrays
as well?
1) Could this be done/exposed only for GDScript
? My understanding is that GDExtensions
and Modules
are already quite advanced topics, so small inconsistencies between the 2 shouldn't have much of an impact.
2) I understand the performance concerns issue, but this proposal won't make the existing performance metrics any worse, as it's a purely additive change to the API.
You don't really have to change the code because, like you said, Packed Arrays don't exist in C#
I'll try to be more clear: You have to change your code when you translate code from GDScript to C#, that's what I mean
Why not require that the inputs and outputs of the Callables, even in C++ be Variants
We do, that's exactly what I'm saying, please try read again what I say, the problem is that they require Variant
, but you can't make everything into a Variant
. And Array
is completely different because it always has Variant
as a value type, exactly unlike Vector
in general, that's why it can't be a method on Vector<T>
but has to be a bound method elsewhere, which I explained why that's a bad idea...
The point is that the Packed*Array
types aren't designed for what you propose, and they don't need this functionality, and adding it would be costly and messy, so it'd have to be for a very good reason to justify making all that work, and as I've pointed out there's a lot of reasons that would be costly and cause confusion and mess
You are completely ignoring everything I explained, please go back over what I said and try pay more attention to the details
Have a nice day
I am still confused about this variant thing. There's definitely something that I don't fully understnad. Can you possibly provide some code to illustrate the problem?
The way I see it it would work like this:
class Variant;
class Array : public Variant;
class Callable : public Variant
{
// variadic operator
Variant operator()(Variant...);
}
// constrain type T further
template<class T>
class PackedArray<T>
{
// runtime typechecking like the Array.Filter
PackedArray<T> filter (Callable method) const;
void sort_custom(Callable func);
Array<Variant> map(Callable method) const;
}
I'll give a final explanation repeating myself from above:
There's no PackedArray<T>
type, it's a Vector<T>
internally so it can take any type (it being a Vector
internally is the entire reason they exist, as an interop type), not just the specific types, this might be the root of your confusion that you missed that in my multiple explanations, it's not it's own type (outside of GDExtension, which as I've explained causes its own problems)
So if we add this method to Vector<T>
it will break if the T
type can't be a Variant
which is often the case, for example something as simple as Vector<Vector<Node *>>
or similar
Only some types can be a Variant
and that isn't something that's restricted on Vector
This means that the implementation of filter
has to be made separately, which means:
Vector
and Packed*Array
and they are not the same classes (unlike c++)So to summarize a bit and clarify the points I've tried to make here:
Packed*Array
s are not designed to be general purpose containers for scripting, their purpose is for communicating with the engine internals, especially servers. This is done because using an Array
(even a typed Array
) is very slow when used for specific purposes in c++. If you want to deal with a lot of data in c++ you want the underlying Vector
, not Array
. So to avoid having to copy data and convert data every time someone calls some method like updating the vertices of some mesh or something we use Packed*Array
over Array
, or we'd have to iterate over it converting it to the Vector
type we want. It also allows binding methods that take these types directly instead of making a custom method taking an Array
.Packed*Array
in scripting is for very large data, or efficient data storage, as a Packed*Array
is much better handled when stored in a file for example, at least compared to Array
where each entry has to be encoded with the type etc.So unless you're either 1) communicating with the engine internals or 2) dealing with large data or 3) want to store data efficiently you probably don't need to use Packed*Array
, you can still use it if you wish (it absolutely has some benefits, there's nothing wrong with using them outside of that), but the point is that that falls outside the main purpose and what they are designed for.
So if you want a general purpose container you should probably just use Array
. This will be further helped by improvements to how type coercion and casting works with typed arrays going forward, it's being hotly debated and worked on as we speak but it's a pretty complicated question so might take some time to fully figure out.
So while there are some benefits to using Packed*Array
s over Array
even outside of the intended purpose it is still not intended to be a general purpose container, but a type to aid in doing some core engine things more (far more) efficiently.
That's not to say that conceptually the idea of improving these types in this way is wrong, absolutely not, but the problem comes down to:
So to me, and what I've tried to express here, this isn't a "this idea is bad in itself" but "this idea is something that goes against what has been intended for this feature, and it comes with a great cost that makes it hard to justify"
I hope that makes things clearer, and thank you for this discussion it's been very informative :)
Thank you for your time, and thanks for explaining it again. I was under the impression that Packed Arrays
existed both on C++
and GDScript
as proper types. If I understand you correctly Packed(<T>)Array
in GDScript
is just a Vector<T>
on the C++
side, meaning only the naming has changed, but not the underlying object type. (Let me know if I'm misunderstanding yet again.)
So unless you're either 1) communicating with the engine internals or 2) dealing with large data or 3) want to store data efficiently you probably don't need to use Packed*Array, you can still use it if you wish (it absolutely has some benefits, there's nothing wrong with using them outside of that), but the point is that that falls outside the main purpose and what they are designed for.
So to me, and what I've tried to express here, this isn't a "this idea is bad in itself" but "this idea is something that goes against what has been intended for this feature, and it comes with a great cost that makes it hard to justify"
I understand that Packed Arrays
are not "general purpose", what I'm saying is that their functionality could be improved. To be 100% honest, all this started because I wanted the erase
method on it, which can be implemented as is, without doing anything with Variants
and Callables
(find
+ remove_at
).
Finally considering my new understanding of the problem, I think by using SFINAE
and conditional compilation of the missing methods can be implemented just for Variant
types. I think something like this (same code below) would work pretty well.
It enables the other methods only for Variant
types. I think this also solves the difference between GDExtensions
and Modules
since the user would get a compilation error when using the Variant
only methods on vectors that do not hold Variant
values. (PS. This is only an example and can surely be refined to better suit the application.)
meaning only the naming has changed, but not the underlying object type
Yes it's just a wrapper around Vector<T>
, it probably got lost in my longer comments above when I said it before :)
I think by using SFINAE and conditional compilation of the missing methods can be implemented just for Variant types.
That'd be very fragile as there's not necessarily any way to directly tell if a type is compatible, it'd also create dependencies that'd be messy (I'd say it might not even be directly possible to even add Callable
to Vector
as Callable
might on Vector
, at least as it needs to be able to cast to Variant
which does depend on Vector
, so you probably wouldn't be able to call a Callable
in the vector.h
header, and you'd have to because of templates)
So regardless it'd still be something that has to be put in separate methods
erase
could easily just be bound, and would make far more sense than most of the other cases IMO without being relevant as a generalization (it's already implemented on Vector
I'm actually preparing adding erase
to Packed*Array
, it makes sense and should be simple, will see what the tests result in
Done:
erase
is also a prime example of a method that doesn't have the properties listed above:
For the remainder I'd make the following classifications:
Must be implemented separately (with all the consequences above):
map
filter
sort_custom
any
all
bsearch_custom
reduce
Requires decisions on how to implement them, as already discussed with the relevant PR implementing it for other cases, these would very likely have to crash when the index is unavailable, as it cannot signal an invalid result, as there's no way to signal that with any data type, you could still store things like Variant()
or nullptr
:
pop_front
pop_back
pop_at
Same as above but with the added mess that these would have to return a const T &
or T &
depending, which is even more impossible than the cases above:
front
back
Would not have to be implemented separately but should because of introducing major dependencies into all cases using Vector
otherwise which would be undesirable, we really don't want to have to include the random generator in everything that uses Vector
(which is essentially the entire engine):
shuffle
pick_random
Further the pop
methods, front
, back
, and pick_random
can easily be worked around with essentially no peformance loss when implemented in GDScript, all
, any
, filter
, and reduce
can be worked around easily but with some performance reduction as you'd have to do the iteration and processing in GDScript which would be slower, bsearch_custom
can be worked around but isn't very enjoyable, sort_custom
is very difficult to work around
The back
and pop_back
methods would be made much easier if Vector
supported negative indices, but that's something that might be unnecessarily costly to add as the index operator is reasonable performant as is and doing that sort of checking might harm that
As you can see several of these issues arise from that Vector
is a header only file because it's a template, you cannot implement the code itself outside of the header, so it makes you unable to do a lot of the tricks you can otherwise to avoid introducing heavy dependencies, as is done with Array
which handles the randomness internally
Describe the project you are working on
ID3 metadata tag editor.
Describe the problem or limitation you are having in your project
Due to godot@#91048 the code samples below does not work.
extends Node
func _ready(): var callableOK: Callable = TestOK.bind(["one", "two", "three"]) callableOK.call()
func TestProblem(arr: Array[String]) -> void: print("TestProblem arr: %s" % arr)
func TestOK(arr: PackedStringArray) -> void: print("TestOK arr: %s" % arr)
In trying to solve the first issue (and for performance) I changed from
Array[String]
toPackedStringArray
, but this container is missing some methods as explained in godot@#92562. The code below shows the limitations I encountered while trying to use thePackedStringArray
.⚪ Related Proposals
1213
7556
7364
Describe the feature / enhancement and how it helps to overcome the problem or limitation
Implementing the missing methods.
Describe how your proposal will work, with code, pseudo-code, mock-ups, and/or diagrams
🟤
GDScript
example🟤
C++
method types forPackedArray<T>
If this enhancement will not be used often, can it be worked around with a few lines of script?
All the missing methods are implementable as free functions by the user, but is not rare for them to be used.
Is there a reason why this should be core and not an add-on in the asset library?
This should be part of the core as these are core types used in many places inside godot.