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

Add support for multiple hulls #18

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bluthej
Copy link

@bluthej bluthej commented Dec 7, 2024

As mentioned in the UK Tricontours example:

The triangulation does not need to be Delaunay, nor to be convex, for tricontours to work. It must however be properly defined, and have only one hull. (If it has disconnected pieces, you will need to run the tricontours for each piece, or upgrade the algorithm and data structure.)

This PR is a proposed implementation of the upgrade mentioned in the case where there are multiple hulls. This is useful for two different cases: when the domain is a single connected component but is multiply connected (i.e. it's in one piece but it has holes), and/or when the domain is made up of several connected components.

This upgrade required very few changes in the code and is backwards compatible.

I saw there is a test folder in the repo, but I don't know how to run the test suite, so I did not add a test. If that is something you would like me to add, I'd be happy to exchange on how you test this repo. For my own purposes, the new implementation works correctly, even in cases where there are several connected components and each has several holes.

The input hull from the triangulation can be either a single array for a
single hull, or an array of arrays for several hulls.
This makes it possible to handle the cases where the region is made up
of multiply connected components or has holes (multiply connected).
@Fil
Copy link
Owner

Fil commented Dec 7, 2024

Nice! To run the tests simply run yarn; yarn test.

Please also update the documentation.

@bluthej
Copy link
Author

bluthej commented Dec 8, 2024

Thanks for the pointer, all the tests do pass :)

I'd be happy to update the docs, could you just specify where that would go? Did you mean in the README or something else?

@Fil
Copy link
Owner

Fil commented Dec 9, 2024

Re: documentation, yes I was thinking the README maybe if there is a good place for it, and also maybe as a stand-alone example (notebook?)

@bluthej
Copy link
Author

bluthej commented Dec 10, 2024

Ok I think I know where to put it in the README.

An example notebook would indeed be nice to showcase the functionality. I assume you meant something like the examples you have here right? Should I then add an example to your collection? I'm new to observable so I'm not sure what the process is.

@Fil
Copy link
Owner

Fil commented Dec 11, 2024

yes; if you can create the example (anywhere convenient for you), I will then set it up properly

@bluthej
Copy link
Author

bluthej commented Dec 23, 2024

@Fil I just added the documentation and I wrote an example that you can find here. I made this public so you should be able to access the example, but if you are having any trouble just let me know.

I adapted the existing examples on re-using a triangulation but made a couple of tweaks that are necessary in the case of multiple hulls (in particular the triangulate function is a bit different to accommodate the multiple hulls). Also, I took that opportunity to fix a small thing in the display, there was one edge missing on each hull, and that's also the case on the UK Tricontours example (it's just not very noticeable because the edges are small). It was a matter of appending the first node at the end to close the contour :)

Let me know if that looks good to you! And happy holidays :)

@Fil
Copy link
Owner

Fil commented Dec 26, 2024

It's working well, but do you think it would be possible to accept a GeoJSON (Polygon, MultiPolygon) rather than an "array of arrays of indices"? It would be closer to standards. One difficulty is that we'd need to ensure that the (x,y) coordinates are matched to the internal numbering of vertices.

@bluthej
Copy link
Author

bluthej commented Dec 26, 2024

I can certainly look into that, could you point me to the GeoJSON documentation so that I can look up how we could go about accepting that as input?

@Fil
Copy link
Owner

Fil commented Jan 6, 2025

The GeoJSON specification includes Polygon and MultiPolygon type geometries. Basically the difference with your code is that the coordinates are given as pairs of numbers (x, y), not as indices. So there would be a bit of rematching to do.

@bluthej
Copy link
Author

bluthej commented Jan 25, 2025

So if I understand correctly, you would like the hull or hulls to be passed in as a Polygon or MultiPolygon, right?

The thing is we are currently using hull to determine whether a given triangle vertex is on the hull or not, and it's easier and cheaper to be comparing indices rather than coordinates. If we pass in a Polygon for instance, which stores the coordinates of the points but not the indices, then we need to compare the coordinates of the triangle vertices with those of the points on the hull. This will require doing the comparison with a certain tolerance, and will be much less performant.

Also, we still need the indices to access the values which are used as a stopping criterion.

And there is also the fact that this would be a breaking change, I was trying to be backwards compatible but maybe that's something you don't mind?

What do you think? Feel free to let me know if I misunderstood what you were suggesting.

@Fil Fil mentioned this pull request Feb 1, 2025
@Fil
Copy link
Owner

Fil commented Feb 1, 2025

Yes. My suggestion to go back to the geometries (points) would only have added more work and indirection. However, I rewrote the algorithm to use the triangulation (halfedges and triangles) rather than the hull, which simplifies things quite a bit internally as well as when you set up your triangulation. See #22.

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.

2 participants