PR #2/5 Astolfo feature/builtin-macro #66
Conversation
Co-Authored-By: Thomas ten Cate <ttencate@gmail.com>
There was a problem hiding this comment.
Thanks for these macros!
I think the macros are suitable to avoid code duplication for cases that are very boiler-platey or trivial. However, this PR goes a bit too far in some places, as it forcibly reuses code even if that code is significantly different. This necessitates extra helpers to make the reuse work, making the logic non-local and harder to read.
The use of paste! for snake_case conversion + concatenation is quite clever! 😎
I hope it doesn't affect compile times too badly.
| /// Currently, this is always `f32`; working with an engine compiled with `precision=double` is | ||
| /// not supported yet. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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.
| #[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() | ||
| } |
| 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 | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Alright, adding from_glam and as_glam instead.
There was a problem hiding this comment.
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()),*) | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| /// 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),*) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We could do something like:
Vector2i::from_float_lossy(v: Vector2)
Vector2::from_int(v: Vector2i)| $( | ||
| #[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; | ||
| } | ||
| )* | ||
| } |
There was a problem hiding this comment.
As mentioned elsewhere, I would just use public x and y.
There was a problem hiding this comment.
... 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!
| // Name of the vector type. | ||
| $vector:ty, | ||
| // Type of each individual component, for example `i32`. | ||
| $component_type:ty, |
There was a problem hiding this comment.
Maybe this could be called scalar instead of component_type.
|
Since @RealAstolfo allowed me to take this PR back, I've created a new PR: #71. There are a lot of changes compared to this one, to address @Bromeon's comments above. Most notably we now have public x/y/z fields, which changes how the rest of the API is implemented. @RealAstolfo This will probably cause you some merge pain; I'm very sorry about that. |
71: Implement the basics of built-in vector types r=Bromeon a=ttencate
For Vector{2,3,4}{,i} this implements:
- public fields
- constructors
- constants
- operators
- indexing by axis
- (private) conversions to/from glam types
- Display
- a couple of functions like `abs()` and `length()` for demonstration
See also #6.
This PR supersedes #66.
Co-authored-by: Thomas ten Cate <ttencate@gmail.com>
Co-authored-by: Jan Haller <bromeon@gmail.com>
|
Now implemented in #71. Thanks a lot to both of you, the discussion of potential vector designs was really fruitful! ❤️ |
Co-Authored-By: Thomas ten Cate ttencate@gmail.com
These are the macros created by ttencate, and is meant to be merged after #65,
this pr implements "Real" to be closer to upstream godot's C++ internals, also adds a vector macro to construct vector2 through vector4 easier.