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

Change _can_handle and _edit virtual methods to take Object* #66121

Merged

Conversation

Zylann
Copy link
Contributor

@Zylann Zylann commented Sep 19, 2022

Not sure why they were taking Variant while it's always been for Objects and other related virtual methods take Object* already.

Comment on lines 657 to 665
if (p_object->is_class("Resource")) {
GDVIRTUAL_CALL(_edit, Ref<Resource>(Object::cast_to<Resource>(p_object)));
} else {
GDVIRTUAL_CALL(_edit, p_object);
}
GDVIRTUAL_CALL(_edit, p_object);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes the behavior, no? How does this affect existing plugins and the actual reference counting on that resource?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont think it is supposed to change behavior, it's passed just the same as every other virtual method taking Object* already. If reference count has to increase, it will happen in the callee.

@mhilbrunner
Copy link
Member

cc @reduz you changed this in #51970

@reduz
Copy link
Member

reduz commented Dec 4, 2022

The reason it takes Variant is because, if you are editing a Ref<> type, it properly passes the reference counter. If you pass a pointer, you may miss it until you re assign it to something. Not sure if in practice it makes any difference.

@Zylann
Copy link
Contributor Author

Zylann commented Dec 4, 2022

My understanding is that it really makes no practical difference because in every case if the callee wants to store it, it should be using a Ref<T> or Variant anyways. If the callee stores the pointer without checking the reference case, that's a mistake of the callee. Also, there are other places where the API takes Object* already, and in Godot modules it always takes Object*. So it is at least inconsistent, which is why I made this PR.

Copy link
Member

@RandomShaper RandomShaper left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine. RefCounted object's reference count is respected through the GDVIRTUAL_CALL(). Using Variant was incrementing it to later decrement back on return, which didn't really add any value (maybe in past codebases that had a role?). Moreover, if the call chain required the cast, handles() would already be in trouble. I agree with @Zylann in that the callee is the responsible for keeping the object referenced if it needs to. In any case, doing the cast wasn't really woking as a safety measure, preventing a failure to do so from causing trouble.

@akien-mga
Copy link
Member

akien-mga commented Feb 17, 2023

@Zylann Could you rebase this PR?

Edit: Did it myself to be able to merge this today.

@akien-mga akien-mga force-pushed the plugin_handles_edit_object_argument branch from 066f44f to d2b4e30 Compare February 17, 2023 13:11
@akien-mga akien-mga merged commit 7c7ba88 into godotengine:master Feb 17, 2023
@akien-mga
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants