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

Initial tests for geometry3D #44974

Merged
merged 1 commit into from
Feb 27, 2021
Merged

Initial tests for geometry3D #44974

merged 1 commit into from
Feb 27, 2021

Conversation

Gorgonx7
Copy link
Contributor

@Gorgonx7 Gorgonx7 commented Jan 6, 2021 •
edited
Loading

This is part of issue #43440.
These are a a list of test cases that layout the functions to be tested within geometry 3D

@Calinou Calinou added this to the 4.0 milestone Jan 6, 2021
@Calinou Calinou mentioned this pull request Jan 6, 2021
Open
99 tasks
@Calinou
Copy link
Member

Calinou commented Feb 20, 2021 •
edited
Loading

Thanks for opening a pull request!

Note that we don't use std containers in Godot's codebase. Instead, we have our own container types which should be used instead.

@Gorgonx7
Copy link
Contributor Author

Yep just spotted that, most things are tested now or at least structured in a way that it's easy to add more cases, gonna make sure I clean this up before I mark it ready for review :)

@Gorgonx7 Gorgonx7 force-pushed the master branch 5 times, most recently from 339772d to 3a826fb Compare February 21, 2021 19:26
@Gorgonx7 Gorgonx7 marked this pull request as ready for review February 21, 2021 19:42
@Gorgonx7 Gorgonx7 requested a review from a team as a code owner February 21, 2021 19:42
@Gorgonx7
Copy link
Contributor Author

Marked as ready for review now. Happy to make changes as needed, almost everything is tested, or structured so that test cases can be added easily :)

Xrayez
Xrayez reviewed Feb 22, 2021
View reviewed changes
@Gorgonx7 Gorgonx7 force-pushed the master branch 3 times, most recently from 08b8729 to 16c5c67 Compare February 25, 2021 22:19
@Gorgonx7
Copy link
Contributor Author

@Xrayez I think I addressed all your comments, let me know if there are any additional actions :) happy to address

@Xrayez
Copy link
Contributor

Xrayez commented Feb 26, 2021

Tests look good to me but I'm still concerned with the usage of camelCase or PascalCase throughout tests. Again, I've just noticed Mesh name is used as a variable name, but Godot has a Mesh class. No unit tests are written for the Mesh class currently, but in the future this may cause compilation errors for those who'd like to write tests for the Mesh class.

Note that test headers are currently compiled in a single translation unit.

I presume you're coming from C#?

@akien-mga should confirm that Godot follows snake_case convention for variable names (so does GDScript), otherwise the current approach is error-prone considering the fact that Godot rarely uses namespaces for core classes.

Even if this won't introduce further problems, it makes sense to follow existing style conventions in any case.

Xrayez
Xrayez reviewed Feb 26, 2021
View reviewed changes
Xrayez
Xrayez reviewed Feb 26, 2021
View reviewed changes
@akien-mga
Copy link
Member

Yes, identifiers should use snake_case. PascalCase is used for class names, SHOUTING_SNAKE_CASE for constants, and camelCase is never used (in C++).

@Gorgonx7
Copy link
Contributor Author

Ahh that's annoying, yes I'm coming from C#, I'll fix the rest up tonight/tomorrow

@Gorgonx7 Gorgonx7 force-pushed the master branch 2 times, most recently from a8f2d5e to 01ddbf7 Compare February 27, 2021 13:10
@akien-mga akien-mga merged commit 213ea17 into godotengine:master Feb 27, 2021
@akien-mga
Copy link
Member

Thanks!

aaronfranke
aaronfranke reviewed Aug 23, 2021
View reviewed changes
tests/test_geometry_3d.h
Comment on lines +148 to +168
TEST_CASE("[Geometry3D] Build Convex Mesh") {
struct Case {
Vector<Plane> object;
int want_faces, want_edges, want_vertices;
Case(){};
Case(Vector<Plane> p_object, int p_want_faces, int p_want_edges, int p_want_vertices) :
object(p_object), want_faces(p_want_faces), want_edges(p_want_edges), want_vertices(p_want_vertices){};
};
Vector<Case> tt;
tt.push_back(Case(Geometry3D::build_box_planes(Vector3(5, 10, 5)), 6, 12, 8));
tt.push_back(Case(Geometry3D::build_capsule_planes(5, 5, 20, 20, Vector3::Axis()), 820, 7603, 6243));
tt.push_back(Case(Geometry3D::build_cylinder_planes(5, 5, 20, Vector3::Axis()), 22, 100, 80));
tt.push_back(Case(Geometry3D::build_sphere_planes(5, 5, 20), 220, 1011, 522));
for (int i = 0; i < tt.size(); ++i) {
Case current_case = tt[i];
Geometry3D::MeshData mesh = Geometry3D::build_convex_mesh(current_case.object);
CHECK(mesh.faces.size() == current_case.want_faces);
CHECK(mesh.edges.size() == current_case.want_edges);
CHECK(mesh.vertices.size() == current_case.want_vertices);
}
}
Copy link
Member

@aaronfranke aaronfranke Aug 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@Gorgonx7 Can you help me debug an issue with this? The problem I'm having is that this test case seems to be very fragile. Any slight change causes the test to fail.

In #37842, the constructor for Plane with normal and point had its arguments swapped, so we took the opportunity to improve the code in Geometry3D::build_sphere_planes by just passing in the radius distance directly instead of constructing using a point and normal (which the constructor just turns back into a distance).

Here are some examples of the planes being passed. Before:

[N: (0.791327, -0.257118, 0.554700), D: 5.000001]
[N: (0.527551, -0.171412, 0.832050), D: 5.000001]
[N: (0.230665, -0.074948, 0.970142), D: 4.999999]

After:

[N: (0.791327, -0.257118, 0.554700), D: 5]
[N: (0.527551, -0.171412, 0.832050), D: 5]
[N: (0.230665, -0.074948, 0.970142), D: 5]

The after output is slightly more precise, but it breaks the test case code:

tests/test_geometry_3d.h:154:
TEST CASE:  [Geometry3D] Build Convex Mesh

tests/test_geometry_3d.h:171: ERROR: CHECK( mesh.edges.size() == current_case.want_edges ) is NOT correct!
  values: CHECK( 1009 == 1011 )

This same test case also fails when Godot is compiled with doubles. At this point I'm entirely convinced that this just a bad / fragile test case, hopefully you have an idea on how to make it better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aaronfranke sorry I never got back to you, things have been a bit hectic at work, so hectic I've not had much luck working on open source at the moment, did you ever figure this out?

Copy link
Member

Choose a reason for hiding this comment

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

@Gorgonx7 No, currently the test is just disabled in master.

@kleonc kleonc mentioned this pull request Jun 30, 2023
Open
@aaronfranke aaronfranke mentioned this pull request Sep 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronfranke aaronfranke aaronfranke left review comments

@Xrayez Xrayez Xrayez approved these changes

Assignees
No one assigned
Labels
enhancement topic:tests
Projects
None yet
Milestone
4.0
Development

Successfully merging this pull request may close these issues.
5 participants
@Gorgonx7 @Calinou @Xrayez @akien-mga @aaronfranke