Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
RonYanDaik opened this issue Jun 6, 2024 · 3 comments
Open

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

RonYanDaik opened this issue Jun 6, 2024 · 3 comments
Labels
discussion enhancement This is an enhancement on the current functionality

Comments

@RonYanDaik
Copy link

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
Copy link
Member

AThousandShips commented Jun 6, 2024

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
Copy link

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
Copy link
Member

AThousandShips commented Jun 22, 2024

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

@AThousandShips AThousandShips added enhancement This is an enhancement on the current functionality discussion labels Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion enhancement This is an enhancement on the current functionality
Projects
None yet
Development

No branches or pull requests

3 participants