Change Ref<T> to allow non const access to ptr

[saki7]

This is the godot-cpp counterpart for godotengine/godot#64789
Fixes #693

(pinging @akien-mga since he requested this)

[derammo]

Hi @saki7 I was just coming here to ask who is going to do the counterpart PR and you already finished it :)

For sanity checking, I diffed the implementations of Ref<> from core and extension for your review of any other changes that may have crept int. These seem fine by me.

diff core.h ext.h
32d31
<     // virtual RefCounted * get_reference() const { return reference; }
89a89
> 
98c98
<         Object *object = p_variant.get_validated_object();
---
>         // Needs testing, Variant has a cast to Object * here.
99a100,102
>         // Object *object = p_variant.get_validated_object();
>         Object *object = p_variant;
> 
148a152
> 
165c169
<         Object *object = p_variant.get_validated_object();
---
>         // Needs testing, Variant has a cast to Object * here.
166a171,173
>         // Object *object = p_variant.get_validated_object();
>         Object *object = p_variant;
> 
184,187d190
<         // TODO: this should be moved to mutexes, since this engine does not really
<         // do a lot of referencing on references and stuff
<         // mutexes will avoid more crashes?
< 
197c200
<         ref(memnew(T));
---
>         ref(memnew(T()));
206c209,218
< };
\ No newline at end of file
---
> 
>     // Used exclusively in the bindings to recreate the Ref Godot encapsulates in return values,
>     // without adding to the refcount.
>     inline static Ref<T> ___internal_constructor(Object *obj)
>     {
>         Ref<T> r;
>         r.reference = (T *)obj;
>         return r;
>     }
> };

[akien-mga]

Seems to match the upstream changes which were reviewed by GDExtension contributors.

[dsnopek]

akien already approved, but an extra approval can't hurt :-)

[Gallilus]

I was able to test compiling godot and godot-cpp using this change.
The project compiled OK and at runtime, I got the expected errors as explained by #1119

So I think it is fine to merge.

[Gallilus]

Reminder: THIS IS NOT A BLOCKER and may be ignored
I want to document my runtime error more completely here.

I created a new ResourceFormatSaver that is implementing virtual PackedStringArray _get_recognized_extensions(const Ref<Resource> &resource) const;

In the call stack, I see it go true this function.
https://github.com/godotengine/godot-cpp/blob/48635729b990b6a84e2e2bb7334b2b4ca2c28ee8/include/godot_cpp/classes/ref.hpp#LL259C1-L259C1
resulting In an Access violation reading location 0xFFFFFFFFFFFFFFFF. I hope this image below gives useful information.

Reminder: THIS IS NOT A BLOCKER and may be ignored

[dsnopek]

@Gallilus:

In the call stack, I see it go true this function.
https://github.com/godotengine/godot-cpp/blob/48635729b990b6a84e2e2bb7334b2b4ca2c28ee8/include/godot_cpp/classes/ref.hpp#LL259C1-L259C1
resulting in an Access violation reading location 0xFFFFFFFFFFFFFFFF. I hope this image below gives useful information.

Yep, this looks exactly like the regression from PR 1045 as detailed in issue #1119.

I've seen it manifest like this (as an "access violation" or "segfault" on locking the mutex) or as infinitely hanging on locking the mutex.

[saki7]

@dsnopek Could you press that big green button™? 😉

[saki7]

Thanks!