Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add QuadDistortion to ProjectiveTransformBuilder #2748

Merged
merged 10 commits into from
Oct 21, 2024

Conversation

Socolin
Copy link
Contributor

@Socolin Socolin commented Jun 8, 2024

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the existing coding patterns and practice as demonstrated in the repository. These follow strict Stylecop rules 👮.
  • I have provided test coverage for my change (where applicable)

Description

Add 2 new methods: PrependQuadDistortion and AppendQuadDistortion to ProjectiveTransformBuilder.

Those method allow to apply a distortion on an image by specifying the requested coordinate for each corner.

Example:

using (var img1 = Image.Load("cat-original.png"))
{
	var topLeft = new Point(25, 25);
	var topRight = new Point(200, 75);
	var bottomRight = new Point(225, 175);
	var bottomLeft = new Point(38, 225);

	img1.Mutate(c => c.Transform(new ProjectiveTransformBuilder()
		.AppendQuadDistort(topLeft, topRight, bottomRight, bottomLeft)));
	img1.Save("output-small2.png");
}

Source
image
Result
image

@CLAassistant
Copy link

CLAassistant commented Jun 8, 2024

CLA assistant check
All committers have signed the CLA.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
You have signed the CLA already but the status is still pending? Let us recheck it.

@JimBobSquarePants
Copy link
Member

Thanks @Socolin for this, it is very interesting! I'll have to put my math's hat on to look at this.

I'm curious. Is this potentially an alternative (better) approach to ProjectiveTransformBuilder.PrependTaper?

@Socolin
Copy link
Contributor Author

Socolin commented Jul 10, 2024

FYI: The performance test I did

TestPerformanceGaussianElimination.zip

  • Matrix is using MathNet.Numerics Nuget
  • MatrixCustom is the implementation included in this PR
  • MatrixCustomFloat is the same implementation with float instead of a generic
  • Matrix8x8 is using TNumber[,] instead of TNumber[][] with static size of 8x8

BenchmarkDotNet v0.13.12, Ubuntu 22.04.4 LTS (Jammy Jellyfish)
AMD Ryzen 9 7950X, 1 CPU, 32 logical and 16 physical cores
.NET SDK 8.0.302
  [Host]     : .NET 8.0.6 (8.0.624.26715), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI
  DefaultJob : .NET 8.0.6 (8.0.624.26715), X64 RyuJIT AVX-512F+CD+BW+DQ+VL+VBMI


| Method            | Mean     | Error   | StdDev  |
|------------------ |---------:|--------:|--------:|
| Matrix            | 526.6 ns | 1.02 ns | 0.96 ns |
| MatrixCustom      | 237.0 ns | 3.31 ns | 3.09 ns |
| MatrixCustomFloat | 234.1 ns | 4.64 ns | 4.34 ns |
| Matrix8x8         | 293.5 ns | 0.32 ns | 0.28 ns |

@JimBobSquarePants
Copy link
Member

@Socolin I'll see if I can find some time over the next few days to pull down your fork and help reimplement it into the latest codebase. I'm keen to add this functionality.

@Socolin
Copy link
Contributor Author

Socolin commented Jul 31, 2024

@Socolin I'll see if I can find some time over the next few days to pull down your fork and help reimplement it into the latest codebase. I'm keen to add this functionality.

I finally have some time again.
I'll push changes related to the today. If it's easier for you to take over this PR, feel free to do it :)

@Socolin
Copy link
Contributor Author

Socolin commented Jul 31, 2024

I rebased the branch and pushed the changes

@Socolin
Copy link
Contributor Author

Socolin commented Jul 31, 2024

I just saw I'm getting an error because ProjectiveTransformBuilder Append take 2 arguments now, should this be computed twice ? Since this operation is taking some processing time, I'm not sure about what to do here.

@JimBobSquarePants
Copy link
Member

I just saw I'm getting an error because ProjectiveTransformBuilder Append take 2 arguments now, should this be computed twice ? Since this operation is taking some processing time, I'm not sure about what to do here.

The operation takes nanoseconds so I'm not concerned by that.

@JimBobSquarePants
Copy link
Member

Hi @Socolin would it be possible for you to give me full write access to your fork so I can complete this PR for you?

There's a known issue with Git LFS and GitHub which means I cannot update the reference images without full write access.

Thanks!

@Socolin
Copy link
Contributor Author

Socolin commented Oct 16, 2024

I did grant you access to the repo, can you confirm it's all right ?

@JimBobSquarePants
Copy link
Member

I did grant you access to the repo, can you confirm it's all right ?

Perfect. Thank you!

@JimBobSquarePants
Copy link
Member

I think this is good to go now. Thanks @Socolin for submitting the solution for this. It's a fantastic addition to the library!

@JimBobSquarePants JimBobSquarePants merged commit f61cbe7 into SixLabors:main Oct 21, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants