Skip to content

PR #2/5 Astolfo feature/builtin-macro #66

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

Closed
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
1 change: 1 addition & 0 deletions godot-core/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ unit-test = ["godot-ffi/unit-test"] # If this crate is built for a downstream un
[dependencies]
godot-ffi = { path = "../godot-ffi" }
once_cell = "1.8"
paste = "1.0"

# See https://docs.rs/glam/latest/glam/index.html#feature-gates
glam = { version = "0.22", features = ["debug-glam-assert", "scalar-math"] }
Expand Down
27 changes: 27 additions & 0 deletions godot-core/src/builtin/real.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/*
* 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/.
*/

pub use single::*;

mod single {
/// Floating-point type used throughout the engine. This is the equivalent of `real_t` in the
/// engine's C++ code.
///
/// Currently, this is always `f32`; working with an engine compiled with `precision=double` is
/// not supported yet.
Comment on lines +13 to +14
Copy link
Member

Choose a reason for hiding this comment

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

Since we don't support double, I would avoid all the indirection through Real and just use f32 and the respective glam types everywhere. This differentiation is relatively easy to add later, if necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

I seem to recall that the engine uses float in some places even if compiled with precision=double, so it seemed smart to make people think about this every time they use a floating-point type in gdextension. However, there's no guarantee that adding Real prematurely will do this. Removing it.

Copy link
Member

Choose a reason for hiding this comment

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

Once (if ever) Godot 4 fully supports switching between float and double, we should probably spend some time to design how to integrate that feature. It will affect a lot of places like codegen, FFI etc., so a solution should consider all of those.

Copy link

Choose a reason for hiding this comment

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

Doesn't Godot already support double precision?
godotengine/godot-proposals#892
Granted it's a compile-time flag but all the main PRs have been merged and the biggest issues with it (camera jitter) have been fixed already to my knowledge. Seeing as they are taking bug reports regarding double-precision usage I think it's safe to say it's supported. Unless I'm misunderstanding your comment 😄

Copy link
Member

Choose a reason for hiding this comment

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

Ohh, even better! I seem to have missed it 😬

Thanks for the input! Support for double is then something we might consider for godot-rust, although I'd like to get the core features ready first.

pub type Real = f32;

pub(crate) type Vec2 = glam::f32::Vec2;
pub(crate) type Vec3 = glam::f32::Vec3;
pub(crate) type Vec4 = glam::f32::Vec4;
}

