-
-
Notifications
You must be signed in to change notification settings - Fork 888
Pixel remapping/Swizzling #1388
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
Conversation
src/ImageSharp/Processing/Processors/Transforms/SwizzleProcessor{TSwizzler,TPixel}.cs
Outdated
Show resolved
Hide resolved
|
@Evangelink are you still interested / do you have time to push this to completion? |
|
Sorry @antonfirsov I got distracted by some other work. I will give it a try tomorrow, I hope to be able to make it work! |
|
@antonfirsov I have made some change but I have no confidence at all in what I have written, I have tried to put up 2 tests based on others but I also don't feel confident about them (not sure I really got how the test infra is working). I am happy to try to finish the work but I feel like I need a full baby-sitting so I am afraid that's pointless for you... Let me know if you'd rather close this. |
|
Also I am not sure what's the process to update your submodule. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Evangelink you don't need reference images to test this feature. I think it's better to implement some basic "unswizzle" code right in the tests then call ImageComparer.Exact.VerifySimilarity to compare the unswizzled image to the original one.
Avoid relying on the submodule change by implementing the invert swizzle operation
f2065bb to
f1a799e
Compare
Codecov Report
@@ Coverage Diff @@
## master #1388 +/- ##
=======================================
Coverage 83.68% 83.68%
=======================================
Files 732 734 +2
Lines 31984 31990 +6
Branches 3605 3605
=======================================
+ Hits 26766 26772 +6
Misses 4505 4505
Partials 713 713
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good with one suggestion.
@onepiecefreak3 I know it's been ages, but any chance you can take a look if this is something that would meet your expectations?
src/ImageSharp/Processing/Processors/Transforms/SwizzleProcessor{TSwizzler,TPixel}.cs
Outdated
Show resolved
Hide resolved
|
@antonfirsov @JimBobSquarePants Issues are unrelated to this PR, e.g.: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Should we add the ability to swizzle a sub rectangle of the source or are we happy just doing the whole image? |
|
@JimBobSquarePants I am not sure how you could apply it only to some sub-rectangle. The new coordinates could be on some space outside of the rectangle (either on an existing location in the image or even outside of the image), what would you do in such case? |
|
@Evangelink Let's not bother for now then. I'm happy with this as is. Thanks very much for helping out!! 👍 |
|
@JimBobSquarePants You are welcome! @antonfirsov Thank you for the strong support :) |
Pixel remapping/Swizzling
Prerequisites
Description
Fix #1115