mathnet-spatial icon indicating copy to clipboard operation
mathnet-spatial copied to clipboard

CoordinateSystem Transformation Methods Behaving Unexpected

Open Sockenfresser opened this issue 6 years ago • 5 comments

The methods Transform, TransformToCoordSys and TransformFromCoordSys of the CoordinateSystem class are behaving unexpected and are hard to understand. Transform() seems to transform a point from the CoordinateSystem to the base system. This is not visible from either the method name or the comment ("Transforms a point and returns the transformed point").

The TransformFrom and TransformTo methods seem to do a proper transform but add the offset afterwards, which in my opinion does not make much sense. But maybe I'm missing something.

Unfortunately there are no unit tests explaining the expected behaviour of these methods.

We are willing to contribute but at first we have to understand what is expected from the methods and if a change in interface or behaviour is acceptable.

Sockenfresser avatar Jul 24 '17 07:07 Sockenfresser

I'm sorry about this, the whole API for coordinate systems is not very nice. I should probably not have included it but the damage is done. What I think we should do is:

  1. Figure out a new API and mark the old as [Obsolete] where needed.
  2. Rewrite coordinate system so that it does not wrap a matrix as it is now. I'm not sure if it should be a struct as that would mean it would be a pretty big struct. Struct makes some sense as we want value semantics but we can get that with a class also.

JohanLarsson avatar Jul 24 '17 09:07 JohanLarsson

I don't have a problem with the underlying matrix. Maybe it shouldn't be inherited but aggregated. But it is pretty hard to mark the inheritance as [Obsolete] I guess.

The use of the matrix itself provides a lot of useful stuff already implemented in MathNet.Numerics.

I doubt that it makes sense to reimplement all this. The alternative would be, to create a Matrix<T> every time we want to make a transform or calculation. Maybe the whole invariant struct scheme does not apply to the CoordianteSystem. In contrast to points or vectors I don't expect to have thousands of CoordinateSystems. But this might depend on the domain you are in.

What I need for my project, is a way to create a CoordinateSystem for translations, rotations and mirroring and combinations of these tranformations. And we need matrix multiplication, matrix inversion and point transformations from and to the vector space of the coordinate system.

Sockenfresser avatar Jul 24 '17 12:07 Sockenfresser

@Sockenfresser can you please check the current state of the library? At the time of writing the CoordinateSystem type has been refactored and you should see an effect on its performance.

It no longer derives from DenseMatrix, as it shouldn't. It's a very special 3x3 matrix, and we don't need to carry the whole burden of a complete linear algebra package for it. It's inverse for example is its transpose and so on. I also added some more unit tests for better test coverage.

Let me know what you think

jkalias avatar Apr 13 '23 12:04 jkalias

Unfortunately I'm not working for the same company and with the library any more. I will let my former colleagues know about the change.

Sockenfresser avatar Apr 25 '23 18:04 Sockenfresser

Unfortunately the changes had to be reverted due to community backlash

jkalias avatar Apr 26 '23 04:04 jkalias