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

Implement the basics of built-in vector types #71

Merged
merged 2 commits into from
Jan 20, 2023

Conversation

ttencate
Copy link
Contributor

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.

Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot, this looks very clean as a foundation for future expansion!

Commented inline.

godot-core/src/builtin/mod.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/mod.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector_macros.rs Show resolved Hide resolved
godot-core/src/builtin/vector2i.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector3i.rs Show resolved Hide resolved
godot-core/src/builtin/vector2.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector_macros.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector_macros.rs Outdated Show resolved Hide resolved
Comment on lines 100 to 162
// vector += vector
impl std::ops::$operator for $vector {
fn $func(&mut self, rhs: $vector) {
$(
self.$components.$func(rhs.$components);
)*
}
}

// vector += scalar
impl std::ops::$operator<$scalar> for $vector {
fn $func(&mut self, rhs: $scalar) {
$(
self.$components.$func(rhs);
)*
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

See above.

In general, the operation a @= b should be provided if and only if a = a @ b is provided.
(for any operator @)

godot-core/src/builtin/vector_macros.rs Outdated Show resolved Hide resolved
@ttencate ttencate force-pushed the feature/builtins branch 2 times, most recently from d1e6e61 to 1930877 Compare January 17, 2023 19:59
@ttencate
Copy link
Contributor Author

I think I addressed all your comments, PTAL.

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 godot-rust#6.
Copy link
Member

@Bromeon Bromeon left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the update! Only minor things left, I can also apply them myself as there might be some more breaks introduced by my recent clippy addition. Let's see...

bors try

godot-core/src/builtin/vector_macros.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector_macros.rs Show resolved Hide resolved
godot-core/src/builtin/vector2.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector2.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector2.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector3.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector3i.rs Outdated Show resolved Hide resolved
godot-core/src/builtin/vector3i.rs Show resolved Hide resolved
bors bot added a commit that referenced this pull request Jan 20, 2023
@bors
Copy link
Contributor

bors bot commented Jan 20, 2023

try

Build failed:

@Bromeon
Copy link
Member

Bromeon commented Jan 20, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Jan 20, 2023

Build succeeded:

@bors bors bot merged commit e66ac03 into godot-rust:master Jan 20, 2023
@Bromeon
Copy link
Member

Bromeon commented Jan 20, 2023

This is a great foundation for vector types, thank you very much for this PR! 🚀

I added some minor amendments in a 2nd commit.

@ttencate
Copy link
Contributor Author

Oh, now I noticed that it's already merged. Still getting used to GitHub's code review workflow... Tiny additional PR coming up...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: core Core components feature Adds functionality to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants