Faulty downcasting

[jiaxiongjiao]

Following up from discord that Gd::try_cast can cast to any derived class.

Both Player and Enemy inherits from CharacterBody3D and can be seen and selected from Godot engine editor.
Player request to scan environment.

if input.is_action_pressed("scan".into(), false) {
    self.scan();
}

Implementation of scan() method.

pub fn scan(&self) {
    print_to_godot(&["=====".to_variant()]);

    let area3d: Gd<Area3D> = self.get_node_as::<Area3D>("ScanArea");
    let near_nodes: Array<Gd<Node3D>> = area3d.get_overlapping_bodies();

    if near_nodes.len() != 0 {
        let first_node: Gd<Node3D> = near_nodes.get(0);

        print_to_godot(&["The first node is: ".to_variant(), first_node.to_variant()]);
        
        // Downcasting first node to Enemy
        let possible_enemy: Option<Gd<Enemy>> = first_node.try_cast::<Enemy>();  // HERE: Downcast to Enemy

        if let Some(enemy) = possible_enemy {
            print_to_godot(&["This is an Enemy: ".to_variant(), enemy.to_variant()]);
        } else {
            print_to_godot(&["Not Enemy type".to_variant()]);
    }

    print_to_godot(&["=====".to_variant()]);
}

Output:

=====
The first node is: Player:<Player#30635197676>
This is an Enemy: Player:<Player#30635197676>
=====

If I change the downcast section to downcast to Player. (The first node did not change. It's still a Player node):

// Downcasting first node to Player
let first_node: Gd<Node3D> = near_nodes.get(0);
let possible_player: Option<Gd<Player>> = first_node.try_cast::<Player>();  // HERE: Downcast to Player

if let Some(player) = possible_player {
    print_to_godot(&["This is a Player: ".to_variant(), player.to_variant()]);
} else {
    print_to_godot(&["Not Player type".to_variant()]);
}

Outputs:

=====
The first node is: Player:<Player#30719083757>
This is a Player: Player:<Player#30719083757>
=====

Conclusion

Basically, the first node, a Player node, can get downcasted into both Player and Enemy.

More:

• I tried to replicate this in example game. But it worked as expected. Mob did not get downcasted to Player........
• I don't know how to test Gd<...> alone.

Edit:

• Ignore TypedArray. It's just Array. Bug still persists after I changed them to Array.

[lilizoey]

I can reproduce this.

Minimal reproduction:

#[derive(GodotClass)]
#[class(init, base=Object)]
pub struct A;

#[derive(GodotClass)]
#[class(init, base=Object)]
pub struct B;

#[derive(GodotClass)]
#[class(init, base=Node)]
pub struct Mini;

#[godot_api]
impl GodotExt for Mini {
    fn ready(&mut self) {
        let a = Gd::<A>::new_default();
        let b = a.upcast::<Object>().try_cast::<B>();
        if let Some(b) = b {
            godot_print!("b is B: {b:?}");
        } else {
            godot_print!("b is not B");
        }
    }
}

output:

b is B: Gd { id: 26759659954, class: A }

This does not happen if you try to cast the a to a Node instead

[Bromeon]

This is quite bad, thanks a lot to both of you for reporting/reproducing the bug! 👍

[lilizoey]

Another strange issue that is probably related is this: https://discord.com/channels/723850269347283004/1043514894198255737/1084878309457928324
Where a downcast succeeds, but trying to call bind on it fails.

[Bromeon]

Another strange issue that is probably related is this

Turned out to be user error (wrong node type in .tscn scene file), so not related.

[lilizoey]

Turned out to be user error (wrong node type in .tscn scene file), so not related.

True but it's still weird that the bind is what fails.

[lilizoey]

So it seems like in c++, the class_tag we're using to check if we can cast things safely is the first Godot-class in the class-hierarchy. so the reason the cast succeeds here is because the class tag of A and B is both Object's class tag

[Bromeon]

Very interesting observation. In GDScript it probably works differently though?

I just had a quick look at godot-cpp:

template <class T>
T *Object::cast_to(Object *p_object) {
	if (p_object == nullptr) {
		return nullptr;
	}
	StringName class_name = T::get_class_static();
	GDExtensionObjectPtr casted = internal::gde_interface->object_cast_to(p_object->_owner, internal::gde_interface->classdb_get_class_tag(class_name._native_ptr()));
	if (casted == nullptr) {
		return nullptr;
	}
	return reinterpret_cast<T *>(internal::gde_interface->object_get_instance_binding(casted, internal::token, &T::___binding_callbacks));
}

And we do it like this:

unsafe fn ffi_cast<U>(&self) -> Option<Gd<U>>
    where
        U: GodotClass,
    {
        let class_name = ClassName::of::<U>();
        let class_tag = interface_fn!(classdb_get_class_tag)(class_name.string_sys());
        let cast_object_ptr = interface_fn!(object_cast_to)(self.obj_sys(), class_tag);

        // Create weak object, as ownership will be moved and reference-counter stays the same
        sys::ptr_then(cast_object_ptr, |ptr| Gd::from_obj_sys_weak(ptr))
    }

So they go through object_get_instance_binding, but that's already past the check whether a cast succeeds or not. It seems like the first part that checks "null or not" is the same for both...

[lilizoey]

ok so this is a bug in godot. we can likely fix it when godotengine/godot#73511 gets merged

godot cpp also has the same bug
godotengine/godot-cpp#995

[Bromeon]

@SayakS Thanks a lot for investigating this! Marked it as upstream now, so let's wait until that bugfix gets merged 🙂

[someguynamedjosh]

Temporary workaround:

/// Necessary to work around a bug in GDExtension.
pub fn safe_cast<T: GodotClass + Inherits<Object>>(mut obj: Gd<Object>) -> Result<Gd<T>, ()> {
    let class_name = obj.call(StringName::from("get_class"), &[]);
    if class_name.to_string() == T::CLASS_NAME {
        Ok(obj.cast::<T>())
    } else {
        Err(())
    }
}

/// Necessary to work around a bug in GDExtension.
pub fn safe_cast_variant<T: GodotClass + Inherits<Object>>(
    obj: Variant,
) -> Result<Gd<T>, VariantConversionError> {
    safe_cast(obj.try_to()?).map_err(|_| VariantConversionError)
}

[Bromeon]

@joshua-maros awesome, thanks a lot!

I think until godotengine/godot-cpp#995 is fixed, such a workaround would be nice in the core library.
Would you be interested in opening a pull request?