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.

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

Add basic IScriptable methods #39

Merged
merged 6 commits into from
Jan 25, 2024
Merged

Conversation

Roms1383
Copy link
Contributor

This small PR adds IScriptable most basic methods :

  • GetClassName
  • IsA
  • IsExactlyA

I'm not sure how to treat DetectScriptableCycles so I left it aside.

to avoid having to create a newtype pattern like so:

use red4ext_rs::{
    conv::ClassType,
    macros::redscript_import,
    types::{CName, Ref},
};

#[repr(transparent)]
pub struct IScriptable(red4ext_rs::types::IScriptable);

impl ClassType for IScriptable {
    type BaseClass = red4ext_rs::types::IScriptable;

    const NAME: &'static str = red4ext_rs::types::IScriptable::NAME;
    const NATIVE_NAME: &'static str = red4ext_rs::types::IScriptable::NATIVE_NAME;
}

#[redscript_import]
impl IScriptable {
    /// public native func GetClassName() -> CName
    #[redscript(native)]
    pub fn get_class_name(self: &Ref<Self>) -> CName;
    /// public native func IsA(className: CName) -> Bool
    #[redscript(native)]
    pub fn is_a(self: &Ref<Self>, class_name: CName) -> bool;
    /// public native func IsExactlyA(className: CName) -> Bool
    #[redscript(native)]
    pub fn is_exactly_a(self: &Ref<Self>, class_name: CName) -> bool;
}

especially when deriving children classes like so:

use crate::interop::iscriptable::IScriptable;
use red4ext_rs::types::Ref;
use red4ext_rs::{conv::ClassType, types::CName};

#[derive(Debug)]
pub struct Event;

impl ClassType for Event {
    type BaseClass = IScriptable;
    const NAME: &'static str = "redEvent";
}

// instead of reimporting each of all parents methods with #[redscript_import], we probably want to upcast instead
impl Event {
    pub fn get_class_name(self: &Ref<Self>) -> CName {
        red4ext_rs::prelude::Ref::<Self>::upcast(self.clone()).get_class_name()
    }
    pub fn is_exactly_a(self: &Ref<Self>, class_name: CName) -> bool {
        red4ext_rs::prelude::Ref::<Self>::upcast(self.clone()).is_exactly_a(class_name)
    }
}

@Roms1383
Copy link
Contributor Author

Roms1383 commented Dec 14, 2023

I haven't tested it yet, will do tomorrow.
That will also allow to check if I need to use *self / self.deref() or self.clone().

@Roms1383
Copy link
Contributor Author

I realize though it's convenient when inheriting for a couple of classes but far from optimal when using tons: each call to upcast when getting up the inheritance tree probably incurs a small performance cost on top of #[redscript_import]'s final call, and in that regards a bindings generator would be more adapted.

That being said, having this implementation on IScriptable only does not hurt much, and still provide some convenience.

What do you think, should I drop this PR ?

@Roms1383
Copy link
Contributor Author

@jac3km4 could you give me your opinion on whether this feature is worth finishing ?

@jac3km4
Copy link
Owner

jac3km4 commented Jan 24, 2024

Some suggestions:

  • I think these methods shouldn't take &self because that'll lead to conflict w methods on the Self type, I'd define the parameter as 'this: Self' instead
  • it might make sense to just define this generically for all types: impl <A> Ref<A>, it's already assumed in a few places that methods are only invoked on IScriptables

@Roms1383 Roms1383 marked this pull request as ready for review January 24, 2024 12:51
Co-authored-by: jac3km4 <[email protected]>
@jac3km4 jac3km4 merged commit 8747075 into jac3km4:master Jan 25, 2024
4 checks passed
@Roms1383 Roms1383 deleted the feat/iscriptable branch January 25, 2024 04:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants