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.

Sign up for GitHub

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

Discern between VIRTUAL and ABSTRACT class bindings #58972

Merged
merged 1 commit into from
Mar 10, 2022
Merged

Discern between VIRTUAL and ABSTRACT class bindings #58972

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion .github/workflows/linux_builds.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,9 @@ jobs:
tests: true
sconsflags: float=64 use_asan=yes use_ubsan=yes
proj-test: true
godot-cpp-test: true
# Can be turned off for PRs that intentionally break compat with godot-cpp,
# until both the upstream PR and the matching godot-cpp changes are merged.
godot-cpp-test: false
bin: "./bin/godot.linuxbsd.double.tools.64.san"
build-mono: false
# Skip 2GiB artifact speeding up action.
Expand Down
2 changes: 2 additions & 0 deletions core/extension/gdnative_interface.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,7 @@ typedef void *GDExtensionClassInstancePtr;

typedef GDNativeBool (*GDNativeExtensionClassSet)(GDExtensionClassInstancePtr p_instance, const GDNativeStringNamePtr p_name, const GDNativeVariantPtr p_value);
typedef GDNativeBool (*GDNativeExtensionClassGet)(GDExtensionClassInstancePtr p_instance, const GDNativeStringNamePtr p_name, GDNativeVariantPtr r_ret);
typedef uint64_t (*GDNativeExtensionClassGetRID)(GDExtensionClassInstancePtr p_instance);

typedef struct {
uint32_t type;
Expand Down Expand Up @@ -228,6 +229,7 @@ typedef struct {
GDNativeExtensionClassCreateInstance create_instance_func; /* this one is mandatory */
GDNativeExtensionClassFreeInstance free_instance_func; /* this one is mandatory */
GDNativeExtensionClassGetVirtual get_virtual_func;
GDNativeExtensionClassGetRID get_rid_func;
void *class_userdata;
} GDNativeExtensionClassCreationInfo;

Expand Down
1 change: 1 addition & 0 deletions core/extension/native_extension.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ void NativeExtension::_register_extension_class(const GDNativeExtensionClassLibr
extension->native_extension.create_instance = p_extension_funcs->create_instance_func;
extension->native_extension.free_instance = p_extension_funcs->free_instance_func;
extension->native_extension.get_virtual = p_extension_funcs->get_virtual_func;
extension->native_extension.get_rid = p_extension_funcs->get_rid_func;

ClassDB::register_extension_class(&extension->native_extension);
}
Expand Down
20 changes: 20 additions & 0 deletions core/io/resource.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,21 @@ void Resource::_take_over_path(const String &p_path) {
}

RID Resource::get_rid() const {
if (get_script_instance()) {
Callable::CallError ce;
RID ret = get_script_instance()->callp(SNAME("_get_rid"), nullptr, 0, ce);
if (ce.error == Callable::CallError::CALL_OK && ret.is_valid()) {
return ret;
}
}
if (_get_extension() && _get_extension()->get_rid) {
RID ret;
ret.from_uint64(_get_extension()->get_rid(_get_extension_instance()));
if (ret.is_valid()) {
return ret;
}
}

return RID();
}

Expand Down Expand Up @@ -428,6 +443,11 @@ void Resource::_bind_methods() {
ADD_PROPERTY(PropertyInfo(Variant::BOOL, "resource_local_to_scene"), "set_local_to_scene", "is_local_to_scene");
ADD_PROPERTY(PropertyInfo(Variant::STRING, "resource_path", PROPERTY_HINT_NONE, "", PROPERTY_USAGE_EDITOR), "set_path", "get_path");
ADD_PROPERTY(PropertyInfo(Variant::STRING_NAME, "resource_name"), "set_name", "get_name");

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);
Copy link
Member

Choose a reason for hiding this comment

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

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

}

Resource::Resource() :
Expand Down
13 changes: 13 additions & 0 deletions core/object/class_db.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -557,6 +557,19 @@ bool ClassDB::can_instantiate(const StringName &p_class) {
return (!ti->disabled && ti->creation_func != nullptr && !(ti->native_extension && !ti->native_extension->create_instance));
}

bool ClassDB::is_virtual(const StringName &p_class) {
OBJTYPE_RLOCK;

ClassInfo *ti = classes.getptr(p_class);
ERR_FAIL_COND_V_MSG(!ti, false, "Cannot get class '" + String(p_class) + "'.");
#ifdef TOOLS_ENABLED
if (ti->api == API_EDITOR && !Engine::get_singleton()->is_editor_hint()) {
return false;
}
#endif
return (!ti->disabled && ti->creation_func != nullptr && !(ti->native_extension && !ti->native_extension->create_instance) && ti->is_virtual);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

}

void ClassDB::_add_class2(const StringName &p_class, const StringName &p_inherits) {
OBJTYPE_WLOCK;

Expand Down
17 changes: 12 additions & 5 deletions core/object/class_db.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ class ClassDB {
StringName name;
bool disabled = false;
bool exposed = false;
bool is_virtual = false;
Object *(*creation_func)() = nullptr;

ClassInfo() {}
Expand Down Expand Up @@ -156,20 +157,21 @@ class ClassDB {
}

template <class T>
static void register_class() {
static void register_class(bool p_virtual = false) {
GLOBAL_LOCK_FUNCTION;
T::initialize_class();
ClassInfo *t = classes.getptr(T::get_class_static());
ERR_FAIL_COND(!t);
t->creation_func = &creator<T>;
t->exposed = true;
t->is_virtual = p_virtual;
t->class_ptr = T::get_class_ptr_static();
t->api = current_api;
T::register_custom_data_to_otdb();
}

template <class T>
static void register_virtual_class() {
static void register_abstract_class() {
GLOBAL_LOCK_FUNCTION;
T::initialize_class();
ClassInfo *t = classes.getptr(T::get_class_static());
Expand Down Expand Up @@ -210,6 +212,7 @@ class ClassDB {
static bool class_exists(const StringName &p_class);
static bool is_parent_class(const StringName &p_class, const StringName &p_inherits);
static bool can_instantiate(const StringName &p_class);
static bool is_virtual(const StringName &p_class);
static Object *instantiate(const StringName &p_class);
static void set_object_extension_instance(Object *p_object, const StringName &p_class, GDExtensionClassInstancePtr p_instance);

Expand Down Expand Up @@ -436,9 +439,13 @@ _FORCE_INLINE_ Vector<Error> errarray(P... p_args) {
if (!GD_IS_DEFINED(ClassDB_Disable_##m_class)) { \
::ClassDB::register_class<m_class>(); \
}
#define GDREGISTER_VIRTUAL_CLASS(m_class) \
if (!GD_IS_DEFINED(ClassDB_Disable_##m_class)) { \
::ClassDB::register_virtual_class<m_class>(); \
#define GDREGISTER_VIRTUAL_CLASS(m_class) \
if (!GD_IS_DEFINED(ClassDB_Disable_##m_class)) { \
::ClassDB::register_class<m_class>(true); \
}
#define GDREGISTER_ABSTRACT_CLASS(m_class) \
if (!GD_IS_DEFINED(ClassDB_Disable_##m_class)) { \
::ClassDB::register_abstract_class<m_class>(); \
}

#include "core/disabled_classes.gen.h"
Expand Down
5 changes: 5 additions & 0 deletions core/object/make_virtuals.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
StringName _gdvirtual_##m_name##_sn = #m_name;\\
mutable bool _gdvirtual_##m_name##_initialized = false;\\
mutable GDNativeExtensionClassCallVirtual _gdvirtual_##m_name = nullptr;\\
template<bool required>\\
_FORCE_INLINE_ bool _gdvirtual_##m_name##_call($CALLARGS) $CONST { \\
ScriptInstance *script_instance = ((Object*)(this))->get_script_instance();\\
if (script_instance) {\\
Expand All @@ -25,6 +26,10 @@
$CALLPTRRET\\
return true;\\
}\\
\\
if (required) {\\
WARN_PRINT_ONCE("Required virtual method: "+get_class()+"::" + #m_name + " must be overriden before calling.");\\
}\\
\\
return false;\\
}\\
Expand Down
9 changes: 7 additions & 2 deletions core/object/object.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,7 @@ struct ObjectNativeExtension {
GDNativeExtensionClassToString to_string;
GDNativeExtensionClassReference reference;
GDNativeExtensionClassReference unreference;
GDNativeExtensionClassGetRID get_rid;

_FORCE_INLINE_ bool is_class(const String &p_class) const {
const ObjectNativeExtension *e = this;
Expand All @@ -273,8 +274,12 @@ struct ObjectNativeExtension {
GDNativeExtensionClassGetVirtual get_virtual;
};

#define GDVIRTUAL_CALL(m_name, ...) _gdvirtual_##m_name##_call(__VA_ARGS__)
#define GDVIRTUAL_CALL_PTR(m_obj, m_name, ...) m_obj->_gdvirtual_##m_name##_call(__VA_ARGS__)
#define GDVIRTUAL_CALL(m_name, ...) _gdvirtual_##m_name##_call<false>(__VA_ARGS__)
#define GDVIRTUAL_CALL_PTR(m_obj, m_name, ...) m_obj->_gdvirtual_##m_name##_call<false>(__VA_ARGS__)

#define GDVIRTUAL_REQUIRED_CALL(m_name, ...) _gdvirtual_##m_name##_call<true>(__VA_ARGS__)
#define GDVIRTUAL_REQUIRED_CALL_PTR(m_obj, m_name, ...) m_obj->_gdvirtual_##m_name##_call<true>(__VA_ARGS__)

#ifdef DEBUG_METHODS_ENABLED
#define GDVIRTUAL_BIND(m_name, ...) ::ClassDB::add_virtual_method(get_class_static(), _gdvirtual_##m_name##_get_method_info(), true, sarray(__VA_ARGS__));
#else
Expand Down
32 changes: 16 additions & 16 deletions core/register_core_types.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,42 +141,42 @@ void register_core_types() {

GDREGISTER_CLASS(Object);

GDREGISTER_VIRTUAL_CLASS(Script);
GDREGISTER_ABSTRACT_CLASS(Script);

GDREGISTER_CLASS(RefCounted);
GDREGISTER_CLASS(WeakRef);
GDREGISTER_CLASS(Resource);
GDREGISTER_CLASS(Image);

GDREGISTER_CLASS(Shortcut);
GDREGISTER_VIRTUAL_CLASS(InputEvent);
GDREGISTER_VIRTUAL_CLASS(InputEventWithModifiers);
GDREGISTER_VIRTUAL_CLASS(InputEventFromWindow);
GDREGISTER_ABSTRACT_CLASS(InputEvent);
GDREGISTER_ABSTRACT_CLASS(InputEventWithModifiers);
GDREGISTER_ABSTRACT_CLASS(InputEventFromWindow);
GDREGISTER_CLASS(InputEventKey);
GDREGISTER_CLASS(InputEventShortcut);
GDREGISTER_VIRTUAL_CLASS(InputEventMouse);
GDREGISTER_ABSTRACT_CLASS(InputEventMouse);
GDREGISTER_CLASS(InputEventMouseButton);
GDREGISTER_CLASS(InputEventMouseMotion);
GDREGISTER_CLASS(InputEventJoypadButton);
GDREGISTER_CLASS(InputEventJoypadMotion);
GDREGISTER_CLASS(InputEventScreenDrag);
GDREGISTER_CLASS(InputEventScreenTouch);
GDREGISTER_CLASS(InputEventAction);
GDREGISTER_VIRTUAL_CLASS(InputEventGesture);
GDREGISTER_ABSTRACT_CLASS(InputEventGesture);
GDREGISTER_CLASS(InputEventMagnifyGesture);
GDREGISTER_CLASS(InputEventPanGesture);
GDREGISTER_CLASS(InputEventMIDI);

// Network
GDREGISTER_VIRTUAL_CLASS(IP);
GDREGISTER_ABSTRACT_CLASS(IP);

GDREGISTER_VIRTUAL_CLASS(StreamPeer);
GDREGISTER_ABSTRACT_CLASS(StreamPeer);
GDREGISTER_CLASS(StreamPeerExtension);
Comment on lines +173 to 174
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

GDREGISTER_CLASS(StreamPeerBuffer);
GDREGISTER_CLASS(StreamPeerTCP);
GDREGISTER_CLASS(TCPServer);

GDREGISTER_VIRTUAL_CLASS(PacketPeer);
GDREGISTER_ABSTRACT_CLASS(PacketPeer);
GDREGISTER_CLASS(PacketPeerExtension);
GDREGISTER_CLASS(PacketPeerStream);
GDREGISTER_CLASS(PacketPeerUDP);
Expand All @@ -200,7 +200,7 @@ void register_core_types() {
resource_format_loader_crypto.instantiate();
ResourceLoader::add_resource_format_loader(resource_format_loader_crypto);

GDREGISTER_VIRTUAL_CLASS(MultiplayerPeer);
GDREGISTER_ABSTRACT_CLASS(MultiplayerPeer);
GDREGISTER_CLASS(MultiplayerPeerExtension);
GDREGISTER_CLASS(MultiplayerAPI);
GDREGISTER_CLASS(MainLoop);
Expand All @@ -226,19 +226,19 @@ void register_core_types() {
GDREGISTER_CLASS(PCKPacker);

GDREGISTER_CLASS(PackedDataContainer);
GDREGISTER_VIRTUAL_CLASS(PackedDataContainerRef);
GDREGISTER_ABSTRACT_CLASS(PackedDataContainerRef);
GDREGISTER_CLASS(AStar);
GDREGISTER_CLASS(AStar2D);
GDREGISTER_CLASS(EncodedObjectAsID);
GDREGISTER_CLASS(RandomNumberGenerator);

GDREGISTER_VIRTUAL_CLASS(ResourceImporter);
GDREGISTER_ABSTRACT_CLASS(ResourceImporter);
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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


GDREGISTER_CLASS(NativeExtension);

GDREGISTER_VIRTUAL_CLASS(NativeExtensionManager);
GDREGISTER_ABSTRACT_CLASS(NativeExtensionManager);

GDREGISTER_VIRTUAL_CLASS(ResourceUID);
GDREGISTER_ABSTRACT_CLASS(ResourceUID);

GDREGISTER_CLASS(EngineProfiler);

Expand Down Expand Up @@ -276,7 +276,7 @@ void register_core_settings() {

void register_core_singletons() {
GDREGISTER_CLASS(ProjectSettings);
GDREGISTER_VIRTUAL_CLASS(IP);
GDREGISTER_ABSTRACT_CLASS(IP);
GDREGISTER_CLASS(core_bind::Geometry2D);
GDREGISTER_CLASS(core_bind::Geometry3D);
GDREGISTER_CLASS(core_bind::ResourceLoader);
Expand All @@ -286,7 +286,7 @@ void register_core_singletons() {
GDREGISTER_CLASS(core_bind::special::ClassDB);
GDREGISTER_CLASS(core_bind::Marshalls);
GDREGISTER_CLASS(TranslationServer);
GDREGISTER_VIRTUAL_CLASS(Input);
GDREGISTER_ABSTRACT_CLASS(Input);
GDREGISTER_CLASS(InputMap);
GDREGISTER_CLASS(Expression);
GDREGISTER_CLASS(core_bind::EngineDebugger);
Expand Down
7 changes: 7 additions & 0 deletions doc/classes/AudioEffect.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,11 @@
<tutorials>
<link title="Audio Mic Record Demo">https://godotengine.org/asset-library/asset/527</link>
</tutorials>
<methods>
<method name="_instantiate" qualifiers="virtual">
<return type="AudioEffectInstance" />
<description>
</description>
</method>
</methods>
</class>
15 changes: 15 additions & 0 deletions doc/classes/AudioEffectInstance.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,19 @@
</description>
<tutorials>
</tutorials>
<methods>
<method name="_process" qualifiers="virtual">
<return type="void" />
<argument index="0" name="src_buffer" type="const void*" />
<argument index="1" name="dst_buffer" type="AudioFrame*" />
<argument index="2" name="frame_count" type="int" />
<description>
</description>
</method>
<method name="_process_silence" qualifiers="virtual const">
<return type="bool" />
<description>
</description>
</method>
</methods>
</class>
19 changes: 19 additions & 0 deletions doc/classes/AudioStreamPlaybackResampled.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,23 @@
</description>
<tutorials>
</tutorials>
<methods>
<method name="_get_stream_sampling_rate" qualifiers="virtual const">
<return type="float" />
<description>
</description>
</method>
<method name="_mix_resampled" qualifiers="virtual">
<return type="int" />
<argument index="0" name="dst_buffer" type="AudioFrame*" />
<argument index="1" name="frame_count" type="int" />
<description>
</description>
</method>
<method name="begin_resample">
<return type="void" />
<description>
</description>
</method>
</methods>
</class>
22 changes: 21 additions & 1 deletion doc/classes/Material.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,26 @@
<link title="Third Person Shooter Demo">https://godotengine.org/asset-library/asset/678</link>
</tutorials>
<methods>
<method name="_can_do_next_pass" qualifiers="virtual const">
<return type="bool" />
<description>
</description>
</method>
<method name="_can_use_render_priority" qualifiers="virtual const">
<return type="bool" />
<description>
</description>
</method>
<method name="_get_shader_mode" qualifiers="virtual const">
<return type="int" enum="Shader.Mode" />
<description>
</description>
</method>
<method name="_get_shader_rid" qualifiers="virtual const">
<return type="RID" />
<description>
</description>
</method>
<method name="inspect_native_shader_code">
<return type="void" />
<description>
Expand All @@ -22,7 +42,7 @@
Sets the [Material] to be used for the next pass. This renders the object again using a different material.
[b]Note:[/b] This only applies to [StandardMaterial3D]s and [ShaderMaterial]s with type "Spatial".
</member>
<member name="render_priority" type="int" setter="set_render_priority" getter="get_render_priority" default="0">
<member name="render_priority" type="int" setter="set_render_priority" getter="get_render_priority">
Sets the render priority for transparent objects in 3D scenes. Higher priority objects will be sorted in front of lower priority objects.
[b]Note:[/b] This only applies to [StandardMaterial3D]s and [ShaderMaterial]s with type "Spatial".
[b]Note:[/b] This only applies to sorting of transparent objects. This will not impact how transparent objects are sorted relative to opaque objects. This is because opaque objects are not sorted, while transparent objects are sorted from back to front (subject to priority).
Expand Down
Loading