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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Projective Transforms #546

Merged
merged 20 commits into from May 4, 2018

Conversation

Projects
None yet
2 participants
@JimBobSquarePants
Copy link
Member

JimBobSquarePants commented Apr 26, 2018

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 matches 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

Adds the ability to perform projective transforms (perspective, taper) on images

@JimBobSquarePants JimBobSquarePants requested a review from antonfirsov Apr 26, 2018

@JimBobSquarePants

This comment has been minimized.

Copy link
Member

JimBobSquarePants commented Apr 26, 2018

@antonfirsov Surprising comparison results here. 32bit passing 64 bit failing? WTF?

@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented Apr 26, 2018

@JimBobSquarePants the difference is not big however. I suspect the root cause is a difference between SIMD and software implementations of Vector4 operations.

Hope it's not CPU dependent & locally reproducible.

image.Mutate(i => { i.Transform(m); });

string testOutputDetails = $"{taperSide}-{taperCorner}";
image.DebugSave(provider, testOutputDetails);

This comment has been minimized.

@antonfirsov

antonfirsov Apr 28, 2018

Member

@JimBobSquarePants added some tests to cover CreateTaperMatrix() parameters. I may get it wrong but the XxxOrYyy stuff might be the inverse of the intended. Can you check the output?

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants Apr 28, 2018

Member

I'll have to do some tests locally to ensure the output is expected. The class is based on a Skia example and and old Charles Petzold blog post.

@antonfirsov
Copy link
Member

antonfirsov left a comment

I think I managed to workaround the floating point issue. It is related to Vector2 again.

The API is perfect. If CreateTaperMatrix() parameters behave as you originally intended, we can add the reference images for Transform_WithTaperMatrix and merge the PR. (But we probably need to invert TaperCorner behavior.)

/// <param name="taperCorner">An enumeration that indicates on which corners to taper the rectangle.</param>
/// <param name="taperFraction">The amount to taper.</param>
/// <returns>The <see cref="Matrix4x4"/></returns>
public static Matrix4x4 CreateTaperMatrix(Size size, TaperSide taperSide, TaperCorner taperCorner, float taperFraction)

This comment has been minimized.

@antonfirsov

antonfirsov Apr 28, 2018

Member

Cool suff! Covers basic cases & good for testing without getting outside the scope of the library.

This comment has been minimized.

@JimBobSquarePants

JimBobSquarePants Apr 30, 2018

Member

Thanks for adding the extra tests, they affirm the expected output well.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Apr 28, 2018

Codecov Report

Merging #546 into master will increase coverage by 0.41%.
The diff coverage is 98.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #546      +/-   ##
==========================================
+ Coverage   88.28%   88.69%   +0.41%     
==========================================
  Files         838      840       +2     
  Lines       35186    35300     +114     
  Branches     2538     2544       +6     
==========================================
+ Hits        31064    31310     +246     
+ Misses       3377     3243     -134     
- Partials      745      747       +2
Impacted Files Coverage 螖
...Sharp/Processing/Transforms/TransformExtensions.cs 54.54% <酶> (+18.18%) 猬嗭笍
.../Transforms/Processors/AffineTransformProcessor.cs 100% <100%> (酶) 猬嗭笍
...s/Processors/InterpolatedTransformProcessorBase.cs 100% <100%> (酶) 猬嗭笍
...nsforms/Processors/ProjectiveTransformProcessor.cs 100% <100%> (+100%) 猬嗭笍
...ests/Processing/Transforms/AffineTransformTests.cs 98.52% <100%> (-0.02%) 猬囷笍
.../Processing/Transforms/ProjectiveTransformTests.cs 97.22% <97.22%> (酶)
...Processing/Transforms/ProjectiveTransformHelper.cs 97.22% <97.22%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update 6b4de97...775aa90. Read the comment docs.

JimBobSquarePants added some commits Apr 28, 2018

@JimBobSquarePants

This comment has been minimized.

Copy link
Member

JimBobSquarePants commented Apr 30, 2018

@antonfirsov Back to you; do your magic please.

