Imagesharp: Inconsistent Primitives

[willdean]

Prerequisites

• [x] I have written a descriptive issue title
• [x] I have verified that I am running the latest version of ImageSharp
• [x] I have verified if the problem exist in both DEBUG and RELEASE mode
• [x] I have searched open and closed issues to ensure it has not already been reported

Description

ImageSharp has Rectangle and RectangleF (and SixLabors.Shapes.Rectangle) but no PointF - instead Vector2 is used in some places.

I'm new to ImageSharp, and have experimentally ported a .NET System.Drawing-based chart drawing routine to ImageSharp (which has gone sort of OK-ish)

A first impression which I feel is worth expressing, because (if it isn't already) it will soon be too late to fix is that the primitive classes feel rather inconsistent - you can draw a Rectangle but you can't draw a RectangleF, if you want a non-integer Point then you need a Vector2 not a PointF.

Could you get rid of the integer types and just standardise on floats everywhere? Things like Rectangle often get cast to SixLabors.Shapes.Rectangle anyway, which is floating point, and both SixLabors and ImageSharp publish Size structs, one floating point and one integer.

None of this is show-stopping - I've written a few more extension methods on Image<> - but it does feel a bit rough, and it's perhaps the kind of thing only a newcomer would notice - after a while everyone gets numbed to this sort of stuff.

[JimBobSquarePants]

Yeah you're absolutely right there. We need to organize the primitives.

Dropping integer based ones is probably infeasible since there are places outside the library that with expect integer values and unless they are careful they can fall fowl of incorrect rounding behaviour. We should create the missing primitives and offer implicit casting throughout and make sure our API looks correct from the outside.

// Implicit casting works in reverse also.
Rectange -> RectangleF 
Point -> PointF -> Vector2
Size -> SizeF

This is a pretty easy task if someone wants to take it on?

[willdean]

I'll do this if you want - like you say I don't think it's a huge task.

I'm not sure we should have implicit conversions from float to integer types, because that's lossy and it wouldn't be obvious what rounding approach was being taken - having explicit round/ceiling methods to handle that direction is perhaps safer? (I'm not trying to rebuild System.Drawing, honest...)

Is this a fair summary of the tasks:

• Add PointF struct
• Standardise on PointF rather than Vector2 in the public API
• Add implicit Point -> PointF and Rectangle->RectangleF conversions
• Add explicit (or implicit if you think it's wise) PointF -> Point and RectangleF -> Rectangle conversions (some exist)
• Prefer RectangleF / PointF in the public API where appropriate (e.g. on all the drawing operations)
• Have easy (implicit?, explicit?) conversions to Vector2 and Vector4 for internal use

[JimBobSquarePants]

That would be absolutely brilliant if you could!

Yeah you're right. Explicit for the reverse.

I actually quite like the System.Drawing methods on the primitives so am happy to flesh them out to match also. Their all in corefx now so trivial to copy anything we need from there.

[tocsoft]

Definitely shouldn't have implicit float -> int conversion explicit would be ok as long we we internally used the explicit conversion provided by the runtime instead of rounding ourselves( we would be consistent then) we should also provide static methods for Ceiling/Floor available on the float variants.

[willdean]

OK - I made a start on this - I've done a PointF class, but I'm afraid that the attempt to get the project to build (VS2017.2 + Core SDK 1.0.4) and run any unit tests has entirely defeated me, and once again I'm in retreat from the whole .NET Core business.

I doubt the actual code changes required are more than a couple of hours work, but having spent more than that on the tooling, I'm going to pass on this. Best of luck, anyway.

[JimBobSquarePants]

No worries. I can do this, thanks for giving it a go.

@tocsoft I'm thinking use explicit PointF to Point etc using truncation, but define a Round method for specific requirements. That, to me would be behavior developers would expect as it matches int, float etc.