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.

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

Calculate volume of the current group / surface area of selected faces #449

Closed
Evil-Spirit opened this issue Jul 11, 2019 · 14 comments
Closed

Comments

@Evil-Spirit
Copy link
Collaborator

Evil-Spirit commented Jul 11, 2019

This can be useful (I use this to calculate concrete).
Evil-Spirit@ef85e9e

@phkahler
Copy link
Member

It may be a stretch, but could these values be used in the solver? I saw someone using a different CAD use a volume constraint to set the height of a container. I doubt that's really feasible here, just wanted to share the thought.

@whitequark
Copy link
Contributor

whitequark commented Jul 11, 2019

The function of a volume from dimensions is not in general analytical, is it?

@ruevs
Copy link
Member

ruevs commented Jul 11, 2019

That's an interesting idea. But apart from the Volume(dimensions) function not being analytical, the volume is calculated on the triangle mesh - which is certain to cause "noise" in the value.

@phkahler
Copy link
Member

@whitequark That was a dumb question from me. Of course volume is computed numerically from the shell/mesh. While there may (theoretically) be a way to twiddle free parameters to get numeric derivatives, that isn't a thing in SS.

@ruevs Good point. A simple triangle mesh shouldn't be a problem but when the number of triangles changes there would be a discontinuity in the calculated volume.

@phkahler
Copy link
Member

Do the triangles have a consistent ordering? Meaning ABC is always CCW or CW when viewed from the outside? If so, the volume can be calculated by summing a.Cross(b).Dot(c)/6 for all the triangles. That would be a lot less code.

@Evil-Spirit
Copy link
Collaborator Author

Evil-Spirit commented Jul 13, 2019

@phkahler look at the STriangle::SignedVolume used in GetCenterOfMass(). Code of @jwesthues leaved intentionally since I have no ability to test new volume calculation funtions.

@Evil-Spirit
Copy link
Collaborator Author

BTW can someone explain me why I wrote div by 4 instead of div by 3 here:
https://github.com/solvespace/solvespace/blob/master/src/mesh.cpp#L352
I can't remember why, so this is probably a typo (since 3 and 4 close to each other).

@phkahler
Copy link
Member

@Evil-Spirit The division by 4 is because you're calculating the centre of a tetrahedron formed by the 3 triangle verticies and the origin. No need to add in the vector (0,0,0) but you still need to divide by 4 since there really are 4.

So to my point, you can just sum the STriangle::SignedVolume() of each triangle to get the total volume. Its implementation is exactly what I indicated in the previous comment.

This is explained in the following stack overflow question:
general-formula-to-calculate-polyhedron-volume

@Evil-Spirit
Copy link
Collaborator Author

So to my point, you can just sum the STriangle::SignedVolume() of each triangle to get the total volume.
Its implementation is exactly what I indicated in the previous comment.

Yes this will be good. You are welcome to contribute!

@whitequark whitequark added this to the 3.0 milestone Jul 31, 2019
@phkahler
Copy link
Member

@whitequark Does anything else need to be done for this to get in?

@whitequark
Copy link
Contributor

@phkahler Yes--SMesh::CalculateSurfaceArea shouldn't be a function on the mesh. As usual with commits that combine random refactoring that wasn't even necessary with adding a feature, it takes a while to review and merge.

@Evil-Spirit
Copy link
Collaborator Author

Evil-Spirit commented Sep 11, 2019

@whitequark please, write more arguments why SMesh::CalculateSurfaceArea should be implemented in different way.
@phkahler it needs to be merged by hands because I used an old branch where some fixes about dimensions was not made. Sorry, I have no to time to rebase.

@whitequark
Copy link
Contributor

please, write more arguments why SMesh::CalculateSurfaceArea should be implemented in different way.

Sorry, I was wrong because I misread the code. I'll merge it soon.

@whitequark
Copy link
Contributor

@Evil-Spirit Thank you for this contribution. Merged as 915f55a and 7f9117b.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants