manuelbl / SwissQRBill.NET

.NET library for Swiss QR bill payment slips (aka QR-Rechnung)
MIT License
86 stars 32 forks source link

Eventually replace `System.Drawing.Common` #34

Closed 0xced closed 2 years ago

0xced commented 3 years ago

See https://github.com/dotnet/designs/pull/234 (merged in Make System.Drawing.Common only supported on Windows) Using SkiaSharp or ImageSharp is the recommended alternative.

Note: I'm currently working on a Verify branch that should make it easy to catch differences when changing the rendering engine (by displaying a visual diff of the generated QR-bills).

0xced commented 2 years ago

For reference: How to draw a dotted line? in ImageSharp.Drawing discussions.

0xced commented 2 years ago

I have implemented PNG rendering with ImageSharp.Drawing in my ImageSharp branch. It was a nice exercice and the results are very good. Once pull request #35 is merged I can open a new pull to switch the rendering engine from System.Drawing.Common to ImageSharp.Drawing.

manuelbl commented 2 years ago

It's not obvious yet if a new PNGCanvas implementation should be based on SkiaSharp or ImageSharp. In the branch "skia-sharp" I've created one based in SkiaSharp. You are welcome to add your implementation based on ImageSharp to another branch off of "v3".

The image comparison tests for PNG fail, mostly because the positioning of text is slightly off. It has never been super consistent between PDF, PNG and SVG. The SkiaSharp implementation seems to be closer to PDF than the earlier System.Drawing.Common implementation.

0xced commented 2 years ago

It's not obvious yet if a new PNGCanvas implementation should be based on SkiaSharp or ImageSharp. In the branch "skia-sharp" I've created one based in SkiaSharp. You are welcome to add your implementation based on ImageSharp to another branch off of "v3".

I will rebase my ImageSharp branch on the v3 branch.

The image comparison tests for PNG fail, mostly because the positioning of text is slightly off. It has never been super consistent between PDF, PNG and SVG. The SkiaSharp implementation seems to be closer to PDF than the earlier System.Drawing.Common implementation.

I think this can be worked around by configuring another error metric (perceptual hash) for the image comparison and tweaking the threshold:

VerifyImageMagick.RegisterComparers(threshold: 0.?, ErrorMetric.PerceptualHash);
0xced commented 2 years ago

A thought for the v3: how about separating the NuGet package into several ones?

So that consumers can choose the bitmap rendering package or not take a dependency on an image rendering package at all if they are happy with PDF/SVG and don't need to render bitmap images.

Or just choose only one to avoid a high maintenance cost (probably ImageSharp or SkiaSharp), name it Codecrete.SwissQRBill.Generator.Bitmap and let the consumer write its own implementation it they need another PNG renderer.

What do you think?

manuelbl commented 2 years ago

The QR Bill specification more or less forbids bitmaps to represent the bill even though there are good applications for it in user interfaces. So my early version of the QR Bill libraries for Java and .NET had an easy way to generate SVG and PDF but required extra code for PNG. From user feedback I quickly figured out that it caused a lot of confusion and came as a surprise to users. They expected that SVG, PDF and PNG can be generated in the same way.

By separating the PNG rendering into a separate package, we would reintroduce the additional complexity. There must be a good reason for it, and not just an edge case.

There would also need to be a good reason to maintain three different packages. I often find that software aspects that are configurable and marketed as "choice for consumer" are in fact due to insufficient analysis of consumer needs and fear of making a decision. It is usually more valuable for consumers if it's not configurable but just works. So I strongly prefer to select a single graphics library unless it causes severe incompatibility for certain environments.

0xced commented 2 years ago

By separating the PNG rendering into a separate package, we would reintroduce the additional complexity. There must be a good reason for it, and not just an edge case.

I'm not sure to understand what complexity it would introduce. Want PDF + SVG rendering ⇒ out of the box in the Codecrete.SwissQRBill.Generator package. Want PNG rendering ⇒ use Codecrete.SwissQRBill.Generator.Bitmap package which depends on Codecrete.SwissQRBill.Generator. That would be two lines in the README.

On the plus side, if you don't need bitmap rendering, you avoid a huge dependency.

There would also need to be a good reason to maintain three different packages. I often find that software aspects that are configurable and marketed as "choice for consumer" are in fact due to insufficient analysis of consumer needs and fear of making a decision. It is usually more valuable for consumers if it's not configurable but just works. So I strongly prefer to select a single graphics library unless it causes severe incompatibility for certain environments.

Agreed, choosing a single implementation is the best solution.

manuelbl commented 2 years ago

Before PNG was integrated into the base package, generating PDF and SVG was quite different from generating PNG. For the former one, you could simply copy the code from the README and choose the format with GraphicsFormat = GraphicsFormat.PDF. For PNG, it was completely different. You had to create an instance of PNGCanvas and use then QRBill.Draw to generate the bill. Users didn't understand it (even though there was a complete example) and a got a lot of questions.

Is there an approach where the PNG rendering is split into a separate package (as you propose) but can still be used with the same basic code for QR bill generation?

0xced commented 2 years ago

Is there an approach where the PNG rendering is split into a separate package (as you propose) but can still be used with the same basic code for QR bill generation?

That's what I had in mind and I think it's quite easily achievable with the current architecture. I will try to draft this and let you know how it goes.

SimonCropp commented 2 years ago

The image comparison tests for PNG fail, mostly because the positioning of text is slightly off

note that if r using verify for image comparison u can use the imagemagick or phash to ignore minor differences based on a tolerance https://github.com/VerifyTests/Verify/blob/main/docs/comparer.md#pre-packaged-comparers

0xced commented 2 years ago

We're already using the perceptual hash actually:

VerifyImageMagick.RegisterComparers(threshold: 0.07, ImageMagick.ErrorMetric.PerceptualHash);
manuelbl commented 2 years ago

Is there an approach where the PNG rendering is split into a separate package (as you propose) but can still be used with the same basic code for QR bill generation?

That's what I had in mind and I think it's quite easily achievable with the current architecture. I will try to draft this and let you know how it goes.

Do you already have a draft for it? If not, I will try to come up with a solution.

My plan is to release a new major version shortly after .NET 6 has been officially released.

manuelbl commented 2 years ago

I have been working on a solution with two NuGet packages for some time (core package including SVG and PDF generation, add-on package for PNG). Unfortunately, the additional complexity seems to be very high.

Having two NuGet packages, the core package being a dependency of the add-on package, makes it hard to develop the add-on package. I see two approaches:

At the moment, my conclusion is that the additional effort is not worth the gain. Unless I can find a better setup, I'll go with a single NuGet package using SkiaSharp.

wagich commented 2 years ago

@manuelbl I stumbled upon your library because I'll need to generate QR bills and noticed this issue. I hope I'm not overstepping by giving my 2 cents on the multiple packages story:

Unless I'm fundamentally misunderstanding your setup, dealing with multiple packages should be as easy as:

The project reference will automatically be converted into a nuget package reference.

I just tested this using the skia-sharp branch: in commit 65284423 before the split into multiple solutions, you can just change the package to a project reference in PixelCanvas.csproj

-   <PackageReference Include="Codecrete.SwissQRBill.Generator" Version="3.0.0-rc1" />
+   <ProjectReference Include="..\Generator\Generator.csproj" />

the resulting Codecrete.SwissQRBill.PixelCanvas package then correctly references the Generator package.

manuelbl commented 2 years ago

Thanks a lot for your input. I will try it later this week.

It sounds like a great setup for development. Hopefully it also solves the CI/CD challenges.

manuelbl commented 2 years ago

Release 3.0.0 is now fully compatible with .NET 6 by removing the dependency on System.Drawing. It has been split into to libraries:

Thanks a lot for your help and input.

@wagich: Thanks for your suggestion regarding a single solution with project references. That's how I've implemented it. It wasn't without issues though: dotnet tool doesn't fully support it but sufficiently to find a working combination.

0xced commented 2 years ago

Good job on the v3 release and the removal of System.Drawing. Introducing the Codecrete.SwissQRBill.Core package that doesn't depend on SkiaSharp was perfect, thanks! 👌