Discern between VIRTUAL and ABSTRACT class bindings #58972

reduz · 2022-03-10T07:35:24Z

Previous "virtual" classes (which can't be instantiated) are now correctly named "abstract" (GDREGISTER_ABSTRACT_CLASS).
Added a new "virtual" category for classes, they can't be instantiated from the editor, but can be inherited from script and extensions (GDREGISTER_VIRTUAL_CLASS).
Converted a large amount of classes from "abstract" to "virtual" where it makes sense, and added the relevant GDVIRTUAL bindings so they can be extended via extension.

Most classes (that make sense) have been converted. Missing:

Physics servers
VideoStream
Script* classes.

which will go in separate PRs due to the complexity involved.

touilleMan · 2022-03-10T08:22:08Z

Wow this is super cool ! 😃

Script* classes.

Does it means the plan is to expose ScriptServer/Script/ScriptInstance to ClassDB and hence to allow them to be used from GDExtension ? (this is what point 1 of godotengine/godot-proposals#3927 was about, so I'm super happy if this is happening ^^)

reduz · 2022-03-10T08:38:00Z

@touilleMan I think ScriptInstance will be exposed via a native extension API (Plain C), since its more efficient if it does not use the binder, but Script and ScriptLanguage will be exposed as regular ClassDB classes. Likely as ScriptExtension and ScriptLanguageExtension.

akien-mga

Looks pretty good to me, some comments and nitpicks.

akien-mga · 2022-03-10T08:28:36Z

core/io/resource.cpp

+	MethodInfo get_rid_bind("_get_rid");
+	get_rid_bind.return_val.type = Variant::RID;
+
+	::ClassDB::add_virtual_method(get_class_static(), get_rid_bind, true, Vector<String>(), true);


Just curious, does it actually need the :: scope here? Just above there's ClassDB::bind_method without extra scoping.

akien-mga · 2022-03-10T08:32:29Z

core/object/class_db.cpp

+		return false;
+	}
+#endif
+	return (!ti->disabled && ti->creation_func != nullptr && !(ti->native_extension && !ti->native_extension->create_instance) && ti->is_virtual);


I see you adapted this from can_instantiate() so this is basically can_instantiate() && is_virtual.

Just wondering if all checks from can_instantiate() are indeed wanted - e.g. do we want to exclude classes without creation func or disabled from being qualified virtual? Just a theoretical question.

Good point, I am not sure. For the time being I only made this changes to affect the editor, but not more. We can improve it towards the future I suppose.

akien-mga · 2022-03-10T08:34:25Z

core/register_core_types.cpp

+	GDREGISTER_ABSTRACT_CLASS(StreamPeer);
 	GDREGISTER_CLASS(StreamPeerExtension);


StreamPeer, PacketPeer and MultiplayerPeer all have an *Extension wrapper which AFAIU was specifically added to workaround the limitation that this PR lifts. So these could likely be registered as virtual instead, and the *Extension wrappers dropped.

But that's probably stuff for a future PR so it might be good to keep it as is in this one. CC @Faless

Extension makes sense if its something very large, so I guess it is up to Fabio to change.

akien-mga · 2022-03-10T08:34:47Z

core/register_core_types.cpp

 	GDREGISTER_CLASS(AStar);
 	GDREGISTER_CLASS(AStar2D);
 	GDREGISTER_CLASS(EncodedObjectAsID);
 	GDREGISTER_CLASS(RandomNumberGenerator);

-	GDREGISTER_VIRTUAL_CLASS(ResourceImporter);
+	GDREGISTER_ABSTRACT_CLASS(ResourceImporter);


Isn't that something that might typically make sense to implement as virtual?

Yes, I completely forgot about these, I will add them.

Looking at them, this will require likely another PR because its a lot of code.

akien-mga · 2022-03-10T08:38:04Z

doc/classes/Texture2D.xml

+		<method name="_draw" qualifiers="virtual const">
+			<return type="void" />


Just a note in passing but as we recently moved the docs for constructors and operators to dedicated <constructors> and <operators> sections, it might make sense to do the same (in a follow-up) with a <virtuals> or something like this.

So we can make the docs a bit clearer with a separation between the API users can use, and the one users can extend/reimplement.

scene/resources/mesh.cpp

scene/resources/texture.cpp

akien-mga · 2022-03-10T08:55:11Z

scene/resources/texture.h

-	virtual int get_depth() const = 0;
-	virtual bool has_mipmaps() const = 0;
-	virtual Vector<Ref<Image>> get_data() const = 0;
+	TypedArray<Image> _get_datai() const;


What does i stand for here?

internal I suppose, its not user exposed and _get_data is virtual already.

servers/audio/audio_stream.cpp

akien-mga · 2022-03-10T08:58:19Z

servers/register_server_types.cpp

+	GDREGISTER_ABSTRACT_CLASS(TextServer);
 	GDREGISTER_CLASS(TextServerExtension);


Another candidate for merging TextServerExtension into a virtual TextServer I guess @bruvzg.

This make sense, but last time I have tried to do so, every GDVIRTUAL binding which is using enums for the TextServer stopped working, seems like registering enums and binding virtual methods should have a certain order. So it might need some extra changes.

It also sounds like @reduz suggests keeping *Extension classes for classes where a lot of the API is virtual, so this might be the case here.

I think for the large ones (like script, physics server, etc. Using Extension makes more sense because otherwise the code gets to be a mess.

akien-mga · 2022-03-10T09:05:27Z

Looking at the kind of GDVIRTUAL_CALL + ERR_FAIL_MSG construct which happens a lot for virtual methods which must be implemented, I was thinking that it could make sense to add a new GDVIRTUAL_CALL_REQUIRED that would handle this automatically with an explicit error message.

So this:

Ref<Material> Mesh::surface_get_material(int p_idx) const {
	Ref<Material> ret;
	if (GDVIRTUAL_CALL(_surface_get_material, p_idx, ret)) {
		return ret;
	}

	ERR_FAIL_V_MSG(Ref<Material>(), "Unimplemented function");
}

Would become this:

Ref<Material> Mesh::surface_get_material(int p_idx) const {
	Ref<Material> ret;
	if (GDVIRTUAL_CALL_REQUIRED(_surface_get_material, p_idx, ret)) {
		return ret;
	}
	return Ref<Material>();
}

And GDVIRTUAL_CALL_REQUIRED would use ##m_name## and get_class() to compose a nice error message when it's not implemented, referring to the name of the virtual methods that must be implemented.
So you'd get an error like this:

Missing implementation for required virtual method '_surface_get_material' in Mesh.

akien-mga · 2022-03-10T09:20:15Z

For the record, godot-cpp fails building after this PR with:

g++ -o src/register_types.os -c -std=c++17 -fPIC -Wwrite-strings -Og -g -m64 -fPIC -DDEBUG_ENABLED -DDEBUG_METHODS_ENABLED -I. -Igodot-headers -Iinclude -Igen/include -I/home/runner/work/godot/godot/godot-cpp/gen/include -I/home/runner/work/godot/godot/godot-cpp/include -I/home/runner/work/godot/godot/godot-cpp/godot-headers -Isrc src/register_types.cpp
In file included from src/register_types.cpp:35:
/home/runner/work/godot/godot/godot-cpp/include/godot_cpp/core/class_db.hpp: In instantiation of ‘static void godot::ClassDB::register_class() [with T = ExampleRef]’:
src/register_types.cpp:44:38:   required from here
/home/runner/work/godot/godot/godot-cpp/include/godot_cpp/core/class_db.hpp:162:14: error: invalid conversion from ‘void*’ to ‘GDNativeExtensionClassGetRID’ {aka ‘long unsigned int (*)(void*)’} [-fpermissive]
  162 |   (void *)cl.name, // void *class_userdata;
      |           ~~~^~~~
      |              |
      |              void*
/home/runner/work/godot/godot/godot-cpp/include/godot_cpp/core/class_db.hpp: In instantiation of ‘static void godot::ClassDB::register_class() [with T = Example]’:
src/register_types.cpp:45:35:   required from here
/home/runner/work/godot/godot/godot-cpp/include/godot_cpp/core/class_db.hpp:162:14: error: invalid conversion from ‘void*’ to ‘GDNativeExtensionClassGetRID’ {aka ‘long unsigned int (*)(void*)’} [-fpermissive]

We need to disable the godot-cpp test in this PR temporarily until this is merged and a matching counterpart is merged in godot-cpp.

akien-mga · 2022-03-10T09:22:38Z

For the record, here's the list of classes currently exposed as virtual as of this PR (it's not visible in the diff due to the name swap with virtual -> abstract so those didn't get a diff):

$ rg GDREGISTER_VIRTUAL_CLASS
core/object/class_db.h
442:#define GDREGISTER_VIRTUAL_CLASS(m_class)            \

servers/register_server_types.cpp
142:    GDREGISTER_VIRTUAL_CLASS(AudioStreamPlaybackResampled);
145:    GDREGISTER_VIRTUAL_CLASS(AudioEffect);
146:    GDREGISTER_VIRTUAL_CLASS(AudioEffectInstance);

scene/register_scene_types.cpp
320:    GDREGISTER_VIRTUAL_CLASS(BaseButton);
341:    GDREGISTER_VIRTUAL_CLASS(Range);
462:    GDREGISTER_VIRTUAL_CLASS(VisualInstance3D);
463:    GDREGISTER_VIRTUAL_CLASS(GeometryInstance3D);
748:    GDREGISTER_VIRTUAL_CLASS(Mesh);
756:    GDREGISTER_VIRTUAL_CLASS(PrimitiveMesh);
767:    GDREGISTER_VIRTUAL_CLASS(Material);
808:    GDREGISTER_VIRTUAL_CLASS(Texture);
809:    GDREGISTER_VIRTUAL_CLASS(Texture2D);
822:    GDREGISTER_VIRTUAL_CLASS(TextureLayered);
824:    GDREGISTER_VIRTUAL_CLASS(Texture3D);
845:    GDREGISTER_VIRTUAL_CLASS(StyleBox);

.github/workflows/linux_builds.yml

* Previous "virtual" classes (which can't be instantiated) are not corretly named "abstract". * Added a new "virtual" category for classes, they can't be instantiated from the editor, but can be inherited from script and extensions. * Converted a large amount of classes from "abstract" to "virtual" where it makes sense. Most classes that make sense have been converted. Missing: * Physics servers * VideoStream * Script* classes. which will go in a separate PR due to the complexity involved.

akien-mga · 2022-03-10T13:09:23Z

Thanks!

reduz requested review from a team as code owners March 10, 2022 07:35

akien-mga added enhancement topic:core labels Mar 10, 2022

akien-mga added this to the 4.0 milestone Mar 10, 2022

akien-mga reviewed Mar 10, 2022

View reviewed changes

reduz force-pushed the expose-more-gdextension branch from 038ea7c to f3b3c1e Compare March 10, 2022 10:17

reduz requested a review from a team as a code owner March 10, 2022 10:17

akien-mga reviewed Mar 10, 2022

View reviewed changes

.github/workflows/linux_builds.yml Outdated Show resolved Hide resolved

reduz force-pushed the expose-more-gdextension branch from f3b3c1e to 6f51eca Compare March 10, 2022 11:28

akien-mga approved these changes Mar 10, 2022

View reviewed changes

akien-mga merged commit 9b59736 into godotengine:master Mar 10, 2022

bruvzg mentioned this pull request Mar 14, 2022

Fix build after "Discern between VIRTUAL and ABSTRACT class bindings". godotengine/godot-cpp#724

Merged

Calinou mentioned this pull request Mar 27, 2022

It's possible to create abstract resource classes using the inspector (e.g. Mesh or PrimitiveMesh in MeshInstance3D) #59605

Open

lufog mentioned this pull request May 25, 2022

Sync with Godot master GDRETools/gdsdecomp#53

Merged

8 tasks

aaronfranke mentioned this pull request Oct 19, 2022

Don't allow instancing virtual node types in the Create New Node dialog #67614

Merged

aaronfranke mentioned this pull request Nov 6, 2022

Fix default values not showing up on virtual classes #68344

Merged

adamscott mentioned this pull request Dec 11, 2022

Using non-global class for plugin custom type throws an error #69889

Closed

YuriSizov mentioned this pull request Jan 19, 2023

Clean-up, harmonize, and improve StyleBox API #71686

Merged

akien-mga mentioned this pull request Feb 26, 2023

BaseButton and other virtual Node and Resource types can't be instantiated even when derived in custom types #35484

Closed

lilizoey mentioned this pull request May 1, 2023

Class without init cannot be returned from Rust to GDScript godot-rust/gdext#194

Closed

kleonc mentioned this pull request Oct 14, 2023

Allow inherit from BaseButton when creating new script to create custom buttons godotengine/godot-proposals#8101

Open

Okxa mentioned this pull request Jun 12, 2024

Script class extending from abstract class is not shown in Create New Node dialog. #93080

Closed

Tsunjigiri mentioned this pull request Jan 23, 2025

Type 'Noise' does not provide any constructors in C# #96396

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Discern between VIRTUAL and ABSTRACT class bindings #58972

Discern between VIRTUAL and ABSTRACT class bindings #58972

reduz commented Mar 10, 2022 •

edited

Loading

touilleMan commented Mar 10, 2022

reduz commented Mar 10, 2022 •

edited

Loading

akien-mga left a comment

akien-mga Mar 10, 2022

akien-mga Mar 10, 2022

reduz Mar 10, 2022

akien-mga Mar 10, 2022

reduz Mar 10, 2022

akien-mga Mar 10, 2022

reduz Mar 10, 2022

reduz Mar 10, 2022

akien-mga Mar 10, 2022

akien-mga Mar 10, 2022

reduz Mar 10, 2022

akien-mga Mar 10, 2022

bruvzg Mar 10, 2022

akien-mga Mar 10, 2022

reduz Mar 10, 2022

akien-mga commented Mar 10, 2022

akien-mga commented Mar 10, 2022

akien-mga commented Mar 10, 2022

akien-mga commented Mar 10, 2022

		GDREGISTER_ABSTRACT_CLASS(StreamPeer);
		GDREGISTER_CLASS(StreamPeerExtension);

		<method name="_draw" qualifiers="virtual const">
		<return type="void" />

		GDREGISTER_ABSTRACT_CLASS(TextServer);
		GDREGISTER_CLASS(TextServerExtension);

Discern between VIRTUAL and ABSTRACT class bindings #58972

Discern between VIRTUAL and ABSTRACT class bindings #58972

Conversation

reduz commented Mar 10, 2022 • edited Loading

touilleMan commented Mar 10, 2022

reduz commented Mar 10, 2022 • edited Loading

akien-mga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akien-mga commented Mar 10, 2022

akien-mga commented Mar 10, 2022

akien-mga commented Mar 10, 2022

akien-mga commented Mar 10, 2022

reduz commented Mar 10, 2022 •

edited

Loading

reduz commented Mar 10, 2022 •

edited

Loading