davideberly / GeometricTools

A collection of source code for computing in the fields of mathematics, geometry, graphics, image analysis and physics.
Boost Software License 1.0
1.08k stars 202 forks source link

A couple suggestions with the library #60

Closed ethindp closed 1 year ago

ethindp commented 1 year ago

So, this mainly goes for the mathematics library, but it might apply to others. And I have no idea how feasible these suggestions are, given how large the library is. But here they are, anyway, for pondering purposes:

  1. Replace all C-style casts with C++ casts.
  2. If possible, try to employ constraints and concepts using the Concepts library. Both of these are C++20 only, so this would jump the standard of C++ to C++20, but it would also provide significant static checking for all the templates. Right now, the concept used is the auto concept, which is akin to the old typename T template style: allow any type, even if it's obviously not a type that's compatible with the class, and let the compiler yell at you for operations that you couldn't possibly do on, say, a struct that doesn't define arithmetic operators.

As an example of the concepts, the Vector class template could be changed from:

template <int32_t N, typename Real>

to:

#include <concepts>

// ...

template <unsigned_integral N, floating_point Real>

Or if you wanted to allow vectors to work with both integral types and FP types:

template <typename N, typename E>
requires unsigned_integral<N> && (floating_point<E> || integral<E>)

This would give much cleaner error messages if someone decided to try to violate the constraints, and it would also make the purpose of the template arguments a bit clearer.

davideberly commented 1 year ago

Thank you for the feedback.

I have replaced the C-style casts by static_cast in the GTL development code.

I am not willing yet to move to C++ 17 or later because some of the code users are still on older compilers.

The "concepts" sound useful, but one of the goals with a large part of my mathematical code is to support rational arithmetic types as well as floating-point types. Your suggestion for will not allow this. In GTL, I have added my own type traits to support arbitrary precision arithmetic with division (BSRational for example) or without division (BSNumber for example). I also eliminated the unreadable std::enable_if approach to make the code somewhat easier to read. I also had tried C++ 17 for enabling/disabling of code blocks based on conditions, but that turned out to be a pain.

I have a tool for instantiating template classes and functions, designed for trapping problems with the template parameters. I run this tool before posting any code. Yes, the compiler support rather than a manual tool is better, but for now the tool suffices to meet my time constraints for library development.

A long time ago I had chosen to use a signed integral type for array/vector accesses (via operator[]), and the compilers did not complain. Now they do. In the GTL code I have tried to be consistent about using size_t, and in some classes I have added template parameters to allow the user to choose an appropriate type. Such parameters are for data rather than lookups into array/vector.

Once I finish GTL development, that will give me some breathing room to think about the issues you mention.

ethindp commented 1 year ago

Thanks, @davideberly. Would it be better for me to use GTL when using this library or GTE? (Also, one other suggestion I was going to suggest in another issue, but here works as well, is to add the marching cubes algorithm: right now you just generate the table, but the actual algorithm isn't there, and the file MarchinCubes.h is a bit misleading, as it implies that the algorithm is implemented, instead of just the LUT generation portion.)

davideberly commented 1 year ago

GTL is not yet finished, although I have some files posted here at github. I keep trying to finish GTL, but my technical support queue seems never to be empty these days. Nearly all the GTE files have been ported to GTL, but a large chunk of time is writing unit tests and end-to-end tests in a test environment that is mostly automated (compared to GTE which has a lot of manual intervention).

The file SurfaceExtractorMC.h is the implementation of the Marching Cubes algorithm. The class SurfaceExtractorMC derives from the class MarchingCubes.

The sample application GeometricTools/GTE/Samples/Imagics/ExtractLevelSurfaces illustrates MarchingCubes; see line 139 of ExtractLevelSurfacesWindow3.cpp, the function CreateMeshCubes().