SharpFont icon indicating copy to clipboard operation
SharpFont copied to clipboard

(FTMatrix a).Multiply(b) changes b instead of a.

Open HinTak opened this issue 9 years ago • 5 comments

I know the original FreeType API changes the 2nd argument. But "a.Mutiply(b)" changes b seems a bit surprising.

It is probably wrong to break API now, but may I propopse a change, or if that's not a good idea, how about adding a "MultipliedBy(b)" method, where the object is changed instead of the argument?

HinTak avatar Jul 08 '16 19:07 HinTak

Since the order of matrix multiplication is important, it probably should be called 'PostMulitpliedBy()' . Either way, the object interface (FTMatrix.Mulitiply(MTMatrix b) which updates the argument being used instead of the object being used, seems to be confusing... I think one should stick to the static Multiply(FTMatrix a, FTMatrix b) method...

HinTak avatar Jul 09 '16 04:07 HinTak

This was definitely an oversight when I built this API. Another thing I'm noticing now is that the static Multiply method should actually not work since matrices are being passed as value types since FTMatrix is a struct.

What I am currently planning on doing is deprecating both Multiply methods with ObsoleteAttribute and applying ref parameters to the static one and implementing MultipliedBy as you suggested.

The deprecated methods would be kept until SharpFont 4.0

Robmaister avatar Jul 11 '16 13:07 Robmaister

Oh dear :-). You are right! The static method does not work correctly! Just tried this on mono's csharp interactive shell:

Mono C# Shell, type "help;" for help

Enter statements below.
csharp> using SharpFont;
csharp> var a = new FTVector(new Fixed16Dot16(1), new Fixed16Dot16(0));
csharp> a.Rotate(new Fixed16Dot16(30));
csharp> a.X
0.86602783203125
csharp> a.Y
0.5
csharp> var b = new FTVector(new Fixed16Dot16(1), new Fixed16Dot16(0));
csharp> b.Rotate(new Fixed16Dot16(30 + 90));
csharp> b.X
-0.5
csharp> b.Y
0.86602783203125
csharp> FTMatrix mrot = new FTMatrix(a, b);
csharp> FTMatrix mrot2 = new FTMatrix(a, b);
csharp> FTMatrix mrot3 = new FTMatrix(a, b); 
csharp> FTMatrix.Multiply(mrot, mrot2)
csharp> mrot2.XX
0.86602783203125
csharp> mrot2.XY 
0.5
csharp> mrot2.YX 
-0.5
csharp> mrot2.YY 
0.86602783203125
csharp> mrot3.XX
0.86602783203125
csharp> mrot3.XY 
0.5
csharp> mrot3.YX 
-0.5
csharp> mrot3.YY 
0.86602783203125
csharp>  

HinTak avatar Jul 11 '16 14:07 HinTak

Since neither of them did work ever, there is no API break then :-).

I would suggest a PreMultipliedBy and a PostMultipliedBy, to distinguish the two cases .

HinTak avatar Jul 11 '16 14:07 HinTak

Even for broken APIs I would prefer to maintain API forward compatibility between minor and patch versions. Trying to follow SemVer properly. Just need to add a new overload and mark the old one as Obsolete and I'll delete it as part of the process for releasing the next major version.

I'm on the fence about keeping the instance methods at all or not (marking them Obsolete and only having the static methods). On one hand, it's convenient, on the other hand, it can be a bit confusing. I'll take some time to weigh the pros and cons. Since the underlying function is b = a*b, if we keep it I think only having one (PreMultiply) where b is this and a is the ref parameter would make the most sense. I wouldn't expect the ref argument of a function called PostMultiply to mutate. I would instead expect it to perform b = b*a.

Robmaister avatar Dec 08 '16 22:12 Robmaister