// mod double {
// pub type Real = f64;
// pub(crate) type Vec2 = glam::f64::DVec2;
// pub(crate) type Vec3 = glam::f64::DVec3;
// pub(crate) type Vec4 = glam::f64::DVec4;
// }
213 changes: 213 additions & 0 deletions godot-core/src/builtin/vector_macros.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,213 @@
/*
* 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]

/// Helper for `impl_vector` to create format strings for `format!` and friends.
macro_rules! format_string {
($a:ident, $b:ident) => { "({}, {})" };
($a:ident, $b:ident, $c: ident) => { "({}, {}, {})" };
($a:ident, $b:ident, $c: ident, $d:ident) => { "({}, {}, {}, {})" };
}

/// Helper for `impl_vector` to implement a set of operators for a vector type.
macro_rules! impl_vector_operators {
(
// Name of the vector type.
$vector:ty,
// Type of each individual component, for example `i32`.
$component_type:ty,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this could be called scalar instead of component_type.

// Name of the operator trait.
$operator:ident
) => {
// `paste` is used for conversion to snake-case: AddAssign -> add_assign.
paste::paste! {
// vector + vector
impl std::ops::$operator for $vector {
type Output = Self;
fn [<$operator:snake>](self, rhs: $vector) -> Self::Output {
self.0.[<$operator:snake>](rhs.0).into()
}
}

// vector + scalar
impl std::ops::$operator<$component_type> for $vector {
type Output = Self;
fn [<$operator:snake>](self, rhs: $component_type) -> Self::Output {
self.0.[<$operator:snake>](rhs).into()
}
}

// scalar + vector
impl std::ops::$operator<$vector> for $component_type {
type Output = $vector;
fn [<$operator:snake>](self, rhs: $vector) -> Self::Output {
self.[<$operator:snake>](rhs.0).into()
}
}

// vector += vector
impl std::ops::[<$operator Assign>] for $vector {
fn [<$operator:snake _assign>](&mut self, rhs: $vector) {
self.0.[<$operator:snake _assign>](rhs.0);
}
}

// vector += scalar
impl std::ops::[<$operator Assign>]<$component_type> for $vector {
fn [<$operator:snake _assign>](&mut self, rhs: $component_type) {
self.0.[<$operator:snake _assign>](rhs);
}
}
}
}
}

/// Implements the basics of a built-in vector type.
macro_rules! impl_vector {
(
// Name of the vector type to be created.
$vector:ident,
// Name of the inner (wrapped) type, typically from glam.
$inner_type:ty,
// Type of each individual component, for example `i32`.
$component_type:ty,
// Names of the components, for example `(x, y)`.
($($components:ident),*)$(,)?
) => {
// Inside a `paste!` invocation, everything between [<...>] gets concatenated.
paste::paste! {
#[derive(Default, Copy, Clone, Debug, PartialEq)]
#[repr(C)]
pub struct $vector($inner_type);

impl $vector {
/// Creates a new vector with the given components.
#[inline]
pub const fn new($($components: $component_type),*) -> Self {
Self($inner_type::new($($components),*))
}

/// Creates a new vector with all components set to `v`.
#[inline]
pub const fn splat(v: $component_type) -> Self {
Self($inner_type::splat(v))
}

$(
#[doc = "Returns the `" $components "` component of this vector."]
#[inline]
pub fn $components(&self) -> $component_type {
self.0.$components
}

#[doc = "Sets the `" $components "` component of this vector."]
#[inline]
pub fn [<set_ $components>](&mut self, $components: $component_type) {
self.0.$components = $components;
}
)*
}
Comment on lines +100 to +113
Copy link
Member

@Bromeon Bromeon Jan 16, 2023

Choose a reason for hiding this comment

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

As mentioned elsewhere, I would just use public x and y.

Copy link
Contributor

@ttencate ttencate Jan 17, 2023

Choose a reason for hiding this comment

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

... which means not wrapping glam. (Or Deref'ing to glam but that still exposes glam in the public API which we don't want.) Let's do it!


impl From<$inner_type> for $vector {
/// Wraps an inner type in a Godot vector.
fn from(inner: $inner_type) -> Self {
Self(inner)
}
}

impl From<$vector> for $inner_type {
/// Unwraps a Godot vector into its inner type.
fn from(vector: $vector) -> Self {
vector.0
}
}
Comment on lines +115 to +127
Copy link
Member

Choose a reason for hiding this comment

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

This should not be public (and thus cannot use From trait). Problem is that it makes the internally used glam version part of the public interface and causes version conflicts. See godot-rust/gdnative#594 (comment) for 3 examples where this happened with euclid.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, adding from_glam and as_glam instead.

Copy link
Member

Choose a reason for hiding this comment

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

If you return a copy, maybe use to_glam(). I think the Rust convention for "as_" is to return references.


impl std::fmt::Display for $vector {
/// Formats this vector in the same way the Godot engine would.
fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
write!(
f,
format_string!($($components),*),
$(self.$components()),*)
}
}
Comment on lines +129 to +137
Copy link
Member

@Bromeon Bromeon Jan 16, 2023

Choose a reason for hiding this comment

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

This is quite hard-to-understand code and needs an external macro format_string! to power its implementation. A reader needs to spend significant time to reconstruct how this string is built. We don't really win anything by having less code.

I'd rather lift the impl Display out of the macro:

impl std::fmt::Display for Vector3 {
	/// Formats this vector in the same way the Godot engine would.
	fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result {
        	let Self { x, y, z } = self;
		write!(f, "({x}, {y}, {z})")
	}
}

This is much easier to read and makes immediately obvious how the resulting string looks.


impl_vector_operators!($vector, $component_type, Add);
impl_vector_operators!($vector, $component_type, Sub);
impl_vector_operators!($vector, $component_type, Mul);
impl_vector_operators!($vector, $component_type, Div);

impl std::ops::Neg for $vector {
type Output = Self;
fn neg(self) -> Self {
self.0.neg().into()
}
}

impl godot_ffi::GodotFfi for $vector {
godot_ffi::ffi_methods! { type godot_ffi::GDExtensionTypePtr = *mut Self; .. }
}
}
}
}

/// Implements `From` that does a component-wise cast to convert one vector type to another.
macro_rules! impl_vector_from {
(
// Name of the vector type.
$vector:ty,
// Name of the original type.
$from:ty,
// Type of target component, for example `Real`.
$component_type:ty,
// Names of the components, for example `(x, y)`.
($($components:ident),*)$(,)?
) => {
paste::paste! {
impl From<$from> for $vector {
#[doc = "Converts a `" $from "` into a `" $vector "`. Note that this might be a lossy operation."]
fn from(from: $from) -> Self {
Self::new($(from.$components() as $component_type),*)
}
}
}
}
}
Comment on lines +158 to +179
Copy link
Member

Choose a reason for hiding this comment

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

We don't want From for lossy operations. There is no impl From<f32> for i32, either.

And I would not even add From the other way around. Since this is a rarely used conversion and has a lot of potential to be used wrongly (prime example: convert between pixels/world coordinates instead of going through proper screen-space transforms), I'd prefer explicitly named methods, if at all.

Does glam provide this conversion? I haven't found it.

Copy link
Contributor

@ttencate ttencate Jan 17, 2023

Choose a reason for hiding this comment

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

I was trying to Rustify the engine API here, which does it with conversion constructors Vector2(Vector2i from). Explicitly named ctors are a better approach indeed.

Copy link
Member

Choose a reason for hiding this comment

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

We could do something like:

Vector2i::from_float_lossy(v: Vector2)
Vector2::from_int(v: Vector2i)


/// Implements common constants and methods for floating-point type vectors.
macro_rules! impl_float_vector {
(
// Name of the vector type.
$vector:ty,
// Type of target component, for example `Real`.
$component_type:ty
) => {
impl $vector {
/// Zero vector, a vector with all components set to `0.0`.
pub const ZERO: Self = Self::splat(0.0);

/// One vector, a vector with all components set to `1.0`.
pub const ONE: Self = Self::splat(1.0);

/// Infinity vector, a vector with all components set to `INFIINTY`.
pub const INF: Self = Self::splat(<$component_type>::INFINITY);

/// Returns the length (magnitude) of this vector.
#[inline]
pub fn length(&self) -> $component_type {
self.0.length()
}

/// Returns the vector scaled to unit length. Equivalent to `self / self.length()`. See
/// also `is_normalized()`.
#[inline]
pub fn normalized(&self) -> Self {
self.0.normalize().into()
}
Comment on lines +200 to +210
Copy link
Member

Choose a reason for hiding this comment

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

Should take self by value.

}
}
}