Skip to content

Godot virtuals are no longer required if a derived Godot class implements them #1136

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 14 additions & 4 deletions godot-codegen/src/generator/virtual_traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ use crate::generator::{docs, functions_common};
use crate::models::domain::{
ApiView, Class, ClassLike, ClassMethod, FnQualifier, Function, TyName,
};
use crate::special_cases;
use crate::util::ident;
use proc_macro2::{Ident, TokenStream};
use quote::quote;
Expand Down Expand Up @@ -150,7 +151,10 @@ fn make_special_virtual_methods(notification_enum_name: &Ident) -> TokenStream {
}
}

fn make_virtual_method(method: &ClassMethod) -> Option<TokenStream> {
fn make_virtual_method(
method: &ClassMethod,
override_is_required: Option<bool>,
) -> Option<TokenStream> {
if !method.is_virtual() {
return None;
}
Expand All @@ -166,7 +170,8 @@ fn make_virtual_method(method: &ClassMethod) -> Option<TokenStream> {
// make_return() requests following args, but they are not used for virtual methods. We can provide empty streams.
varcall_invocation: TokenStream::new(),
ptrcall_invocation: TokenStream::new(),
is_virtual_required: method.is_virtual_required(),
is_virtual_required: override_is_required
.unwrap_or_else(|| method.is_virtual_required()),
is_varcall_fallible: true,
},
None,
Expand All @@ -186,15 +191,20 @@ fn make_all_virtual_methods(

for method in class.methods.iter() {
// Assumes that inner function filters on is_virtual.
if let Some(tokens) = make_virtual_method(method) {
if let Some(tokens) = make_virtual_method(method, None) {
all_tokens.push(tokens);
}
}

for base_name in all_base_names {
let base_class = view.get_engine_class(base_name);
for method in base_class.methods.iter() {
if let Some(tokens) = make_virtual_method(method) {
// Certain derived classes in Godot implement a virtual method declared in a base class, thus no longer
// making it required. This isn't advertised in the extension_api, but instead manually tracked via special cases.
let is_required =
special_cases::is_derived_virtual_method_required(class.name(), method.name());

if let Some(tokens) = make_virtual_method(method, is_required) {
all_tokens.push(tokens);
}
}
Expand Down
18 changes: 12 additions & 6 deletions godot-codegen/src/models/domain_mapping.rs
Original file line number Diff line number Diff line change
Expand Up @@ -483,20 +483,26 @@ impl ClassMethod {

// Since Godot 4.4, GDExtension advertises whether virtual methods have a default implementation or are required to be overridden.
#[cfg(before_api = "4.4")]
let is_virtual_required = special_cases::is_virtual_method_required(
&class_name.rust_ty.to_string(),
rust_method_name,
);
let is_virtual_required =
special_cases::is_virtual_method_required(&class_name, rust_method_name);

#[cfg(since_api = "4.4")]
let is_virtual_required = method.is_virtual
&& method.is_required.unwrap_or_else(|| {
#[allow(clippy::let_and_return)]
let is_virtual_required = method.is_virtual && {
// Evaluate this always first (before potential manual overrides), to detect mistakes in spec.
let is_required_in_json = method.is_required.unwrap_or_else(|| {
panic!(
"virtual method {}::{} lacks field `is_required`",
class_name.rust_ty, rust_method_name
);
});

// Potential special cases come here. The situation "virtual function is required in base class, but not in derived"
// is not handled here, but in virtual_traits.rs. Here, virtual methods appear only once, in their base.

is_required_in_json
};

Some(Self {
common: FunctionCommon {
name: rust_method_name.to_string(),
Expand Down
57 changes: 35 additions & 22 deletions godot-codegen/src/special_cases/special_cases.rs
Original file line number Diff line number Diff line change
Expand Up @@ -620,9 +620,11 @@ pub fn get_interface_extra_docs(trait_name: &str) -> Option<&'static str> {
}

#[cfg(before_api = "4.4")]
pub fn is_virtual_method_required(class_name: &str, method: &str) -> bool {
match (class_name, method) {
("ScriptLanguageExtension", _) => method != "get_doc_comment_delimiters",
pub fn is_virtual_method_required(class_name: &TyName, rust_method_name: &str) -> bool {
// Do not call is_derived_virtual_method_required() here; that is handled in virtual_traits.rs.

match (class_name.godot_ty.as_str(), rust_method_name) {
("ScriptLanguageExtension", method) => method != "get_doc_comment_delimiters",

("ScriptExtension", "editor_can_reload_from_file")
| ("ScriptExtension", "can_instantiate")
Expand Down Expand Up @@ -660,7 +662,7 @@ pub fn is_virtual_method_required(class_name: &str, method: &str) -> bool {
| ("EditorExportPlugin", "customize_scene")
| ("EditorExportPlugin", "get_customization_configuration_hash")
| ("EditorExportPlugin", "get_name")
| ("EditorVcsInterface", _)
| ("EditorVCSInterface", _)
| ("MovieWriter", _)
| ("TextServerExtension", "has_feature")
| ("TextServerExtension", "get_name")
Expand Down Expand Up @@ -766,23 +768,23 @@ pub fn is_virtual_method_required(class_name: &str, method: &str) -> bool {
| ("PacketPeerExtension", "get_available_packet_count")
| ("PacketPeerExtension", "get_max_packet_size")
| ("StreamPeerExtension", "get_available_bytes")
| ("WebRtcDataChannelExtension", "poll")
| ("WebRtcDataChannelExtension", "close")
| ("WebRtcDataChannelExtension", "set_write_mode")
| ("WebRtcDataChannelExtension", "get_write_mode")
| ("WebRtcDataChannelExtension", "was_string_packet")
| ("WebRtcDataChannelExtension", "get_ready_state")
| ("WebRtcDataChannelExtension", "get_label")
| ("WebRtcDataChannelExtension", "is_ordered")
| ("WebRtcDataChannelExtension", "get_id")
| ("WebRtcDataChannelExtension", "get_max_packet_life_time")
| ("WebRtcDataChannelExtension", "get_max_retransmits")
| ("WebRtcDataChannelExtension", "get_protocol")
| ("WebRtcDataChannelExtension", "is_negotiated")
| ("WebRtcDataChannelExtension", "get_buffered_amount")
| ("WebRtcDataChannelExtension", "get_available_packet_count")
| ("WebRtcDataChannelExtension", "get_max_packet_size")
| ("WebRtcPeerConnectionExtension", _)
| ("WebRTCDataChannelExtension", "poll")
| ("WebRTCDataChannelExtension", "close")
| ("WebRTCDataChannelExtension", "set_write_mode")
| ("WebRTCDataChannelExtension", "get_write_mode")
| ("WebRTCDataChannelExtension", "was_string_packet")
| ("WebRTCDataChannelExtension", "get_ready_state")
| ("WebRTCDataChannelExtension", "get_label")
| ("WebRTCDataChannelExtension", "is_ordered")
| ("WebRTCDataChannelExtension", "get_id")
| ("WebRTCDataChannelExtension", "get_max_packet_life_time")
| ("WebRTCDataChannelExtension", "get_max_retransmits")
| ("WebRTCDataChannelExtension", "get_protocol")
| ("WebRTCDataChannelExtension", "is_negotiated")
| ("WebRTCDataChannelExtension", "get_buffered_amount")
| ("WebRTCDataChannelExtension", "get_available_packet_count")
| ("WebRTCDataChannelExtension", "get_max_packet_size")
| ("WebRTCPeerConnectionExtension", _)
| ("MultiplayerPeerExtension", "get_available_packet_count")
| ("MultiplayerPeerExtension", "get_max_packet_size")
| ("MultiplayerPeerExtension", "set_transfer_channel")
Expand All @@ -800,7 +802,18 @@ pub fn is_virtual_method_required(class_name: &str, method: &str) -> bool {
| ("MultiplayerPeerExtension", "get_unique_id")
| ("MultiplayerPeerExtension", "get_connection_status") => true,

(_, _) => false,
_ => false,
}
}

// Adjustments for Godot 4.4+, where a virtual method is no longer needed (e.g. in a derived class).
#[rustfmt::skip]
pub fn is_derived_virtual_method_required(class_name: &TyName, rust_method_name: &str) -> Option<bool> {
match (class_name.godot_ty.as_str(), rust_method_name) {
// Required in base class, no longer in derived; https://github.com/godot-rust/gdext/issues/1133.
| ("AudioStreamPlaybackResampled", "mix")

=> Some(false), _ => None
}
}

Expand Down
6 changes: 0 additions & 6 deletions godot-macros/src/class/data_models/signal.rs
Original file line number Diff line number Diff line change
Expand Up @@ -302,13 +302,7 @@ impl SignalCollection {
// visibility that exceeds the class visibility). So, we can as well declare the visibility here.
#vis_marker fn #signal_name(&mut self) -> #individual_struct_name<'c, C> {
#individual_struct_name {
// __typed: ::godot::register::TypedSignal::new(self.__internal_obj, #signal_name_str)
// __typed: ::godot::register::TypedSignal::<'c, C, _>::new(self.__internal_obj, #signal_name_str)
// __typed: todo!()

// __typed: self.__internal_obj.into_typed_signal(#signal_name_str)
__typed: ::godot::register::TypedSignal::<'c, C, _>::extract(&mut self.__internal_obj, #signal_name_str)

}
}
});
Expand Down
Loading