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

Refactor GodotString, NodePath, and StringName #233

Merged
merged 1 commit into from
Apr 22, 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
14 changes: 12 additions & 2 deletions godot-core/src/builtin/macros.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,17 +107,27 @@ macro_rules! impl_builtin_traits_inner {
}
}
};


// Requires a `hash` function.
( Hash for $Type:ty ) => {
impl std::hash::Hash for $Type {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.hash().hash(state)
}
}
};
}

macro_rules! impl_builtin_traits {
(
for $Type:ty {
$( $Trait:ident => $gd_method:ident; )*
$( $Trait:ident $(=> $gd_method:ident)?; )*
}
) => (
$(
impl_builtin_traits_inner! {
$Trait for $Type => $gd_method
$Trait for $Type $(=> $gd_method)?
}
)*
)
Expand Down
5 changes: 0 additions & 5 deletions godot-core/src/builtin/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@ pub use callable::*;
pub use color::*;
pub use dictionary_inner::Dictionary;
pub use math::*;
pub use node_path::*;
pub use others::*;
pub use packed_array::*;
pub use plane::*;
Expand All @@ -52,7 +51,6 @@ pub use rect2::*;
pub use rect2i::*;
pub use rid::*;
pub use string::*;
pub use string_name::*;
pub use transform2d::*;
pub use transform3d::*;
pub use variant::*;
Expand Down Expand Up @@ -95,7 +93,6 @@ mod callable;
mod color;
mod glam_helpers;
mod math;
mod node_path;
mod others;
mod packed_array;
mod plane;
Expand All @@ -105,8 +102,6 @@ mod rect2;
mod rect2i;
mod rid;
mod string;
mod string_chars;
mod string_name;
mod transform2d;
mod transform3d;
mod variant;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,17 +10,19 @@ use godot_ffi as sys;
use sys::types::OpaqueString;
use sys::{ffi_methods, interface_fn, GodotFfi};

use super::{
string_chars::validate_unicode_scalar_sequence, FromVariant, ToVariant, Variant,
VariantConversionError,
};
use crate::builtin::inner;

use super::string_chars::validate_unicode_scalar_sequence;
use super::{NodePath, StringName};

/// Godot's reference counted string type.
#[repr(C, align(8))]
pub struct GodotString {
opaque: OpaqueString,
}

impl GodotString {
/// Construct a new empty GodotString.
pub fn new() -> Self {
Self::default()
}
Expand All @@ -29,12 +31,12 @@ impl GodotString {
Self { opaque }
}

ffi_methods! {
type sys::GDExtensionStringPtr = *mut Opaque;

fn from_string_sys = from_sys;
fn from_string_sys_init = from_sys_init;
fn string_sys = sys;
/// Returns a 32-bit integer hash value representing the string.
pub fn hash(&self) -> u32 {
Comment on lines +34 to +35
Copy link
Member

Choose a reason for hiding this comment

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

This name may shadow the Hash::hash(&mut H) method from the standard library. Should probably be fine though, as the trait can be invoked with fully-qualified syntax.

self.as_inner()
.hash()
.try_into()
.expect("Godot hashes are uint32_t")
}

/// Move `self` into a system pointer. This transfers ownership and thus does not call the destructor.
Expand Down Expand Up @@ -83,6 +85,19 @@ impl GodotString {
}
std::slice::from_raw_parts(ptr as *const char, len as usize)
}

ffi_methods! {
type sys::GDExtensionStringPtr = *mut Opaque;

fn from_string_sys = from_sys;
fn from_string_sys_init = from_sys_init;
fn string_sys = sys;
}

#[doc(hidden)]
pub fn as_inner(&self) -> inner::InnerString {
inner::InnerString::from_outer(self)
}
}

// SAFETY:
Expand Down Expand Up @@ -122,9 +137,28 @@ impl_builtin_traits! {
Drop => string_destroy;
Eq => string_operator_equal;
Ord => string_operator_less;
Hash;
}
}

impl fmt::Display for GodotString {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let s: String = self.chars_checked().iter().collect();
f.write_str(s.as_str())
}
}

/// Uses literal syntax from GDScript: `"string"`
impl fmt::Debug for GodotString {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let s = String::from(self);
write!(f, "\"{s}\"")
}
}

// ----------------------------------------------------------------------------------------------------------------------------------------------
// Conversion from/into rust string-types

impl<S> From<S> for GodotString
where
S: AsRef<str>,
Expand Down Expand Up @@ -162,7 +196,14 @@ impl From<&GodotString> for String {
}
}

// TODO From<&NodePath> + test
impl From<GodotString> for String {
/// Converts this `GodotString` to a `String`.
///
/// This is identical to `String::from(&string)`, and as such there is no performance benefit.
fn from(string: GodotString) -> Self {
Self::from(&string)
}
}

impl FromStr for GodotString {
type Err = Infallible;
Expand All @@ -172,49 +213,47 @@ impl FromStr for GodotString {
}
}

impl fmt::Display for GodotString {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let s = String::from(self);
f.write_str(s.as_str())
}
}

