godotengine / godot-cpp

C++ bindings for the Godot script API
MIT License
1.57k stars 473 forks source link

TypedArray<T> operator [] returning Variant makes little sence. #1482

Open RonYanDaik opened 1 month ago

RonYanDaik commented 1 month ago

Godot version

4.2

godot-cpp version

4.2

System information

Win 10

Issue description

When using TypedArray I think the one would expect that accessing entries with operator [] will return you variable of type T. Instead it returns Variant, which is loosing all point to use TypedArray in first place.

This should give compilation error:

TypedArray<Node> a; 
Dictionaty d = a[0];

As well as this construction is a bit annoying:

TypedArray<Node> a; 
Node* n = (Node*) (Object*) a[0]; //beacuse varinat can only  be cast to Object*

Steps to reproduce

-

Minimal reproduction project

-

AThousandShips commented 1 month ago

This is a limitation and is the same as the engine, this should be tracked there as it has to be changed there first (and would change here as a follow up)

But part of the problem is that it would have to replace the existing operator, that wouldn't be trivial as it inherits Array

kromenak commented 2 weeks ago

I also noticed this, and I agree that it would be really great if operator[] was typed.

This seems fairly easy to do by modifying typed_array.hpp file directly on my end:

#define MAKE_TYPED_ARRAY(m_type, m_variant_type)                                                                 \
    template <>                                                                                                  \
    class TypedArray<m_type> : public Array {                                                                    \
    public:                                                                                                      \
        _FORCE_INLINE_ void operator=(const Array &p_array) {                                                    \
            ERR_FAIL_COND_MSG(!is_same_typed(p_array), "Cannot assign an array with a different element type."); \
            _ref(p_array);                                                                                       \
        }                                                                                                        \
        _FORCE_INLINE_ TypedArray(const Variant &p_variant) :                                                    \
                Array(p_variant.operator Array(), m_variant_type, StringName(), Variant()) {                     \
        }                                                                                                        \
        _FORCE_INLINE_ TypedArray(const Array &p_array) :                                                        \
                Array(p_array, m_variant_type, StringName(), Variant()) {                                        \
        }                                                                                                        \
        _FORCE_INLINE_ TypedArray() {                                                                            \
            set_typed(m_variant_type, StringName(), Variant());                                                  \
        }                                                                                                        \
                 _FORCE_INLINE_ m_type operator[](int index) {                                                            \
                        return static_cast<m_type>(Array::operator[](index));                                                \
                 }                                                                                                        \
                 _FORCE_INLINE_ m_type operator[](int index) const {                                                      \
                        return static_cast<m_type>(Array::operator[](index));                                                \
                 }                                                                                                        \
    };

This compiles and runs fine on my end, but I guess I do not know enough about the engine to understand if this causes any unintended side effects. However, if not, I would advocate this solution!

AThousandShips commented 2 weeks ago

That is not necessarily safe for non simple types, for example objects, or more importantly refs, you'd have problems with returning a Font instead of a Ref<Font> as the proper way to declare a typed array with Font is TypedArray<Font>, not TypedArray<Ref<Font>>

So this is a bit more complicated, and there's issues involved in overloading operators etc. that might cause issues

But you'd still have various other methods on Array that would need to be typed, like front etc., and this should probably be resolved in GDScript first to make sure typing etc. is handled uniformly