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.

Sign up for GitHub

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

fix shader interpolation and make image comparison tests more tolerant. #10506

Merged
merged 4 commits into from
Mar 27, 2020
Merged

fix shader interpolation and make image comparison tests more tolerant. #10506

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8402880
add debug save image mode
mjkkirschner Mar 25, 2020
5b1f869
enable save
mjkkirschner Mar 25, 2020
a6aa8aa
update shaders to fix them on WARP/real hardware
mjkkirschner Mar 26, 2020
38ae381
rename function, rename params
mjkkirschner Mar 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified src/DynamoCoreWpf/ViewModels/Watch3D/compiledShaders/psDynamoMesh
View file
Open in desktop
Binary file not shown.
Binary file modified src/DynamoCoreWpf/ViewModels/Watch3D/compiledShaders/vsDynamoMesh
View file
Open in desktop
Binary file not shown.
12 changes: 0 additions & 12 deletions src/DynamoCoreWpf/ViewModels/Watch3D/shaderSource/DynamoCommonStructures.hlsl
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions src/DynamoCoreWpf/ViewModels/Watch3D/shaderSource/psDynamoMesh.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
#define MESH
#include"Common.hlsl"
#include"CommonBuffers.hlsl"
#include"DynamoCommonStructures.hlsl"

//--------------------------------------------------------------------------------------
// Phong Lighting Reflection Model
Expand All @@ -26,16 +25,17 @@ SamplerState LinearSampler
MaxAnisotropy = 16;
};

float4 main(PSInputCustom input, bool isFrontFacing : SV_IsFrontFace) : SV_Target
float4 main(PSInput input, bool isFrontFacing : SV_IsFrontFace) : SV_Target
{
//TODO move to common or pass in from material or the vParams.
float4 vSelectionColor = float4(0.0, 0.62, 1.0, 1.0);


//flags were passed through from vertex shader
//flags were passed through from a common buffer per model
//lets decode them
uint flags = int(input.customParams.x);

int flags = int(vParams.x);

//our flags are packed in this order:
/*
None = 0,
Expand Down
8 changes: 2 additions & 6 deletions src/DynamoCoreWpf/ViewModels/Watch3D/shaderSource/vsDynamoMesh.hlsl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,11 @@
#define MESH
#include"Common.hlsl"
#include"DataStructs.hlsl"
#include"DynamoCommonStructures.hlsl"
#pragma pack_matrix( row_major )

PSInputCustom main(VSInput input)
PSInput main(VSInput input)
{
PSInputCustom output;
PSInput output;

//our flags are packed in this order:
/*
Expand Down Expand Up @@ -58,9 +57,6 @@ PSInputCustom main(VSInput input)
//set normal for interpolation
output.n = normalize(mul(input.n, (float3x3)mWorld));

//send our flags through to the frag shader
output.customParams.x = vParams.x;
Copy link
Contributor

@aparajit-pratap aparajit-pratap Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjkkirschner how do the flags get passed to the default PSInput struct?

We currently have a custom struct for the vertex shader called VSInputCustom. Did we also get rid of that in the helix-upgrade branch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean by data interpolation? How did you figure out that that was going on?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we no longer need any custom input structs - the data gets passed to each shader using a directX constant buffer, read about these here:
https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-constants

you can think of this like a uniform in opengl except per model, not shader or triangle.

So imagine we have a triangle with 3 verts - each vert has a color associated with it - to smoothly interpolate those colors for each pixel of the triangle, the renderer interpolates those values and sends those interpolated values to the pixel shader - that was being done to the flags data... it turns out that in directX apparently interpolating between the same value on each vertex does not interpolate correctly... the value should remain constant - but it obviously did not...

in parallels(metal) it did interpolate correctly, so each pixel had the correct flags set.

What we were seeing was each pixel having a random set of flags - some frozen, some selected etc.

I figured out what was going on by noting that the data was being corrupted between the vertex stage and the pixel stage, but was fine if I set it myself using some constant number.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, I would have expected interpolation only to be applied to vertex normals and colors, not for all data in the shader.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it depends on the semantic you use - check this out:
https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-struct#interpolation-modifiers-introduced-in-shader-model-4


return output;
}

Expand Down
73 changes: 58 additions & 15 deletions src/VisualizationTests/HelixImageComparisonTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@

//UNCOMMENT THIS DEFINE TO UPDATE THE REFERENCE IMAGES.
//#define UPDATEIMAGEDATA

//UNCOMMENT TO SAVE DEBUG IMAGES.
//#define SAVEDEBUGIMAGES
using Dynamo.Graph.Nodes.ZeroTouch;
using Dynamo.Selection;
using DynamoCoreWpfTests.Utility;
using HelixToolkit.Wpf.SharpDX;
using NUnit.Framework;
using System;
using System.Collections.Generic;
using System.Drawing;
using System.IO;
using System.Linq;
Expand Down Expand Up @@ -68,9 +70,10 @@ private static System.Drawing.Bitmap BitmapFromSource(BitmapSource bitmapsource)
return bitmap;
}

private string GenerateTestDataPathFromTest(string testname)
private string GenerateTestDataPathFromTest(string testname,bool debug = false)
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
{
var fileName = testname+".png";
var debugstring = debug ? "debug" : string.Empty;
var fileName = testname+debugstring+".png";
string relativePath = Path.Combine(
GetTestDirectory(ExecutingDirectory),
string.Format(@"core\visualization\imageComparison\referenceImages\{0}", fileName));
Expand All @@ -89,32 +92,72 @@ private void RenderCurrentViewAndCompare(string testName)
var refbitmap = BitmapFromSource(refimage);
var newImage = BitmapFromSource(bitmapsource);

#if SAVEDEBUGIMAGES
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
var debugPath = GenerateTestDataPathFromTest(testName,true);
SaveBitMapSourceAsPNG(debugPath, bitmapsource);
#endif

compareImageColors(refbitmap, newImage);
}

//TODO consider something like:
//(diferentPixelsCount / (mainImage.width* mainImage.height))*100
//for a percent image diff.
private static void compareImageColors(Bitmap image1,Bitmap image2)
aparajit-pratap marked this conversation as resolved.
Show resolved Hide resolved
{
Assert.AreEqual(image1.Width, image2.Width);
Assert.AreEqual(image1.Height, image2.Height);
Assert.AreEqual(image1.PixelFormat, image2.PixelFormat);

//x,y,expected,result
var differences = new List<Tuple<int, int, Color, Color>>();

for (var x = 0; x < image1.Width; x++)
{
for (var y = 0; y < image1.Height; y++)
{
var expectedCol = image1.GetPixel(x, y);
var otherCol = image2.GetPixel(x, y);
Assert.AreEqual(expectedCol, otherCol,$"pixel {x}:{y} was not as expected");
if((ColorDistance(expectedCol,otherCol) > 128))
{
differences.Add(Tuple.Create(x, y, expectedCol, otherCol));
// this can be painfully slow, but is useful during debug - uncomment for more info.
// Console.WriteLine($"{expectedCol}, {otherCol}, pixel {x}:{y} was not in expected range");
}
}
}
var diff = CalculatePercentDiff(image1, differences);
Console.WriteLine($"% difference by out of range pixels was {(diff * 100).ToString("N" + 3)}%");
Assert.LessOrEqual(diff, .01);

}

/// <summary>
/// Euclidean distance between colors.
/// Does not use the alpha channel.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the alpha channel not used?

Copy link
Member Author

@mjkkirschner mjkkirschner Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

theres no (meaningful) alpha channel in a full color image with no transparency. These are renders not textures or masks.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about when we are comparing images in isolation mode or frozen geometry?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nope, even then, the final output color is just red,green,blue, the alpha channel is still 0. These are 24bit images anyway, there isn't even an alpha channel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So would the image comparison between an isolated geometry and non-isolated geometry return true (same image)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mjkkirschner OK, let me read your comments carefully again!

Copy link
Member Author

@mjkkirschner mjkkirschner Mar 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it's a bit confusing - but you can see in the shader:

//if isolated - alpha should always be low - 
//overriding other alpha values (except if selected)
if(isIsolated && !isSelected)
{
	I.a = .1f;
}

that if the isolate mode is active we set the alpha of the returned color (returned from the shader)

Because the shader is using AlphaBlending - the renderer uses this alpha to compute a final color.

It doesn't matter if each vertex had a low alpha or a high alpha, since they're not used.... atleast not unless that Model has RequiresPerVertexColor enabled.

I hope that makes sense. Please let me know if not, I can find some better written docs on the subject.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thats unrelated - in your test you were not looking at the color values of an image, you were looking at the color values of individual verts which were not used for computing the final color.

Yup, sorry for confusing the two; my bad.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So when we generate a bitmap the alpha channel has already been incorporated into the final RGB color - you can think of this like premultiplying or something - the color values have been modified by the alpha channel in some way to create a color blended with the other colors that were located at the same pixels.

Ah, so it's something like homogeneous coordinates :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Make sense! @mjkkirschner

/// </summary>
/// <param name="color1"></param>
/// <param name="color2"></param>
/// <returns></returns>
private static double ColorDistance(Color color1, Color color2)
{
var r1 = color1.R;
var g1 = color1.G;
var b1 = color1.B;

var r2 = color2.R;
var g2 = color2.G;
var b2 = color2.B;
return Math.Sqrt(Math.Pow((r2 - r1), 2) + Math.Pow((g2 - g1),2) + Math.Pow((b2 - b1),2));
}

private static double CalculatePercentDiff(Bitmap image1, List<Tuple<int,int,Color,Color>> differences)
{
//TODO should we remove background pixels from this calculation via color?
// or other filtering techniques for more precision
var totalPixels = image1.Width * image1.Height;
return (double)differences.Count / (double)totalPixels;
}

#endregion
#endregion

#region meshes
#region meshes
[Test]
public void StandardMeshGeometryRender()
{
Expand Down Expand Up @@ -171,8 +214,8 @@ public void VertColorFrozenMeshGeometryRender()
node3.IsFrozen = true;
RenderCurrentViewAndCompare(MethodBase.GetCurrentMethod().Name);
}
#endregion
#region pointsAndLines
#endregion
#region pointsAndLines
[Test]
public void points()
{
Expand Down Expand Up @@ -256,9 +299,9 @@ public void linesFrozen()
RenderCurrentViewAndCompare(MethodBase.GetCurrentMethod().Name);
}

#endregion
#endregion

#region SpecialRenderPackages
#region SpecialRenderPackages
[Test]
public void directManipulator()
{
Expand All @@ -272,7 +315,7 @@ public void directManipulator()
RenderCurrentViewAndCompare(MethodBase.GetCurrentMethod().Name);
}

#endregion
#endregion

}
}