Skip to content

Conversation

Cankyre
Copy link
Contributor

@Cankyre Cankyre commented May 19, 2023

Added all methods and tests to Aabb.rs in godot-builtin

@Bromeon
Copy link
Member

Bromeon commented May 19, 2023

Thanks!

A lot of suggestions from #276 are unaddressed in this pull request. Can we close that one first, with only doc aliases (no AABB features)? Otherwise we have a lot of overlap and repeated reviews. 🙂

@Cankyre Cankyre force-pushed the core/reimplementation branch 2 times, most recently from 63a2c9b to 6548821 Compare May 20, 2023 12:21
@Cankyre
Copy link
Contributor Author

Cankyre commented May 20, 2023

I think it should be good now?

@Bromeon
Copy link
Member

Bromeon commented May 24, 2023

bors try

@GodotRust
Copy link

API docs are being generated and will be shortly available at: https://godot-rust.github.io/docs/gdext/pr-280

bors bot added a commit that referenced this pull request May 24, 2023
@bors
Copy link
Contributor

bors bot commented May 24, 2023

try

Build failed:

@Bromeon
Copy link
Member

Bromeon commented May 24, 2023

@Cankyre this fails with the double builds.

To verify that it passes, you can run the tests locally using ./check.sh --double 🙂

@Bromeon Bromeon added feature Adds functionality to the library c: core Core components labels May 24, 2023
@Cankyre
Copy link
Contributor Author

Cankyre commented May 26, 2023

Should pass the tests now! 👍

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 huge implementation! 👍
I had quite an in-depth look at it and found a few more things 🙂

bors try

Comment on lines 96 to 101
/// Returns the volume of the AABB.
///
/// _Godot equivalent: `AABB.get_volume()`_
#[inline]
pub fn volume(&self) -> real {
self.assert_nonnegative();
self.size.x * self.size.y * self.size.z
}
Copy link
Member

Choose a reason for hiding this comment

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

Needs a # Panics section.

bors bot added a commit that referenced this pull request Jun 6, 2023
@bors
Copy link
Contributor

bors bot commented Jun 6, 2023

try

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@Bromeon
Copy link
Member

Bromeon commented Jun 10, 2023

bors try

@bors
Copy link
Contributor

bors bot commented Jun 10, 2023

try

Merge conflict.

@Cankyre Cankyre force-pushed the core/reimplementation branch from 2c3ded5 to 91629d6 Compare June 10, 2023 16:24
@Cankyre
Copy link
Contributor Author

Cankyre commented Jun 10, 2023

@Bromeon seems good to go!

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 patience and all the updates!

A few methods could take their parameter by borrow for consistency (I marked them), other than that good!

@Cankyre Cankyre force-pushed the core/reimplementation branch from 91629d6 to 23b3c2f Compare June 10, 2023 19:03
@Bromeon
Copy link
Member

Bromeon commented Jun 10, 2023

Thanks a lot!

bors r+

@bors
Copy link
Contributor

bors bot commented Jun 10, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 58dee73 into godot-rust:master Jun 10, 2023
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.

4 participants