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.

Sign up for GitHub

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

Jump to bottom

Reimplement Rect2, Rect2i, Aabb, and Plane in rust #209

Closed
4 tasks done
lilizoey opened this issue Mar 29, 2023 · 2 comments · Fixed by #242
Closed
4 tasks done

Reimplement Rect2, Rect2i, Aabb, and Plane in rust #209

lilizoey opened this issue Mar 29, 2023 · 2 comments · Fixed by #242
Labels
c: core Core components good first issue Good for newcomers quality-of-life No new functionality, but improves ergonomics/internals

Comments

@lilizoey
Copy link
Member

lilizoey commented Mar 29, 2023 •
edited by Bromeon

Currently, Rect2, Rect2i, Aabb, and Plane have only a very simple implementation, requiring users to use InnerX to call most methods on them. We should reimplement these in rust, along the lines of the other mathematical types.

Have a look at for instance Vector2 and Basis to see what this should look like.

Another good source for inspiration is our gdnative bindings, they can be found here: Rect2, Aabb, Plane

In addition unit tests and integration tests would be needed.

For unit tests it can be useful to look at godot's unit tests for the same types, they can be found here.

For integration tests, basis_test.rs is a good example to go by, something like basis_equiv is a minimum to ensure our methods behave similarly to godot.

Finally, for writing documentation and ensuring you're covering all methods we want, look at the godot docs for the various types: Rect2, Rect2i, Aabb and Plane.

Not everything has to be done at once.

Reimplemented so far:
@Bromeon Bromeon added quality-of-life No new functionality, but improves ergonomics/internals c: core Core components good first issue Good for newcomers labels Mar 29, 2023
@yannick-was-taken yannick-was-taken mentioned this issue Apr 7, 2023
Merged
bors bot added a commit that referenced this issue Apr 12, 2023
@bors @yannick-was-taken
Merge #218
21401a1 
218: godot-core: builtin: reimplement Rect2i to reduce need of inner usage r=Bromeon a=yannick-was-taken

Addresses part of #209 .

Only doing `Rect2i` for now, so that suggestions only have to be applied once, instead of four times ;)

The implementations are tested (regular case + edge case where applicable) including an itest that checks equivalence between the inner type's fns and the implemented fns.

Documentation is mostly taken from Godot's docs for Rect2i, with little changes here and there.

There are a bunch of FIXMEs that need to be resolved before this is mergable: Godot's rect2i.h prints errors in some cases when the rect size is negative. In accordance with what I believe to be a design principle of gdext, the equivalent fns panic in that case (compare to `cast`/`try_cast`: default is to assume and panic?).

There could be `_unchecked` fns that likely should be marked unsafe, as though they would not introduce memory unsafety, they would result in "Garbage In / Garbage Out", which in Rust usually is considered unsafe, as the safe bet would be to prevent the Garbage In situation via typing, or at least make it return `Option<Garbage Out>`. The Option approach could be used here as well, but would change the type's API as opposed to what it is in GdScript.

So please provide input on those FIXMEs as to what you think should be done here.

Co-authored-by: yannick-was-taken <yannick-was-taken@posteo.de>
This was referenced Apr 13, 2023
Closed
Open
@juliohq juliohq mentioned this issue Apr 23, 2023
Merged
@T4rmin T4rmin mentioned this issue May 9, 2023
Merged
bors bot added a commit that referenced this issue May 24, 2023
@bors @T4rmin
Merge #268
bc24100 
268: godot-core: builtin: reimplement Plane functions/methods r=Bromeon a=T4rmin

Partly addresses issue #209 by re-implementing `Plane`'s functions/methods.

I know there's a need to implement test cases for it by looking at the `Rect2i` commits, but I would like to know if this part is correct yet since I am an still an absolute beginner at this stuff. Please let me know of anything that is an issue.

Also, would implementing test cases for `Plane` be similar in implementation as the `Rect2i` ones?

Co-authored-by: Aaron Lawrence <t4rmin@protonmail.com>
@T4rmin T4rmin mentioned this issue May 27, 2023
Closed
@T4rmin T4rmin mentioned this issue Jun 4, 2023
Merged
@Cankyre
Copy link
Contributor

Cankyre commented Jun 13, 2023

Aabb is done, you can mark it as closed. #280

@Bromeon Bromeon linked a pull request Jun 13, 2023 that will close this issue
Reimplement Rect2 functions #242
Merged
@Bromeon
Copy link
Member

Bromeon commented Jun 13, 2023

Will close automatically when #242 is merged.

@lilizoey lilizoey mentioned this issue Jun 14, 2023
Open
30 tasks
bors bot added a commit that referenced this issue Jun 17, 2023
@bors @T4rmin
Merge #295
2398bf6 
295: Implementation of Plane integration tests r=Bromeon a=T4rmin

Adds integration tests to the re-implemented Plane functions (#268, and fixing a function in it) and should also address the Plane section of issue #209.

This PR is also a recreation of #286 since it has the great error of force-pushing after rebasing.


Co-authored-by: Aaron Lawrence <t4rmin@protonmail.com>
bors bot added a commit that referenced this issue Jun 17, 2023
@bors @T4rmin
Merge #295
6608393 
295: Implementation of Plane integration tests r=Bromeon a=T4rmin

Adds integration tests to the re-implemented Plane functions (#268, and fixing a function in it) and should also address the Plane section of issue #209.

This PR is also a recreation of #286 since it has the great error of force-pushing after rebasing.


Co-authored-by: Aaron Lawrence <t4rmin@protonmail.com>
bors bot added a commit that referenced this issue Jun 17, 2023
@bors @juliohq @Bromeon
Merge #242
594e6b4 
242: Reimplement Rect2 functions r=Bromeon a=juliohq

Reimplement Rect2 functions as proposed by #209. It's missing to reimplement `is_infinite()` somehow, since it needs an `inf` type.

Co-authored-by: juliohq <juliohenrique31501234@gmail.com>
Co-authored-by: Jan Haller <bromeon@gmail.com>
bors bot added a commit that referenced this issue Jun 17, 2023
@bors @juliohq @Bromeon
Merge #242
08ea421 
242: Reimplement Rect2 functions r=Bromeon a=juliohq

Reimplement Rect2 functions as proposed by #209. It's missing to reimplement `is_infinite()` somehow, since it needs an `inf` type.

Co-authored-by: juliohq <juliohenrique31501234@gmail.com>
Co-authored-by: Jan Haller <bromeon@gmail.com>
@bors bors bot closed this as completed in #242 Jun 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
c: core Core components good first issue Good for newcomers quality-of-life No new functionality, but improves ergonomics/internals
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Reimplement Rect2 functions juliohq/gdext
3 participants
@Bromeon @lilizoey @Cankyre