Public Vector4 to/from conversions #1050
Conversation
| @@ -83,7 +83,7 @@ public partial class PixelOperations<TPixel> | |||
| /// <param name="configuration">A <see cref="Configuration"/> to configure internal operations</param> | |||
| /// <param name="sourcePixels">The <see cref="Span{T}"/> to the source colors.</param> | |||
| /// <param name="destVectors">The <see cref="Span{T}"/> to the destination vectors.</param> | |||
| internal virtual void ToVector4( | |||
| public virtual void ToVector4( | |||
There was a problem hiding this comment.
This method should not be virtual.
There was a problem hiding this comment.
Wait, this method is actually overridden by some pixel-format-specific implementations of PixelOperations<TPixel>, like for RgbaVector, Rgba32 and others. It's also in the GenerateRgba32CompatibleVector4ConversionMethods part in the _Common.ttinclide file.
I don't think we can remove that virtual right now, given the rest of the codebase as it is now. That is, unless you have some refactoring in mind to make that possible 🤔
There was a problem hiding this comment.
Yeah, we're gonna have to have a think about this!
There was a problem hiding this comment.
@Sergio0694 I did not have the chance to check the code, but if this method is overriden, it's a mistake. At the bottom of the call stack, all scenarios shall be covered with a single virtual overload which is taking the most arguments, the shorter overloads shall be non-virtual wrappers only.
Otherwise everything else LGTM, if someone can confirm that T4 generation is working.
There was a problem hiding this comment.
@antonfirsov So, I'm not sure how to proceed here. From what I see, even before my changes in this branch, that virtual ToVector4 method was already being overridden in virtually all the pixel-format specific PixelOperations<TPixel> classes.
For instance, this happens both in the explicit RgbaVector and Rgba32 types, as well as in all the generated pixel-format classes (eg. here in Argb32.Generated).
I'm not sure how to proceed here if you want to remove the virtual modifier: as far as I can see, right now each pixel-format is returning an instance of its own PixelOperations<TPixel> private class, exposed as PixelOperations<TPixel>, so that when the methods are called on that instance, the right overload for the specific pixel-format is called. And I mean, this makes sense, and it looks great. My question is: if you want to remove the virtual modifier from the default implementation of those methods in the base PixelOperations<TPixel> class, how can we then have each pixel-format call its own custom version of those APIs?
PixelOperations<TPixel> is not abstract so we can't even just set that method as abstract from there and have each pixel-format specific class implement it. Plus as far as I know there are some pixel-formats that do fallback on that base implementation, so this wouldn't work either.
I might very well be missing something here, I'm confused 🤔
There was a problem hiding this comment.
@Sergio0694 I think you were looking at the wrong method. The one highlighted by @antonfirsov does not have any overloads. I've removed the virtual keyword from it and regenerated the templated code.
Once this builds I'm happy to merge it.
There was a problem hiding this comment.
@JimBobSquarePants @antonfirsov Ooof I was looking at the wrong overload 🤦♂️
I did have the feeling I was being stupid, sorry to have wasted your time on this!
Happy to hear the rest of the PR was ok though, thanks for merging it! 😊
There was a problem hiding this comment.
Haha no worries, easy mistake to make!
|
Mh, not not sure why the build failed there, I'll see if I can repro this on my end and try to fix it. |
|
@Sergio0694 What's with the commit history from July? |
|
@JimBobSquarePants I think that's because I created this branch from my existing fork of |
Codecov Report
@@ Coverage Diff @@
## master #1050 +/- ##
=======================================
Coverage 90.03% 90.03%
=======================================
Files 1145 1145
Lines 51762 51762
Branches 3783 3783
=======================================
Hits 46605 46605
Misses 4402 4402
Partials 755 755
Continue to review full report at Codecov.
|
|
@Sergio0694 Still haven't managed to properly sit down and evaluate this. Rest assured though I will do asap. |
|
@JimBobSquarePants Hey, I was just about to ask you for a status update on this! Glad to hear it's still on your radar, that's awesome! And no worries, there's no rush, I just wanted to make sure this would make it into the next beta/RC build 😄 |
Prerequisites
Description
As discussed with @antonfirsov, I've made the following APIs public:
PixelOperations<TPixel>.FromVector4DestructivePixelOperations<TPixel>.ToVector4Overloads are included as well. I also made the
PixelConversionModifiersenum public, otherwise two of those overloads would've been blocked from being exposed.