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

GDCLASS macro prohibits use of pure virtual functions #1287

Closed
Naros opened this issue Oct 26, 2023 · 4 comments · Fixed by #1359
Closed

GDCLASS macro prohibits use of pure virtual functions #1287

Naros opened this issue Oct 26, 2023 · 4 comments · Fixed by #1359
Labels
bug This has been identified as a bug confirmed

Comments

@Naros
Copy link
Contributor

Naros commented Oct 26, 2023

Godot version

4.2 beta3

godot-cpp version

4.2 beta3

System information

Windows 11

Issue description

When trying to create a GDExtension abstract class, the compiler fails to allow pure virtual functions, reporting:

error C2259: 'ParentClass': cannot instantiate abstract class

This is because the GDCLASS macro explicitly makes two references to where it attempts to create the class object, specifically in these two static methods:

static GDExtensionObjectPtr create(void *data) {
	m_class *new_object = memnew(m_class);
	return new_object->_owner;
}

static GDExtensionClassInstancePtr recreate(void *data, GDExtensionObjectPtr obj) {
	_GDCLASS_RECREATE(m_class, m_inherits);
}

The ability to use GDCLASS in GDNative and construct abstract classes exists. I would suggest that GDCLASS function like it does in GDNative and these two specific methods be added to either a separate macro or we introduce a new GDCLASS_ABSTRACT macro that can be used specifically for abstract class construction with all but the above 2 functions defined.

Steps to reproduce

Create a class hierarchy as follows

class ParentClass : public Resource {
  GDCLASS(ParentClass, Resource);
  static void _bind_methods() {}
  virtual void pure_virtual_method() = 0;
}

class ChildClass : public ParentClass {
  GDCLASS(ChildClass, ParentClass);
  static void _bind_methods() { }
  void pure_virtual_method() override { }
}

Minimal reproduction project

N/A

@AThousandShips AThousandShips added bug This has been identified as a bug confirmed labels Oct 26, 2023
@dsnopek
Copy link
Collaborator

dsnopek commented Oct 26, 2023

Thanks!

This is a tricky one.

Since Godot modules can register abstract classes, we want to allow godot-cpp to do so as well. However, we also want to try to mimic the module API as much as possible, and in a module, you can use GDCLASS() for both normal and abstract classes. So, ideally, we'd like the same GDCLASS() macro to be used for both in godot-cpp as well (as opposed to adding a GDCLASS_ABSTRACT() macro).

Maybe there's a way to move the generation of the create() and recreate() functions to templates that are only realized when calling ClassDB::register_class<T>()?

Something like this super incomplete psuedo code:

template<class T>
class ClassCreator {
	static GDExtensionObjectPtr create(void *data) {
		T *new_object = memnew(m_class);
		return new_object->_owner;
	}
};

template <class T>
void ClassDB::_register_class() {
	GDExtensionClassCreationInfo2 class_info = {
		// ...
		&ClassCreator<T>::create,
		// ...
	};
}	

And then when registering abstract classes, we wouldn't use that, we'd just use nullptr for the creation function.

Hopefully that idea makes sense :-)

@Naros
Copy link
Contributor Author

Naros commented Oct 27, 2023

@dsnopek So I see that _GDCLASS_RECREATE takes the parent class type; however, it isn't used in the macro. Are you okay with simply having the macro accept the m_class argument only?

@Zylann
Copy link
Collaborator

Zylann commented Oct 28, 2023

Is there some sort of if constexpr we could use to check if the class has pure virtual functions?
Otherwise I wonder why create would have to be in the class, if it can be moved out that would be better I think

@AThousandShips
Copy link
Member

There is std::is_abstract

dsnopek added a commit that referenced this issue Jan 19, 2024
Rework GDCLASS macro to allow abstract classes
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 confirmed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants