Implement convex hull for unsupported loudspeaker layouts

[tomjnixon]

This is an alternative to #19

It includes a convex hull implementation; this sounds like a bad idea, but:

• Qhull has its own special license, and is a big project compared to what we need
• There are various smaller C++ libraries. Generally they are 10x larger than this implementation, as they implement a more complex algorithm than we need. These are required if you have lots of points, but they generally introduces numerical accuracy challenges. I trust that these are handled correctly by Qhull, but not by random unknown libraries.
• This has exactly the right interface, including merging of triangles into facets, which is about half the implementation...

This algorithm is really dumb, but it seems trustworthy and completes in <200us on my machine for 9+10+3.

[benjamin-weiss]

This is great. It is exactly what I had in mind. No API change but a great improvement for the library user.

You should however update the documentation. Maybe a good opportunity to fix the documentation builds? I think you can just use the docs/requirements.txt file from libadm and it should work again. The same applies to the github workflows from libadm. Copying the .github/workflows/ from libadm should be enough.

[tomjnixon]

Thanks. I'm not sure there's much to update in the documentation; the most relevant part i could find is

Although Layout objects could be constructed directly from scratch, only layouts listed in [bs2051] are supported

which is still technically true, though

only layouts listed in [bs2051] are supported although non-standard layouts may work

would be more complete.

I was thinking about moving the facets file to the tests and just using the convex hull normally. That should make the library smaller, and a bit less weird. Does that make sense?

Thanks for the pointers re. docs and CI; I've got some commits to push relating to documentation, and i'll have a look at CI tomorrow.

[benjamin-weiss]

Yes, that's the part in the documentation I meant. Would it be possible to state the conditions a layout has to fulfil that it works? If the explanation is easy enough to understand it would be a very helpful part of the documentation.

I don't really see the benefit of moving the facets file. What do you think is weird about the library? I actually think having the distinction between the officially supported layouts and the unsupported ones in the code is a good thing.

[tomjnixon]

Right, I gave the docs a go. It's not super specific, but hopefully it gets across the general idea.

re. facets, keeping them is nice because it feels like it guarantees the behavior of the renderer for standard layouts, but:

• It's dependent on the channel ordering (which should be fine, but it'll break silently if it's changed).
• The triangulation is not actually static; we're actually missing this call, which modifies the nominal positions and therefore the triangulation.
• It's dependent on the layout name in ways that the ear reference is not (so if channels were added but the layout name was not updated it would fail silently).

I also like the idea of a distinction between the officially supported layouts and the unsupported ones, but I don't think the facets file really helps with that -- I'd much rather have an explicit check that the passed-in layouts meet bs.2051 requirements (with a warning if that fails), for example.

[benjamin-weiss]

Documentation looks good to me.

Regarding the facets file: All three of your arguments are good reasons to move it. So let's do it. Also your proposed alternative with checking the layout requirements is a good one. Maybe that could be a two step process: first check if the layout is a bs.2051 layout and then check if it anyways meets the requirements. The result of this could also be made available to the library user. I think it could be helpful to be able to check if the renderer is running in the standardised bs.2127/bs.2051 mode or not.

[codecov-io]

Codecov Report

Merging #20 (c1954a7) into master (b7b5c26) will increase coverage by 0.26%.
The diff coverage is 95.77%.

@@            Coverage Diff             @@
##           master      #20      +/-   ##
==========================================
+ Coverage   85.32%   85.58%   +0.26%     
==========================================
  Files          53       54       +1     
  Lines        2044     2102      +58     
  Branches      167      182      +15     
==========================================
+ Hits         1744     1799      +55     
  Misses        223      223              
- Partials       77       80       +3     

Impacted Files	Coverage Δ
src/common/point_source_panner.hpp	88.88% <ø> (ø)
src/common/convex_hull.cpp	93.75% <93.75%> (ø)
src/common/point_source_panner.cpp	90.47% <100.00%> (+0.29%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b7b5c26...c1954a7. Read the comment docs.

[tomjnixon]

Thanks, that all makes sense. I'll merge this (since it implements the required functionality and doesn't break anything) and add issues for the other bits mentioned.