Skip to content

Conversation

@brianpopow
Copy link
Contributor

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

Update shared infrastructure for the ImageSharp.Drawing project.

@brianpopow brianpopow changed the title Update shared infrastructure WIP: Update shared infrastructure Mar 17, 2020
@brianpopow
Copy link
Contributor Author

@JimBobSquarePants there are still some errors left in the test project like:

##[error]Processing/FakeImageOperationsProvider.cs(12,50): error CS0281: Friend access was granted by 'SixLabors.ImageSharp, Version=1.0.0.0, Culture=neutral, PublicKeyToken=d998eea7b14cab13', but the public key of the output assembly ('') does not match that specified by the InternalsVisibleTo attribute in the granting assembly.

from a quick google search, it seems related to strong naming ImageSharp. Unfortunately i have no experience with that. If you know more about that please let me know.

@brianpopow
Copy link
Contributor Author

Ok no more build errors, i have signed ImageSharp.Drawing.Tests.csproj with key from shared-infrastructure, see: 080acf2. Still some failing tests. I will try to figure out whats wrong here.

@JimBobSquarePants
Copy link
Member

@brianpopow There's nothing in your changes that would cause differences. Could you post an example of two differing images?

@antonfirsov I'm a little concerned that SixLabors/ImageSharp#1143 may have caused more issues here.

@brianpopow
Copy link
Contributor Author

brianpopow commented Mar 23, 2020

@brianpopow There's nothing in your changes that would cause differences. Could you post an example of two differing images?

@JimBobSquarePants
The failing test is in SolidFillBlendedShapesTests: _1DarkBlueRect_2BlendHotPinkRect_3BlendTransparentEllipse

This is the actual image output:
actual

This is the expected one:
expected

They look the same but they are not, the square in the middle is different. Where the actual output has Rgba32(0, 0, 0, 0), the expected has Rgba32(255, 255, 255, 0).

Here is an image where the difference pixels are marked red:
output

edit: additional info: All failing tests use PixelAlphaCompositionMode Src

@JimBobSquarePants
Copy link
Member

Rgba32(0, 0, 0, 0), the expected has Rgba32(255, 255, 255, 0)

@brianpopow That makes me think that the expected result is actually incorrect. I wonder, do we premultiply in the pipeline anywhere?

@brianpopow
Copy link
Contributor Author

Rgba32(0, 0, 0, 0), the expected has Rgba32(255, 255, 255, 0)

@brianpopow That makes me think that the expected result is actually incorrect. I wonder, do we premultiply in the pipeline anywhere?

Premultiply is not used here. I dont think my changes have introduced this, so im keen in changing the reference images. @JimBobSquarePants should i do that?

@JimBobSquarePants
Copy link
Member

@brianpopow I figured it out!

The tests use Color.Transparent which used to be 255,255,255,0 but is now 0,0,0,0.

Update the references 👍

@brianpopow
Copy link
Contributor Author

The tests use Color.Transparent which used to be 255,255,255,0 but is now 0,0,0,0.

great, that explains its.
Reference images are changed now.

@brianpopow brianpopow changed the title WIP: Update shared infrastructure Update shared infrastructure Mar 25, 2020
@JimBobSquarePants
Copy link
Member

@brianpopow I fixed the build/signing issues for you also. Thanks for doing this, I'll merge it now.

@JimBobSquarePants JimBobSquarePants merged commit 1c4b929 into master Mar 25, 2020
@brianpopow
Copy link
Contributor Author

@brianpopow I fixed the build/signing issues for you also. Thanks for doing this, I'll merge it now.

thanks, i could not figure out the problem

@JimBobSquarePants JimBobSquarePants deleted the updateSharedInfra branch March 28, 2020 20:16
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