-
Notifications
You must be signed in to change notification settings - Fork 0
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
added test function MeshClosestPoint #1
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Sophielelerre!
I started to look at the code. Some comments are inline, here some general comments
- Docstring
Just type///
3 times on top of a method and Visual studio (VS) will create a default structure to fill. - Comments should always have a space after
//
(so you can distinguish between comments and commented code:
//Console.Writeline("ciao") // is a commented code
// This is a comment
- Variables names
UsecamelCase
and notPascalCase
Nice to see that the project is getting shaped! :)
Speak tomorrow,
-g
2) Added a loop in the CloudClosestPoint instead of the IndexOf(Min()) method 3) Added an if statement to distiguish triangular faces and/or quad faces in the Mesh class
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Sophie!
I've looked at the code and made suggestions on how to simplify and make the code more performative.
Try to copy-paste/implement what I did and let me know if you have questions. Pay attention on the fact that MeshVertex
is a child class of Point3
. So you don't need to make a conversion (allocating more memory on large meshes) but with minor tweaks at the input type of methods (using IEnumerable over List) the code is getting simplified a lot.
I'm still not convinced about the quad solution. We make 4 triangles (permutations of all possible triangles with 4 points) because we don't know which one of the two is actually the solution that is closer to the real face geometry. But we don't know at all the face geometry! So the point we find could be either ok or either wrong.
For sure if the face is planar our closest point is good BUT we do the calculation on 4 triangles when we could do just with 2.
We could threat the quad and ngons at the same way: finding the average point and then triangulate.
OR
we could check with the same strategy if the point is inside a generic flat polygon and raise an error if a mesh face is not planar. Unsure.
Sorry for this attack late Friday!
Feel free to just look at it on Monday!
- from lists to arrays - code refinements - addition of polygon option (fan strategy)
Ngon doesn't get detected
Main things to notice:
|
…3 class) - added Ngon implementation (stellate method) (Notice: the vertices of the Ngon are being converted to Point3. Can't index a IEnumerable instead of a list)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @Sophielelerre.
Great work. I left few comments, mainly regarding formatting the code. Nothing big to change.
After going through my comments, the only thing left is to fix the docstings. Let's chat about this.
src/GShark/Geometry/Mesh.cs
Outdated
{ | ||
//throw new Exception("Ngon detected"); | ||
// Stellate method (from Ngon to triangles) | ||
List<Point3> GSVerticesPoints = GSVertices.ConvertAll(v => (Point3)v); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before: no need to convert. Let's not convert and then see what G-Shark people think about it.
Added Docstrings for all methods. Quad analysed as Ngon. Cloud closest point now static method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great job!
Just a minor suggestion into a docstring.
Can you please fill in the pull request description? The template you see is from G-Shark people. Make an exaustive description and reference the g.shark issue I've created (its in g-shark repository). When this is done and reviewed you can open the PR that will ask to merge what you did into the real G-Shark repo.
Hurrà!
Co-authored-by: iltabe <64929000+iltabe@users.noreply.github.com>
What type of PR is this? (check all applicable)
Description
This PR adds a ClosestPoint() implementation for meshes. The method takes a mesh and a test point and returns the closest point to the test point on the mesh. The algorithm finds the closest point to all the faces, then the absolute closest. This point can be either on a mesh vertex, mesh edge, or inside a mesh face. The method can be used with triangle meshes, quad meshes, and Ngon meshes. Any Ngons faces are triangulated using vertices' centroid and consecutive vertices. Quads are treated as Ngons. I did not work on the tests yet, I'd like some feedback first.
Related Tickets & Documents
Please use this format link issue numbers: Fixes GSharker#416
GSharker#416 (comment)
Added tests?
Added to documentation?