Skip to content

Virtual function dispatch #136

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

Merged
merged 1 commit into from
Mar 19, 2023
Merged
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
4 changes: 2 additions & 2 deletions examples/dodge-the-creeps/rust/src/hud.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use godot::engine::{Button, CanvasLayer, Label, Timer};
use godot::engine::{Button, CanvasLayer, CanvasLayerVirtual, Label, Timer};
use godot::prelude::*;

#[derive(GodotClass)]
Expand Down Expand Up @@ -61,7 +61,7 @@ impl Hud {
}

#[godot_api]
impl GodotExt for Hud {
impl CanvasLayerVirtual for Hud {
fn init(base: Base<Self::Base>) -> Self {
Self { base }
}
Expand Down
2 changes: 1 addition & 1 deletion examples/dodge-the-creeps/rust/src/main_scene.rs
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ impl Main {
}

#[godot_api]
impl GodotExt for Main {
impl NodeVirtual for Main {
fn init(base: Base<Node>) -> Self {
Main {
mob_scene: PackedScene::new(),
Expand Down
4 changes: 2 additions & 2 deletions examples/dodge-the-creeps/rust/src/mob.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use godot::engine::{AnimatedSprite2D, RigidBody2D};
use godot::engine::{AnimatedSprite2D, RigidBody2D, RigidBody2DVirtual};
use godot::prelude::*;
use rand::seq::SliceRandom;

Expand Down Expand Up @@ -47,7 +47,7 @@ impl Mob {
}

#[godot_api]
impl GodotExt for Mob {
impl RigidBody2DVirtual for Mob {
fn init(base: Base<RigidBody2D>) -> Self {
Mob {
min_speed: 150.0,
Expand Down
4 changes: 2 additions & 2 deletions examples/dodge-the-creeps/rust/src/player.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use godot::engine::{AnimatedSprite2D, Area2D, CollisionShape2D, PhysicsBody2D};
use godot::engine::{AnimatedSprite2D, Area2D, Area2DVirtual, CollisionShape2D, PhysicsBody2D};
use godot::prelude::*;

#[derive(GodotClass)]
Expand Down Expand Up @@ -42,7 +42,7 @@ impl Player {
}

#[godot_api]
impl GodotExt for Player {
impl Area2DVirtual for Player {
fn init(base: Base<Area2D>) -> Self {
Player {
speed: 400.0,
Expand Down
4 changes: 2 additions & 2 deletions godot-codegen/src/api_parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ pub struct BuiltinClassMethod {
pub arguments: Option<Vec<MethodArg>>,
}

#[derive(DeJson)]
#[derive(DeJson, Clone)]
pub struct ClassMethod {
pub name: String,
pub is_const: bool,
Expand Down Expand Up @@ -200,7 +200,7 @@ pub struct MethodArg {
}

// Example: get_available_point_id -> {type: "int", meta: "int64"}
#[derive(DeJson)]
#[derive(DeJson, Clone)]
pub struct MethodReturn {
#[nserde(rename = "type")]
pub type_: String,
Expand Down
160 changes: 143 additions & 17 deletions godot-codegen/src/class_generator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ fn make_constructor(class: &Class, ctx: &Context) -> TokenStream {
fn make_class(class: &Class, class_name: &TyName, ctx: &mut Context) -> GeneratedClass {
// Strings
let godot_class_str = &class_name.godot_ty;
let virtual_trait_str = class_name.virtual_trait_name();

// Idents and tokens
let base = match class.inherits.as_ref() {
Expand All @@ -174,6 +175,7 @@ fn make_class(class: &Class, class_name: &TyName, ctx: &mut Context) -> Generate
let enums = make_enums(&class.enums, class_name, ctx);
let inherits_macro = format_ident!("inherits_transitive_{}", class_name.rust_ty);
let all_bases = ctx.inheritance_tree().collect_all_bases(class_name);
let virtual_trait = make_virtual_methods_trait(class, &all_bases, &virtual_trait_str, ctx);

let memory = if class_name.rust_ty == "Object" {
ident("DynamicRefCount")
Expand All @@ -199,6 +201,7 @@ fn make_class(class: &Class, class_name: &TyName, ctx: &mut Context) -> Generate
pub struct #class_name {
object_ptr: sys::GDExtensionObjectPtr,
}
#virtual_trait
impl #class_name {
#constructor
#methods
Expand Down Expand Up @@ -323,12 +326,14 @@ fn make_module_file(classes_and_modules: Vec<GeneratedClassModule>) -> TokenStre
is_pub,
..
} = m;
let virtual_trait_name = ident(&class_name.virtual_trait_name());

let vis = is_pub.then_some(quote! { pub });

quote! {
#vis mod #module_name;
pub use #module_name::re_export::#class_name;
pub use #module_name::re_export::#virtual_trait_name;
}
});

Expand Down Expand Up @@ -463,12 +468,15 @@ fn is_type_excluded(ty: &str, ctx: &mut Context) -> bool {
}
}

fn is_method_excluded(method: &ClassMethod, #[allow(unused_variables)] ctx: &mut Context) -> bool {
fn is_method_excluded(
method: &ClassMethod,
is_virtual_impl: bool,
#[allow(unused_variables)] ctx: &mut Context,
) -> bool {
// Currently excluded:
//
// * Private virtual methods designed for override; skip for now
// E.g.: AudioEffectInstance::_process(const void*, AudioFrame*, int)
// TODO decide what to do with them, overriding in a type-safe way?
// * Private virtual methods are only included in a virtual
// implementation.
//
// * Methods accepting pointers are often supplementary
// E.g.: TextServer::font_set_data_ptr() -- in addition to TextServer::font_set_data().
Expand All @@ -490,11 +498,14 @@ fn is_method_excluded(method: &ClassMethod, #[allow(unused_variables)] ctx: &mut
}
// -- end.

method.name.starts_with('_')
|| method
.return_value
.as_ref()
.map_or(false, |ret| ret.type_.contains('*'))
if method.name.starts_with('_') && !is_virtual_impl {
return true;
}
Comment on lines +501 to +503
Copy link
Member

Choose a reason for hiding this comment

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

You removed the condition about pointers, why that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the blanket ban on all method names starting with _. All virtual methods have a Godot name that starts with an underscore, so that would exclude all of the methods I intend to add with this MR. The pointer condition is still present (line 508). Looks like the Rust formatter just re-indented it a bit, which is why it shows as a diff.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. This is where semantic diff like difftastic might come in really handy 😎

All good then, thanks for explanation!


method
.return_value
.as_ref()
.map_or(false, |ret| ret.type_.contains('*'))
|| method
.arguments
.as_ref()
Expand Down Expand Up @@ -523,7 +534,8 @@ fn make_method_definition(
class_name: &TyName,
ctx: &mut Context,
) -> TokenStream {
if is_method_excluded(method, ctx) || special_cases::is_deleted(class_name, &method.name) {
if is_method_excluded(method, false, ctx) || special_cases::is_deleted(class_name, &method.name)
{
return TokenStream::new();
}
/*if method.map_args(|args| args.is_empty()) {
Expand Down Expand Up @@ -799,13 +811,7 @@ fn make_receiver(
is_const: bool,
receiver_arg: TokenStream,
) -> (TokenStream, TokenStream) {
let receiver = if is_static {
quote! {}
} else if is_const {
quote! { &self, }
} else {
quote! { &mut self, }
};
let receiver = make_receiver_self_param(is_static, is_const);

let receiver_arg = if is_static {
quote! { std::ptr::null_mut() }
Expand All @@ -816,6 +822,16 @@ fn make_receiver(
(receiver, receiver_arg)
}

fn make_receiver_self_param(is_static: bool, is_const: bool) -> TokenStream {
if is_static {
quote! {}
} else if is_const {
quote! { &self, }
} else {
quote! { &mut self, }
}
}

fn make_params(
method_args: &Option<Vec<MethodArg>>,
is_varcall: bool,
Expand Down Expand Up @@ -930,3 +946,113 @@ fn make_return(

(return_decl, call)
}

fn make_virtual_methods_trait(
class: &Class,
all_bases: &[TyName],
trait_name: &str,
ctx: &mut Context,
) -> TokenStream {
let trait_name = ident(trait_name);

let virtual_method_fns = make_all_virtual_methods(class, all_bases, ctx);
let special_virtual_methods = special_virtual_methods();

quote! {
#[allow(unused_variables)]
#[allow(clippy::unimplemented)]
pub trait #trait_name: crate::private::You_forgot_the_attribute__godot_api + crate::obj::GodotClass {
#( #virtual_method_fns )*
#special_virtual_methods
}
}
}

fn special_virtual_methods() -> TokenStream {
quote! {
fn register_class(builder: &mut crate::builder::ClassBuilder<Self>) {
unimplemented!()
}
fn init(base: crate::obj::Base<Self::Base>) -> Self {
unimplemented!()
}
fn to_string(&self) -> crate::builtin::GodotString {
unimplemented!()
}
}
}

fn make_virtual_method(class_method: &ClassMethod, ctx: &mut Context) -> TokenStream {
let method_name = ident(virtual_method_name(class_method));

// Virtual methods are never static.
assert!(!class_method.is_static);

let receiver = make_receiver_self_param(false, class_method.is_const);
let [params, _, _, _] = make_params(&class_method.arguments, class_method.is_vararg, ctx);

quote! {
fn #method_name ( #receiver #( #params , )* ) {
unimplemented!()
}
}
}

fn make_all_virtual_methods(
class: &Class,
all_bases: &[TyName],
ctx: &mut Context,
) -> Vec<TokenStream> {
let mut all_virtuals = vec![];
let mut extend_virtuals = |class| {
all_virtuals.extend(
get_methods_in_class(class)
.iter()
.cloned()
.filter(|m| m.is_virtual),
);
};

// Get virtuals defined on the current class.
extend_virtuals(class);
// Add virtuals from superclasses.
for base in all_bases {
let superclass = ctx.get_engine_class(base);
extend_virtuals(superclass);
}
all_virtuals
.into_iter()
.filter_map(|method| {
if is_method_excluded(&method, true, ctx) {
None
} else {
Some(make_virtual_method(&method, ctx))
}
})
.collect()
}

fn get_methods_in_class(class: &Class) -> &[ClassMethod] {
match &class.methods {
None => &[],
Some(methods) => methods,
}
}

fn virtual_method_name(class_method: &ClassMethod) -> &str {
// Matching the C++ convention, we remove the leading underscore
// from virtual method names.
let method_name = class_method
.name
.strip_prefix('_')
.unwrap_or(&class_method.name);

// As a special exception, a few classes define a virtual method
// called "_init" (distinct from the constructor), so we rename
// those to avoid a name conflict in our trait.
if method_name == "init" {
"init_ext"
} else {
method_name
}
}
9 changes: 7 additions & 2 deletions godot-codegen/src/context.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,13 @@
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

use crate::api_parser::Class;
use crate::{ExtensionApi, RustTy, TyName};
use std::collections::{HashMap, HashSet};

#[derive(Default)]
pub(crate) struct Context<'a> {
engine_classes: HashSet<TyName>,
engine_classes: HashMap<TyName, &'a Class>,
builtin_types: HashSet<&'a str>,
singletons: HashSet<&'a str>,
inheritance_tree: InheritanceTree,
Expand Down Expand Up @@ -39,7 +40,7 @@ impl<'a> Context<'a> {
}

println!("-- add engine class {}", class_name.description());
ctx.engine_classes.insert(class_name.clone());
ctx.engine_classes.insert(class_name.clone(), class);

if let Some(base) = class.inherits.as_ref() {
let base_name = TyName::from_godot(base);
Expand All @@ -50,6 +51,10 @@ impl<'a> Context<'a> {
ctx
}

pub fn get_engine_class(&self, class_name: &TyName) -> &Class {
self.engine_classes.get(class_name).unwrap()
}

// pub fn is_engine_class(&self, class_name: &str) -> bool {
// self.engine_classes.contains(class_name)
// }
Expand Down
4 changes: 4 additions & 0 deletions godot-codegen/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,10 @@ impl TyName {
format!("{} [renamed {}]", self.godot_ty, self.rust_ty)
}
}

fn virtual_trait_name(&self) -> String {
format!("{}Virtual", self.rust_ty)
}
}

impl ToTokens for TyName {
Expand Down
Loading