Freedom-of-Form-Foundation / anatomy3d

A CAD tool for humanoid anatomy alterations. See the anatomy3d-blender repository for more recent work.
https://freedomofform.org/1856/3d-anatomy-project-scope-phase-1-focus-on-a-limb-joint/
GNU General Public License v2.0
7 stars 5 forks source link

Refactor: ContinuousMap -> delegate #10

Closed AdamNorberg closed 2 years ago

AdamNorberg commented 3 years ago

The current ContinuousMap type is too generic: it's a general function pointer wrapped in an object. It is functionally equivalent to Func<I, O>. C# already has two ways to express this: delegate O TypeName(I) and Func<I, O> if the type itself does not need a separate name because it does not represent a single coherent concept.

Refactor to Func<I, O> or delegate O TypeName(I), depending on which seems more appropriate in context, with suitable concerete types (currently float, but see also issue #9).

AdamNorberg commented 3 years ago

More information on delegates: https://docs.microsoft.com/en-us/dotnet/csharp/programming-guide/delegates/

More information on Func<T, T2, T3, T4... TReturn>: https://docs.microsoft.com/en-us/dotnet/api/system.func-2 and the associated types from 0 to 16 Ts visible on the left side. The related types Action<T, T2, T3...> are void functions. In practice, these are just predeclared delegates for when it doesn't make sense to give a name to a specific delegate type (which is pretty common; when a function takes a function as an argument, it can just be idiosyncratic rather than something that will be used in many places in the code).

Lathreas commented 3 years ago

The current implementation of ContinuousMap indeed looks very generic (since it's just the first iteration of the code), but it seems its intended use is more specialized than Func<T, TReturn> or delegate. For example, the branch I am about to push expands ContinuousMap with a SolveRayTrace(...) function, which is required for the engine to be able to raytrace arbitrary surfaces and functions. This issue might give a bit more insight into the intended use of ContinuousMap.

The intended limitations are the following: ContinuousMap should only ever take numeric or vector types as input or output (such as float, double, Complex, Vector3, matrices etc...), and it should be a continuous function such that it is raytraceable.

What we can do to limit the current code's overly general representation is to constrain I and O using the where keyword to only allow structs, or even constrain them to a list of numeric types we explicitly allow. Preferably we would constrain I and O to some interface (named INumeric, or IScalarMultiplicable? We need a good name for this.). This type should then be implemented by all allowed types. Unfortunately that would require extending the definition of C# standard library classes such as Complex or Decimal to implement this shared interface, so we need to find some way around that. If we go that route, we may need to (re)implement a Real class, Complex class, Vector2 class, Vector3 class and Vector4 class if not more, so this is something we need to look at.

The second property of a ContinuousMap is that it should be a continuous mapping from one space to another (such as a smooth function), meaning it is raytraceable. This would be made explicit in the soon-to-be-pushed branch with the SolveRayTrace method, but I might still need to refactor that.

AdamNorberg commented 3 years ago

The way ContinuousMap<float, float> is used for a dynamic bone radius seems really distinct from its ContinuousMap<Vector2, ???> representation as a computed effective height map - I don't think it helps for these to be the same generic type. IRaytraceable, IDenseFunc<T> , where IDenseFunc<T> represents something that takes a floating-point value (probably Real) between 0 and 1 (inclusive) and emits a value of type T for every value in that range? That covers the radius use.

Lathreas commented 3 years ago

I see what you mean, and it might indeed make sense to split up ContinuousMap in some way with respect to their input or output type. There are some arguments against this as well, but to fully explore those I suppose it's better to wait until I have published the large patch that comes soon™ so that we can fully appreciate the benefits and drawbacks of either representation. That said, I do like the idea of an IRaytraceable type being introduced as well, as it would separate based on function more.