arklumpus/VectSharp

Inconsistent Graphics.Transform API

gmkado opened this issue · 6 comments

gmkado commented

First off, thanks for the great library. I find it much simpler and nicer to work with than other svg libraries. For the most part the interface and documentation is very straightforward.

I did get hung up on the Graphics.Transform methods. They are method overloads so I would expect them to behave the same, however the method on l423 affects subsequent actions and the one on 1070 returns a graphics object with all the previous actions transformed. This wasn't immediately obvious from either the api or documentation.

VectSharp/VectSharp/Graphics.cs

Line 423 in 7da0227

public void Transform(double a, double b, double c, double d, double e, double f, string tag = null)

VectSharp/VectSharp/Graphics.cs

Line 1070 in 7da0227

public Graphics Transform(Func<Point, Point> transformationFunction, double linearisationResolution)

gmkado commented

And I actually missed this page, which does document the Transform behavior:
https://giorgiobianchini.com/VectSharp/advanced_transformations.html

But after reading it I think my stance is the same, it makes it sound like they can be interchanged.

This method has three overloads: an overload that takes six double parameter describing an affine transformation matrix, which has already been discussed in the context of coordinate system transformations, and two overloads whose first parameter is a Func<Point, Point> like the GraphicsPath.Transform method.

arklumpus commented

Hi, thank you for the comments!

I think the documentation comments for the overloads (which should show up in your IDE) should help clarify that the first overload transforms the coordinate system, while the others return a new transformed Graphics:

/// <summary>
/// Transform the coordinate system with the specified transformation matrix [ [a, c, e], [b, d, f], [0, 0, 1] ].
/// </summary>
// ...
public void Transform(double a, double b, double c, double d, double e, double f, string tag = null)
/// <summary>
/// Creates a new <see cref="Graphics"/> object in which all the graphics actions have been transformed using an arbitrary transformation function. Raster images are replaced by grey rectangles.
/// </summary>
// ...
/// <returns>A new <see cref="Graphics"/> object in which all graphics actions have been linearised and transformed using the <paramref name="transformationFunction"/>.</returns>
public Graphics Transform(Func<Point, Point> transformationFunction, double linearisationResolution)

I have updated the online docs to further clarify the difference:

The Graphics.Transform method

The overloads of the Graphics.Transform method can be used to do the same thing as the Transform method of the GraphicsPath class. The Graphics.Transform method has three overloads:

  1. public void Transform(double a, double b, double c, double d, double e, double f, string tag = null): here, the six double parameters describe an affine transformation matrix, which has already been discussed in the context of coordinate system transformations. This method transforms the coordinate system of the Graphics object, and only affects drawing calls performed after it has been invoked.
  2. public Graphics Transform(Func<Point, Point> transformationFunction, double linearisationResolution): the first parameter of this overload is a Func<Point, Point> like the GraphicsPath.Transform method, while the second represents the linearisation resolution. When this overload is used, the Graphics is linearised using the specified linearisation resolution, and then the transformation function is applied. This overload returns a new Graphics object where the transformation has been applied to the elements that had already been drawn.
  3. public Graphics Transform(Func<Point, Point> transformationFunction, double linearisationResolution, double maxSegmentLength): this overload has the same parameters as overload 2, with an additional parameter representing the maximum length of any line segment in the plot. When this overload is used, in addition to linearising all paths in the plot, line segments that are longer than the specified length are also divided in smaller segments, using a similar approach as the one used in the GraphicsPath example above. Like overload 2, this overload returns a new Graphics object where the transformation has been applied to the elements that had already been drawn.

As discussed above:

  • If the transformation you want to apply is an affine transformation, you should use overload 1 before drawing the plot elements on the Graphics object.
  • If the transformation is projective, you should use overload 2 after drawing the plot elements.
  • If the transformation is not projective (or if you are not sure), you should use overload 3 after drawing the plot elements.

I hope this makes things easier to understand, let me know otherwise!

gmkado commented

Thanks for the clarification and updating the docs! One other thing you could do is mark overload 2 and 3 with the [Pure] attribute so users get this warning:

https://www.jetbrains.com/help/resharper/ReturnValueOfPureMethodIsNotUsed.html

arklumpus commented

That's an interesting suggestion! I did some reading, and it would seem that there is a lot of controversy around the [Pure] attribute from System.Diagnostics.Contracts (e.g., it was removed from the System.Collections.Immutable assembly dotnet/runtime#28488, dotnet/runtime#35118 (comment)).

In the end, I suppose we don't really care whether the methods are pure or not, but rather that the returned value is used (the two things are overlapping, but not equivalent). What would be really needed is something like a [DoNotIgnore] attribute (dotnet/runtime#34098), but this doesn't exist yet...

Philosophical issues aside, I will keep this in mind for the next update, but I need to do some thinking on where to actually add the attribute - e.g., only to methods that are actually pure (which would be the semantically correct way of doing it), or to all methods whose return value should not be ignored (which would make it easier to eventually replace it with the [DoNotIgnore] attribute, if it is ever implemented)?

arklumpus commented

I have now (VectSharp v2.5.1) added the [Pure] attribute to hopefully all methods where it doesn't make sense to ignore the return value.

I'm closing this for now, feel free to reopen if you notice something I missed or if you have other suggestions!

  • ❤️1
gmkado commented

Thanks!