Skip to content
This repository has been archived by the owner on Nov 22, 2023. It is now read-only.

add area and volume calculations #144

Merged
merged 9 commits into from
Oct 17, 2019
Merged

add area and volume calculations #144

merged 9 commits into from
Oct 17, 2019

Conversation

BeastyBlacksmith
Copy link
Contributor

@BeastyBlacksmith BeastyBlacksmith commented Aug 14, 2018

Implement calculation of area and volume
This is one of my first PRs so I'm not sure, if it meets the specifics of this package entirely.
The area function may or may not be a substitute for

function area(contour::AbstractVector{Point{N, T}}) where {N, T}
, which is either wrong or I didn't fully understand its purpose.

@CarpeNecopinum
Copy link
Contributor

It's not a replacement for the area method of polygons.jl (which e.g. handles non-convex polygons without having to triangulate them first). But what you're suggesting looks like a valuable addition to me.

Adding that gts parser and a relatively large mesh with it for testing is a bit overkill though, imho. I would suggest to just build a small mesh (e.g. a cube, has the advantage that it's obvious how large the volume/area should be) directly in the code and use that for testing.

@SimonDanisch
Copy link
Member

Sorry for completely ignoring this! @CarpeNecopinum probably has a better idea ;) I basically just copy & pasted the area code for a single purpose usage -.-

@BeastyBlacksmith
Copy link
Contributor Author

how do i get a properly oriented cube?
I tried

    cube = HyperRectangle(Vec3f0(-0.5), Vec3f0(1))
    cube_faces = decompose(Face{3,Int32}, cube)
    cube_vertices = decompose(Point{3,Float32}, cube) |> Array

but the volume computes to zero...

@BeastyBlacksmith
Copy link
Contributor Author

why do I get the comments, if I rebase?

@CarpeNecopinum
Copy link
Contributor

When you rebase against master, it makes new commits that have the same messages, but different hashes. You should have merged master instead.

Happened to me earlier today, too. 😅

@BeastyBlacksmith
Copy link
Contributor Author

that indeed does look better

@CarpeNecopinum
Copy link
Contributor

I just found out that the faces of a 3d HyperRectangle are inconsistently oriented, so you getting a volume of zero is actually the "correct" result (i.e. your algorithm does nothing wrong here).

You should be able to test your algorithm with the branch in #183, there I fixed the orientation so all faces point outwards (with counter-clockwise winding order).

@BeastyBlacksmith BeastyBlacksmith changed the title WIP: add area and volume calculations add area and volume calculations Oct 17, 2019
@BeastyBlacksmith
Copy link
Contributor Author

Thanks! This is fine now

@SimonDanisch
Copy link
Member

Is this mergable then?

b = v3 - v1
cross(a, b)
end

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't call this normal, as the actual normal is the normalized version of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

too much of a pun, if I name it unnormal?

Copy link
Contributor

Choose a reason for hiding this comment

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

edge_cross_product maybe? Personally I'd probably just keep it as a local function inside the other ones. Even though you'd have the same code twice then...

Copy link
Contributor

Choose a reason for hiding this comment

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

Re: Silly names. What about normal_times_twice_the_area? 🤓

Copy link
Member

Choose a reason for hiding this comment

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

maybe something like orthogonal_vector?

@BeastyBlacksmith
Copy link
Contributor Author

orthogonal_vector then. This is ready to merge.

@SimonDanisch SimonDanisch merged commit f956c0c into JuliaGeometry:master Oct 17, 2019
@SimonDanisch
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

3 participants