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

Rework GDCLASS macro to allow abstract classes #1359

Merged
merged 1 commit into from
Jan 19, 2024

Conversation

Naros
Copy link
Contributor

@Naros Naros commented Jan 12, 2024

Fixes #1287

This PR moves the create and recreate methods out of the GDCLASS macro and adjusts the references in the ClassDB to use the new ClassCreator template class that provides the same functionality as the previous methods in the macro.

Users can now freely create Godot classes with pure virtual functions, i.e. this is now possible:

class MyTestClass : public VBoxContainer
{
    GDCLASS(MyTestClass, VBoxContainer);
    static void _bind_methods() {}
public:
    virtual void test_function() = 0;
};

@Naros Naros requested a review from a team as a code owner January 12, 2024 20:27
@Naros Naros force-pushed the GH-1287 branch 2 times, most recently from 68aad1e to 16be4f8 Compare January 12, 2024 21:28
@AThousandShips
Copy link
Member

I'd suggest adding a pure virtual class to the test project to verify the functionality

@AThousandShips AThousandShips added bug This has been identified as a bug topic:buildsystem Related to the buildsystem or CI setup labels Jan 13, 2024
test/src/example.h Outdated Show resolved Hide resolved
include/godot_cpp/classes/wrapped.hpp Outdated Show resolved Hide resolved
include/godot_cpp/classes/wrapped.hpp Outdated Show resolved Hide resolved
test/src/register_types.cpp Outdated Show resolved Hide resolved
@AThousandShips
Copy link
Member

My bad making an improved suggestion

@AThousandShips
Copy link
Member

AThousandShips commented Jan 14, 2024

This fixes the problem, and uses the existing examples, applied on top of your PR:

diff --git a/include/godot_cpp/classes/wrapped.hpp b/include/godot_cpp/classes/wrapped.hpp
index 43dc365..4e071aa 100644
--- a/include/godot_cpp/classes/wrapped.hpp
+++ b/include/godot_cpp/classes/wrapped.hpp
@@ -152,12 +152,20 @@ template <class T>
 class ClassCreator {
 public:
        static GDExtensionObjectPtr create(void *data) {
-               T *new_object = memnew(T);
-               return new_object->_owner;
-       };
+               if constexpr (!std::is_abstract_v<T>) {
+                       T *new_object = memnew(T);
+                       return new_object->_owner;
+               } else {
+                       return nullptr;
+               }
+       }

        static GDExtensionClassInstancePtr recreate(void *data, GDExtensionObjectPtr obj) {
-               _GDCLASS_RECREATE(T)
+               if constexpr (!std::is_abstract_v<T>) {
+                       _GDCLASS_RECREATE(T);
+               } else {
+                       return nullptr;
+               }
        }
 };

diff --git a/test/src/example.h b/test/src/example.h
index 32d66d5..99e0d5c 100644
--- a/test/src/example.h
+++ b/test/src/example.h
@@ -196,31 +196,17 @@ class ExampleVirtual : public Object {

 protected:
        static void _bind_methods() {}
+
+       virtual int test_function() { return 25; }
 };

 class ExampleAbstract : public Object {
        GDCLASS(ExampleAbstract, Object);

-protected:
-       static void _bind_methods() {}
-};
-
-class ExamplePureVirtualBase : public Object {
-       GDCLASS(ExamplePureVirtualBase, Object);
-
 protected:
        static void _bind_methods() {}

        virtual int test_function() = 0;
 };

-class ExamplePureVirtual : public ExamplePureVirtualBase {
-       GDCLASS(ExamplePureVirtual, ExamplePureVirtualBase);
-
-protected:
-       static void _bind_methods() {}
-
-       int test_function() override { return 25; }
-};
-
 #endif // EXAMPLE_CLASS_H
diff --git a/test/src/register_types.cpp b/test/src/register_types.cpp
index 5b99d38..dbb37d9 100644
--- a/test/src/register_types.cpp
+++ b/test/src/register_types.cpp
@@ -26,9 +26,6 @@ void initialize_example_module(ModuleInitializationLevel p_level) {
        ClassDB::register_class<Example>();
        ClassDB::register_class<ExampleVirtual>(true);
        ClassDB::register_abstract_class<ExampleAbstract>();
-
-       GDREGISTER_VIRTUAL_CLASS(ExamplePureVirtualBase);
-       GDREGISTER_CLASS(ExamplePureVirtual);
 }

 void uninitialize_example_module(ModuleInitializationLevel p_level) {

@Naros
Copy link
Contributor Author

Naros commented Jan 15, 2024

Will look at this after work today.

@Naros Naros force-pushed the GH-1287 branch 2 times, most recently from cbb1e72 to ab3db91 Compare January 15, 2024 23:41
@Naros
Copy link
Contributor Author

Naros commented Jan 15, 2024

Hi @AThousandShips, I decided to actually keep the example classes as I think they provide context specifically for this fix. Your suggestion implements a non-pure virtual function on a class, which worked just fine with the GDCLASS macro without this fix. The problem I'm trying to solve here is pure virtual functions, and your suggested test case doesn't explicitly showcase that aspect. I hope that's okay, would just rather be thorough and explicit so we avoid breaking this in the future.

@AThousandShips
Copy link
Member

AThousandShips commented Jan 16, 2024

Not sure how you tested it but the examples I provided failed to work both with your fix and the unchanged library

Also you probably didn't read my suggestion properly, it does add a pure virtual function to the abstract class:

class ExampleAbstract : public Object {
       GDCLASS(ExampleAbstract, Object);

protected:
       static void _bind_methods() {}

       virtual int test_function() = 0;
};

test/src/example.h Outdated Show resolved Hide resolved
test/src/example.h Outdated Show resolved Hide resolved
test/src/register_types.cpp Outdated Show resolved Hide resolved
@AThousandShips AThousandShips changed the title Rework GDCLASS macro to allow pure virtual functions Rework GDCLASS macro to allow abstract classes Jan 16, 2024
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

This looks exactly like the approach I was imagining for solving this problem.

I only have a couple nitpicks in addition to AThousandShips' comments.

include/godot_cpp/classes/wrapped.hpp Outdated Show resolved Hide resolved
include/godot_cpp/classes/wrapped.hpp Outdated Show resolved Hide resolved
@Naros Naros force-pushed the GH-1287 branch 2 times, most recently from aa01df9 to 097f698 Compare January 17, 2024 22:49
Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks! This is looking really great! :-)

I have just some super tiny nitpicks, mostly around naming.

include/godot_cpp/core/class_db.hpp Outdated Show resolved Hide resolved
include/godot_cpp/core/class_db.hpp Outdated Show resolved Hide resolved
include/godot_cpp/core/class_db.hpp Outdated Show resolved Hide resolved
include/godot_cpp/core/class_db.hpp Outdated Show resolved Hide resolved
@Naros
Copy link
Contributor Author

Naros commented Jan 18, 2024

I only have a couple nitpicks in addition to AThousandShips' comments.

Thanks @dsnopek, added a commit with the suggested changes.

@dsnopek
Copy link
Collaborator

dsnopek commented Jan 19, 2024

Thanks! The changes look good.

However, you'll need to squash your PR into a single commit before it can be merged - see:

https://docs.godotengine.org/en/latest/contributing/workflow/pr_workflow.html#the-interactive-rebase

Copy link
Collaborator

@dsnopek dsnopek left a comment

Choose a reason for hiding this comment

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

Thanks!

@dsnopek dsnopek merged commit 6c04514 into godotengine:master Jan 19, 2024
12 checks passed
@dsnopek
Copy link
Collaborator

dsnopek commented Jan 22, 2024

Cherry-picking to 4.2 in #1372

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug This has been identified as a bug topic:buildsystem Related to the buildsystem or CI setup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GDCLASS macro prohibits use of pure virtual functions
3 participants