/// Uses literal syntax from GDScript: `"string"`
impl fmt::Debug for GodotString {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
let s = String::from(self);
write!(f, "\"{s}\"")
}
}
// ----------------------------------------------------------------------------------------------------------------------------------------------
// Conversion from other Godot string-types

impl ToVariant for &str {
fn to_variant(&self) -> Variant {
GodotString::from(*self).to_variant()
impl From<&StringName> for GodotString {
fn from(string: &StringName) -> Self {
unsafe {
Self::from_sys_init_default(|self_ptr| {
let ctor = sys::builtin_fn!(string_from_string_name);
let args = [string.sys_const()];
ctor(self_ptr, args.as_ptr());
})
}
}
}

impl ToVariant for String {
fn to_variant(&self) -> Variant {
GodotString::from(self).to_variant()
impl From<StringName> for GodotString {
/// Converts this `StringName` to a `GodotString`.
///
/// This is identical to `GodotString::from(&string_name)`, and as such there is no performance benefit.
fn from(string_name: StringName) -> Self {
Self::from(&string_name)
}
}

impl FromVariant for String {
fn try_from_variant(variant: &Variant) -> Result<Self, VariantConversionError> {
Ok(GodotString::try_from_variant(variant)?.to_string())
impl From<&NodePath> for GodotString {
fn from(path: &NodePath) -> Self {
unsafe {
Self::from_sys_init_default(|self_ptr| {
let ctor = sys::builtin_fn!(string_from_node_path);
let args = [path.sys_const()];
ctor(self_ptr, args.as_ptr());
})
}
}
}

// While this is a nice optimisation for ptrcalls, it's not easily possible
// to pass in &GodotString when doing varcalls.
/*
impl PtrCall for &GodotString {
unsafe fn from_ptr_call_arg(arg: *const godot_ffi::GDExtensionTypePtr) -> Self {
&*(*arg as *const GodotString)
}
unsafe fn to_ptr_call_arg(self, arg: godot_ffi::GDExtensionTypePtr) {
std::ptr::write(arg as *mut GodotString, self.clone());
impl From<NodePath> for GodotString {
/// Converts this `NodePath` to a `GodotString`.
///
/// This is identical to `GodotString::from(&path)`, and as such there is no performance benefit.
fn from(path: NodePath) -> Self {
Self::from(&path)
}
}
*/
42 changes: 42 additions & 0 deletions godot-core/src/builtin/string/macros.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

#![macro_use]

macro_rules! impl_rust_string_conv {
($Ty:ty) => {
impl<S> From<S> for $Ty
where
S: AsRef<str>,
{
fn from(string: S) -> Self {
let intermediate = GodotString::from(string.as_ref());
Self::from(&intermediate)
}
}
Comment on lines +11 to +19
Copy link
Member

Choose a reason for hiding this comment

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

In the case of $Ty == GodotString, this would first call

GodotString::from(&str)

and then

GodotString::from(&GodotString)

Which trait impl powers the 2nd conversion? There is no From<&GodotString> for GodotString. While From is reflexive, it shouldn't be for references...?

The background behind my question is that it might perform an unnecessary conversion/copy.
Could you implement from() directly like this?

            fn from(string: S) -> Self {
                // note: no &
                Self::from(GodotString::from(string.as_ref()));
            }

Copy link
Member Author

@lilizoey lilizoey Apr 22, 2023

Choose a reason for hiding this comment

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

I don't use this macro for GodotString, precisely because this macro relies on there already existing a From conversion for GodotString and String. it's only used for StringName and NodePath, because their conversions are identical. But GodotString is a special case.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I see. In that case you can go ahead! 🙂


impl From<&$Ty> for String {
fn from(string: &$Ty) -> Self {
let intermediate = GodotString::from(string);
Self::from(&intermediate)
}
}

impl From<$Ty> for String {
fn from(string: $Ty) -> Self {
Self::from(&string)
}
}

impl std::str::FromStr for $Ty {
type Err = std::convert::Infallible;

fn from_str(string: &str) -> Result<Self, Self::Err> {
Ok(Self::from(string))
}
}
};
}
44 changes: 44 additions & 0 deletions godot-core/src/builtin/string/mod.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
/*
* This Source Code Form is subject to the terms of the Mozilla Public
* License, v. 2.0. If a copy of the MPL was not distributed with this
* file, You can obtain one at https://mozilla.org/MPL/2.0/.
*/

//! Godot-types that are Strings.
mod godot_string;
mod macros;
mod node_path;
mod string_chars;
mod string_name;

use godot_ffi::VariantType;
pub use godot_string::*;
pub use node_path::*;
pub use string_name::*;

use super::{meta::VariantMetadata, FromVariant, ToVariant, Variant, VariantConversionError};

impl ToVariant for &str {
fn to_variant(&self) -> Variant {
GodotString::from(*self).to_variant()
}
}

impl ToVariant for String {
fn to_variant(&self) -> Variant {
GodotString::from(self).to_variant()
}
}

impl FromVariant for String {
fn try_from_variant(variant: &Variant) -> Result<Self, VariantConversionError> {
Ok(GodotString::try_from_variant(variant)?.to_string())
}
}

impl VariantMetadata for String {
fn variant_type() -> VariantType {
VariantType::String
}
}
Loading