@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented Apr 30, 2018

Bottom-LeftOrTop:
transform_withtapermatrix_rgba32_solid30x30_ 255 0 0 255 _bottom-leftortop

Bottom-RightOrBottom:
transform_withtapermatrix_rgba32_solid30x30_ 255 0 0 255 _bottom-rightorbottom

@JimBobSquarePants you sure TaperCorner works fine? Isn't it the inverse of the intended? Or I'm getting it wrong?

@JimBobSquarePants

This comment has been minimized.

Copy link
Member

JimBobSquarePants commented May 1, 2018

Bottom-LeftOrTop:
transform_withtapermatrix_rgba32_solid30x30_ 255 0 0 255 _bottom-leftortop

Bottom-RightOrBottom:
transform_withtapermatrix_rgba32_solid30x30_ 255 0 0 255 _bottom-rightorbottom

@JimBobSquarePants you sure TaperCorner works fine? Isn't it the inverse of the intended? Or I'm getting it wrong?

@antonfirsov I think it's correct.

In the first example we are choosing to taper the "Bottom" side on either the "Left" or "Top" corner. Since "Left" is possible regardless of whether "Top" is impossible "Left" is chosen.

In the seconds example we are choosing to taper the "Bottom" side on either the "Right" or "Bottom" corner. Since "Right" is possible regardless of whether "Bottom" is possible "Right" is chosen.

The original article with source code for the class this is based on can be found here:

http://www.charlespetzold.com/blog/2009/07/Taper-Transforms-with-Matrix3DProjection-An-Analytical-Approach.html

SkiaSharp have an example here:
https://docs.microsoft.com/en-us/xamarin/xamarin-forms/user-interface/graphics/skiasharp/transforms/non-affine

I can attempt to compare against the demos. (Feel free to have a look first though) https://developer.xamarin.com/samples/xamarin-forms/SkiaSharpForms/Demos/

@JimBobSquarePants

This comment has been minimized.

Copy link
Member

JimBobSquarePants commented May 2, 2018

Behaviour confirmed 馃槃

taper-bottom-left-or-top
taper-bottom-right-or-bottom

@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented May 2, 2018

LOL .. that image!

But isn't the first example tapering towards the right corner? Or should I start over my English courses? :D

@JimBobSquarePants

This comment has been minimized.

Copy link
Member

JimBobSquarePants commented May 2, 2018

Yeah I felt a little guilty posting that 馃槃

Here's the comparative images side by side and scaled.

Bottom-LeftOrTop:
taper-bottom-left-or-top
taper-bottom-left-or-top

Bottom-RightOrBottom:

taper-bottom-right-or-bottom

@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented May 2, 2018

It seems to me that the original definition is wrong, because the "tapered" edge is not on the left (but on the right!) corner of the image.

image

But again, it might be just my wrong understanding of the term "tapering".

@JimBobSquarePants

This comment has been minimized.

Copy link
Member

JimBobSquarePants commented May 2, 2018

Ah no... I can see how this is confusing but the language is correct.

A classic definition of taper would be

to become progressively smaller toward one end.

You're tapering towards the bottom edge and choosing which corner to taper with.

@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented May 2, 2018

I understand now.
The PR is in a perfect state, sorry for the long linguistic session! :)

@antonfirsov

This comment has been minimized.

Copy link
Member

antonfirsov commented May 2, 2018

Well .. almost. Haven't noticed, the tests are failing again, gonna check it out now.

@JimBobSquarePants

This comment has been minimized.

Copy link
Member

JimBobSquarePants commented May 3, 2018

floating-points-from-hell

Haha! 馃ぃ

It's amazing really the trouble we've had with this. Will be happy when MS push all their Vector2 fixes.

@JimBobSquarePants JimBobSquarePants merged commit 3cc7caa into master May 4, 2018

5 checks passed

codecov/patch 98.19% of diff hit (target 88.28%)
Details
codecov/project 88.69% (+0.41%) compared to 6b4de97
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@JimBobSquarePants JimBobSquarePants deleted the js/projective-transforms branch May 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment