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

Possible fix for subclassing of Ref<> stuff #20

Closed

Conversation

ellenhp
Copy link

@ellenhp ellenhp commented Sep 12, 2021

I think this might fix an issue mux is having that he mentioned in rocket.chat. Needs testing.

@BastiaanOlij
Copy link
Collaborator

Owh I'll test that out later today, that looks like it could indeed fix my issue :)

@BastiaanOlij
Copy link
Collaborator

Hmmm, close but I'm getting this compile warning now:
godot-cpp\include\godot_cpp\classes\ref.hpp(170) : warning C4717: 'godot::Ref<godot::XRInterface>::Ref<godot::XRInterface><godot::XRInterfaceReference>': recursive on all control paths, function will cause runtime stack overflow

So I think it's still missing something...

@BastiaanOlij
Copy link
Collaborator

Trying out the same code as in Godot:

	template <class T_Other>
	Ref(const Ref<T_Other> &p_from) {
		RefCounted *refb = const_cast<RefCounted *>(static_cast<const RefCounted *>(p_from.ptr()));
		if (!refb) {
			unref();
			return;
		}
		Ref r;
		r.reference = Object::cast_to<T>(refb);
		ref(r);
		r.reference = nullptr;
	}

Results in the compile warning to go away but it still crashes on casting. Looks like something is getting mixed up in wrapping the class....

@vnen
Copy link
Owner

vnen commented Sep 13, 2021

Trying out the same code as in Godot:

I believe both constructors should exist: one for the same type and one for the different types. Not sure why I removed it when getting the code from Godot, probably missed it by accident.

@ellenhp
Copy link
Author

ellenhp commented Sep 14, 2021

Superseded by #21

@ellenhp ellenhp closed this Sep 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants