Skip to content

Implementation of Plane integration tests #286

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
wants to merge 21 commits into from

Conversation

T4rmyn
Copy link
Contributor

@T4rmyn T4rmyn commented May 27, 2023

Adds integration tests to the re-implemented Plane functions (#268) and should also address the Plane section of issue #209.

@GodotRust
Copy link

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

@T4rmyn T4rmyn force-pushed the itest_plane branch 4 times, most recently from 9ba3545 to e2e7a42 Compare May 27, 2023 14:28
@T4rmyn T4rmyn marked this pull request as draft May 27, 2023 14:56
@T4rmyn T4rmyn force-pushed the itest_plane branch 2 times, most recently from 6d8d14b to 359fcbb Compare May 27, 2023 15:18
@T4rmyn
Copy link
Contributor Author

T4rmyn commented May 27, 2023

After looking at this for several hours at this point, I don't know how to make the Variant type behave so it can be compared with the builtin one properly, so I need help.

Also, I don't know how, but the inner method for the intersect_segment throws a Vector3 with all variables as NaN, not an Err type or some equivalent as I expect. Is it the correct behavior?

(Sorry about the many force pushes by the way, should probably set up running itest on my local machine)

@lilizoey
Copy link
Member

After looking at this for several hours at this point, I don't know how to make the Variant type behave so it can be compared with the builtin one properly, so I need help.

What do you mean exactly?

Also, I don't know how, but the inner method for the intersect_segment throws a Vector3 with all variables as NaN, not an Err type or some equivalent as I expect. Is it the correct behavior?

It appears that this happens if the plane or vectors contain NaN. otherwise it returns a null variant.

@T4rmyn
Copy link
Contributor Author

T4rmyn commented May 28, 2023

Ah, I see. I was mostly referring to the ways of converting the builtin function's output Option to the Inner's output of Variant. While so far I've converted both into Result for assert_eq purposes, the Err part (that is, the correct error type to compare) and the all NaN vectors is throwing me off.

@lilizoey
Copy link
Member

lilizoey commented May 28, 2023

Ah, I see. I was mostly referring to the ways of converting the builtin function's output Option to the Inner's output of Variant. While so far I've converted both into Result for assert_eq purposes, the Err part (that is, the correct error type to compare) and the all NaN vectors is throwing me off.

If a is an Option<T> where T implements ToVariant, then you can do something like a.as_ref().map(ToVariant::to_variant).unwrap_or_default() to convert it into a Variant that will be nil if a is None.

@T4rmyn T4rmyn force-pushed the itest_plane branch 3 times, most recently from 343144d to 2d3cafe Compare June 1, 2023 16:46
@T4rmyn T4rmyn marked this pull request as ready for review June 1, 2023 17:01
@T4rmyn
Copy link
Contributor Author

T4rmyn commented Jun 1, 2023

Alright, after doing the itest and fixing the normalized function in the Plane to have its d also being normalized, its ready for review.

One question though, about the INF that makes the inner function spits out the NaN vector while the builtin spits out nil. Should it be kept in the comparisons or be removed entirely?

@lilizoey
Copy link
Member

lilizoey commented Jun 1, 2023

Alright, after doing the itest and fixing the normalized function in the Plane to have its d also being normalized, its ready for review.

One question though, about the INF that makes the inner function spits out the NaN vector while the builtin spits out nil. Should it be kept in the comparisons or be removed entirely?

I would only test functionality where the functions are defined. so if it is valid for those methods to be used on planes containing INF, then keep those tests. otherwise dont include those tests.

Bromeon and others added 9 commits June 4, 2023 23:01
This commit splits SignatureTuple into two separate traits:
PtrcallSignatureTuple and VarcallSignatureTuple. The latter is a child
of the former. PtrcallSignatureTuple is used for ptrcall and only
demands GodotFuncMarshall of its arguments. VarcallSignatureTuple is
used for varcall and additionally demands ToVariant, FromVariant, and
VariantMetadata of its arguments, so pointers cannot benefit from the
optimizations provided by varcall over ptrcall.
…t-rust#240

Additionally FromVariant and ToVariant are also implemented for Option<Gd<T>>
to satisfy all the requirements for ffi and godot_api.
@T4rmyn
Copy link
Contributor Author

T4rmyn commented Jun 4, 2023

Uhh... lesson learned about force pushing after rebasing. I'll open a new pr this after fixing it.

@Bromeon
Copy link
Member

Bromeon commented Jun 6, 2023

@T4rmin for the future, you should be able to correct history by force-pushing. Not sure why you got all these commits, it looks like a separate branch? Definitely not a problem of rebasing per se.

If somehow possible, reusing the same PR would be preferred, as it's easier to administrate and for other readers to navigate. But no worries this time 🙂

bors bot added a commit that referenced this pull request Jun 17, 2023
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 <[email protected]>
bors bot added a commit that referenced this pull request Jun 17, 2023
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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants