work in progress for review #2

mjkkirschner · 2020-05-29T22:42:49Z

WIP - definitely cleanup required but this "works", as in we can pass data to the pixelshader this way and points update in response. It needs:

1. cleanup,
2. refactor of duplicated code
3. figure out why the points are rectangles, not ellipses.
4. another shader for lines
5. find another unused property for lines.

update shaders use the blendFactor prop on pointGeometry to pack our data material classes are not used

src/DynamoCoreWpf/ViewModels/Watch3D/shaderSource/vsDynamoPointLine.hlsl

aparajit-pratap · 2020-05-30T00:14:23Z

src/DynamoCoreWpf/ViewModels/Watch3D/DynamoPointLineMaterialVariable.cs

+        /// <summary>
+        /// Is this model Frozen.
+        /// </summary>
+        public bool IsFrozenData { get { return isFrozenData; } internal set { SetAffectsRender(ref isFrozenData, value); } }


Is there something special about doing SetAffectsRender? I've seen Set used in some places and another Setsomething method in helix.

I see that you didn't have to override OnUpdateModelStruct; I remember you mentioned it doesn't get called anyway. Secondly, I'm still a little confused why you didn't need to add the geometry shader to the shader passes.

OnUpdateModelStruct now gets called since the pipeline is setup correctly, unfortunately the point model struct is very small and doesn't have anywhere we can stick our extra data.

it's only 2 bools and a matrix that we need to do the projection - so I just used a value that is set to the PointLine constant buffer, I don't know how to update this otherwise, but it looks like it is somehow hooked into the default material inside the helix implementation ,but fortunately we can access it from the geometryModel object directly, so no need to hook into the render events etc.

I still really want to know how to add arbitrary data instead of reusing these unused variables, hopefully we can get more info from the helix maintainers for the future...

aparajit-pratap · 2020-05-30T00:21:32Z

This is brilliant btw! 🚀

mjkkirschner · 2020-05-30T17:49:18Z

I think all we need to do to get the points rendering as circles is to add:

    if (pfParams[2] == 1)
    {
        float len = length(input.t);
        if (len > 1.4142)
            discard;
    }

to your pixel shader. pfParams[2] or pfParams.z are the same thing and contain the figure type. FigureType = 1 is a circle, this just gets the length of the texture coordinates which range from -1 to 1, but are actually different lengths in world space depending on the output of the GS. So this just checks the radius is under a certain relative size and discards pixels outside that radius,.

aparajit-pratap · 2020-05-31T01:52:55Z

So we need to set pfParams[2] to 1 in CommonBuffers.hlsl? I thought the square shape of the points was coming from the default geometry shader and I tried replacing the makequads with a circle with little success: https://github.com/helix-toolkit/helix-toolkit/blob/66a4fa8bb56ec24a7c1b562225453f85987e08b1/Source/HelixToolkit.Native.ShaderBuilder/GS/gsPoint.hlsl#L55

On a side note I was curious whether replacing our current implementation of coloring points and lines with shaders (or blending) was even worth doing. I noticed that even with a million points, the current implementation sets their colors almost instantaneously with selection and isolation turned on and off very quickly. I didn't test this with lines though. Maybe the slowness that DYN-2702 refers to is seen with lines?

aparajit-pratap · 2020-05-31T01:57:49Z

I thought the square shape of the points was coming from the default geometry shader and I tried replacing the makequads with a circle with little success.

I think what's happening is that quads (squares) are being drawn by the geometry shader and then pixels at the corners of those squares are being discarded in the pixel shader (with the logic you shared) to give the effect of circular points.

mjkkirschner · 2020-05-31T17:29:37Z

I thought the square shape of the points was coming from the default geometry shader and I tried replacing the makequads with a circle with little success.

I think what's happening is that quads (squares) are being drawn by the geometry shader and then pixels at the corners of those squares are being discarded in the pixel shader (with the logic you shared) to give the effect of circular points.

yep, that exactly what happens, the pixel shader clips the output from the geometry shader.

mjkkirschner · 2020-05-31T18:46:59Z

On a side note I was curious whether replacing our current implementation of coloring points and lines with shaders (or blending) was even worth doing. I noticed that even with a million points, the current implementation sets their colors almost instantaneously with selection and isolation turned on and off very quickly. I didn't test this with lines though. Maybe the slowness that DYN-2702 refers to is seen with lines?

On my machine I notice this mostly when drag selecting over the nodes. It's painfully slow, maybe this is a parallels thing? Or just a slow machine thing? (my machine is a few years old)

lines will be at least twice as slow as points, as each line contains more than 1 point and each needs to have its vertex color updated separately.

I think the worst case scenario to test is to try this with points and lines that are using vertex colors ie - those drawn using Display.ByGeometryColor node.

mjkkirschner · 2020-05-31T23:11:07Z

@aparajit-pratap giving a shot at a refactor here, lets try to catch up tomorrow.

aparajit-pratap · 2020-05-31T23:20:16Z

@mjkkirschner will these changes affect the performance of ByGeometryColor as-is? As I see it in that node, we are still individually assigning the new color to each vertex, which is why it still seems slow. Is the goal to set one of the variables used in the pixel shader to the color selected in that node in order to see the benefits of the shader? I think we should be doing the same for meshes as well, don't we?

mjkkirschner · 2020-05-31T23:27:14Z

I think a few subtleties are being confused.
The initial setting of each vertex color at mesh, or point, or line creation time is a small overhead compared to the runtime change that occurs when we say ask the HelixViewModel to reset the colors to their original colors before doing a freeze for example. Only points and lines suffer from this performance penalty because we're using the vertex colors directly.

We could make even more optimizations as you say, to all shaders to use the first vertex color to display as ALL vertex colors, this would let us only send a single color and have the GPU do the rest for us - but we would need to set it into a buffer somewhere, it would not make sense to have each vertex refer to the first vertex in the shader.

This would not work well for custom drawn meshes where users override tessellate and draw whatever colors they want... we still need to support that use case.
⚠️

Thats not something I'm looking into at the moment as I think it's an optimization not a major performance issue.

In my mind the biggest issue is the whole requestResetColor event and the fact that is exists at all.

find same buffer slot for lines and points yay refactor shared data core update tests

mjkkirschner · 2020-06-01T00:06:37Z

@aparajit-pratap - if you have a chance tomorrow I'd like to go over my changes here with you to get your feedback.

summary is as follow:

I moved out the data stuff to a shared core, so that mesh, lines,and points all have an instance of this data since they all share the same properties (selection, freeze etc).
I found that we can access the existing material slots for points and lines and so those can share the same core you added and the shaders can use the same slot - we still need different shaders though because the line shader will require a different GS shader (we should just use the existing helix one) - see how I used the existing point GS shader.
I added the clipping if statement to the PSPoint shader.
updated some tests to use the new data core.

I think all thats left here is implementing the line shader and hooking it up with the correct name.

mjkkirschner · 2020-06-01T00:08:19Z

src/DynamoCoreWpf/ViewModels/Watch3D/DynamoLineGeometryModel3D.cs

+        /// <param name="state"></param>
+        internal void SetState(int state)
+        {  
+             //TODO should we also use this for points if it works for lines to make them consistent?


this TODO is out of date, already did this.

mjkkirschner · 2020-06-01T00:08:49Z

src/DynamoCoreWpf/ViewModels/Watch3D/DynamoPointGeometryModel3D.cs

+        internal void SetPropertyData(DependencyPropertyChangedEventArgs args, GeometryModel3D geo)
+        {
+            this.dataCore.SetPropertyData(args);
+            //re reuse blending factor because our shader does not use it and we need space for an int


comment is out of date, will remove it.

aparajit-pratap · 2020-06-01T00:22:21Z

src/DynamoCoreWpf/ViewModels/Watch3D/DynamoLineGeometryModel3D.cs

+        internal void SetState(int state)
+        {  
+             //TODO should we also use this for points if it works for lines to make them consistent?
+            this.material.FadingNearDistance = state;


Did you replace this for blendingfactor as it is common to both point and line and because blendingfactor is missing for line?

aparajit-pratap · 2020-06-01T00:28:07Z

src/DynamoCoreWpf/ViewModels/Watch3D/DynamoLineGeometryModel3D.cs

@@ -13,6 +13,17 @@ protected override SceneNode OnCreateSceneNode()
        {
            return new DynamoLineNode();


Don't we have to set { Material = material; } in the constructor of DynamoLineNode as well?

yes, will fix that, thanks for catching it.

aparajit-pratap · 2020-06-01T00:34:18Z

@mjkkirschner I think this looks good. Thanks for the hard work on it to bring it to some final shape.

It would be interesting to see if there are any perceivable performance improvements in selection/frozen etc. state changes on point as lines as the current performance still doesn't seem all that bad to me. The following is from the build we used for the helix bug bash. The graph has about 50,000 points and 810,000 lines:

aparajit-pratap · 2020-06-01T00:54:48Z

Don't we also have to set pfParams[2] to 1 in CommonBuffers.hlsl for the Figure type to be chosen to be a circle: https://github.com/helix-toolkit/helix-toolkit/blob/76dc523c9bf928ef2c6f6e72246df28959cc0336/Source/HelixToolkit.Native.ShaderBuilder/Common/CommonBuffers.hlsl#L145?

mjkkirschner · 2020-06-01T00:57:00Z

Don't we also have to set pfParams[2] to 1 in CommonBuffers.hlsl for the figure to be chosen to be a circle: https://github.com/helix-toolkit/helix-toolkit/blob/76dc523c9bf928ef2c6f6e72246df28959cc0336/Source/HelixToolkit.Native.ShaderBuilder/Common/CommonBuffers.hlsl#L145?

no, pfParams is set by the default point material based on the height, width, figure, and figure ratio properties that the PointGeometryModel have been set to.
https://github.com/helix-toolkit/helix-toolkit/blob/76dc523c9bf928ef2c6f6e72246df28959cc0336/Source/HelixToolkit.SharpDX.Shared/Model/Material/Variables/PointMaterialVariable.cs#L55

and we already set the pointFigure to circle when we create DynamoPointGeometryModels

mjkkirschner · 2020-06-01T00:58:09Z

see here:
https://github.com/DynamoDS/Dynamo/blob/37514ba43bb802743555faa8460c45fad807c63c/src/DynamoCoreWpf/ViewModels/Watch3D/HelixWatch3DViewModel.cs#L2073

Ellipse is 1 as this is an enum.

* Initial Commit * Add hosts selections * Add the multi selection * Multiple Host Selection Display * Correct Host Dependencies in Json for publishing and download * Code Clean up * More Cleanup and Comments * Bug fixing - publish version * Workaround to enable publishing from sandbox * Update PublishPackageView.xaml * Updates UI * Adds new resources * Updates UI * Adds new close button icons * Updates UI. Add Markdown selection functionality. * Update AssemblySharedInfo.cs * Adding remaining functionality to publish package view * Updates method names * Update PublishPackageView.xaml * Update PublishPackageView.xaml.cs * Adds ToolTip + Info Icon to Markdown Directory label * Fixes regressions * Responds to comments * Cherry-picks edits from PR DynamoDS#12171 * Undoes whitespace changes * Removes duplicate resources Co-authored-by: tanga <aaron.tang@autodesk.com>

* Python editor visual restyle - initial commit - initial commit for python editor visual restyle - for general idea of the desired scope, visit https://www.figma.com/file/1q7EWQGYO7pPDhyLf8nuwW/Python-node-editor-restyling?node-id=0%3A1&t=QGCSTKllX8Q7OnhK-0 * Icon Update - updated python scrip editor icons * Hover icon update - minor update for one of the icons * Change hyperlink color - due to readability issues, changed the hyperlink text color * Text Folding - first implementation working * Custom indentation strategy added - added custom Python indentation strategy based around line ending with a column * TabFoldingStrategy changes, saves on Esc exist, WIP - WIP code, still playing with the avalon editor visual capabilities - Implemented the 'save on exit' (still missing prompt) - Folding strategy works more or less correctly, except we are not tracking folded states (which we can) - more work needed, but should we? * Undo/Redo, Zoom-in/out buttons added - added buttons for the undo/redo and zoom-in/out functionalities * Keywords color update - updated keywords as per latest Figma color scheme * Warning bar on unsaved changes exit added - added a warning message with controls when user tries to exit the Script interface by pressing the Escape button, but has unsaved changes - made Avalon edit support classes Internal * Fix tab folding - now will correctly continue after ":" not being the end of the text line (no tabbing) * Small icons test - testing screen resolution against 24x24 icons * Back to 48x48 icons - updated icons back to 48x48px * Disable UI when prompt to save changes - will disable any user interaction while in 'warning' mode - forces the user to `keep editing` -> `save` -> `close` * Color brushes replaced with Dynamo brushes where possible - re[placed with dynamo library brushes where possible * Localized unsaved changes prompt texts - unsaved changes prompt title and text localized * Cleaned up tabfoldingstrategy comments - removed old or unnecessary comments - kept comments that help to clarify the logic * Update ScriptEditorWindow.xaml

…vent (DynamoDS#14007) * Create dynamoBinDiff.yml * update nuget step * add bin diff job script * add cache test * fixing cache * test * fixing cache #2 * fixing cache #3 * fixing cache DynamoDS#4 * debug * debug * debug * debug * fixed cache * cleanup step * test cleanup * final version * Update bin_diff.ps1 * remove cleanup step, because failing * add colors * Change hashing algo to SHA256 * add net6 win for bin diff * fix * only status

work in progress for review

87c2d11

update shaders use the blendFactor prop on pointGeometry to pack our data material classes are not used

aparajit-pratap reviewed May 30, 2020

View reviewed changes

src/DynamoCoreWpf/ViewModels/Watch3D/shaderSource/vsDynamoPointLine.hlsl Show resolved Hide resolved

aparajit-pratap reviewed May 30, 2020

View reviewed changes

update shaders

2c34bf2

find same buffer slot for lines and points yay refactor shared data core update tests

mjkkirschner commented Jun 1, 2020

View reviewed changes

aparajit-pratap reviewed Jun 1, 2020

View reviewed changes

aparajit-pratap merged commit 483e765 into aparajit-pratap:dyn-2702 Jun 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

work in progress for review #2

work in progress for review #2

mjkkirschner commented May 29, 2020 •

edited

Loading

aparajit-pratap May 30, 2020

aparajit-pratap May 30, 2020

mjkkirschner May 30, 2020 •

edited

Loading

aparajit-pratap commented May 30, 2020

mjkkirschner commented May 30, 2020 •

edited

Loading

aparajit-pratap commented May 31, 2020

aparajit-pratap commented May 31, 2020 •

edited

Loading

mjkkirschner commented May 31, 2020

mjkkirschner commented May 31, 2020 •

edited

Loading

mjkkirschner commented May 31, 2020

aparajit-pratap commented May 31, 2020

mjkkirschner commented May 31, 2020 •

edited

Loading

mjkkirschner commented Jun 1, 2020 •

edited

Loading

mjkkirschner Jun 1, 2020

mjkkirschner Jun 1, 2020

aparajit-pratap Jun 1, 2020

mjkkirschner Jun 1, 2020

aparajit-pratap Jun 1, 2020

mjkkirschner Jun 1, 2020

aparajit-pratap commented Jun 1, 2020

aparajit-pratap commented Jun 1, 2020 •

edited

Loading

mjkkirschner commented Jun 1, 2020

mjkkirschner commented Jun 1, 2020 •

edited

Loading

		@@ -13,6 +13,17 @@ protected override SceneNode OnCreateSceneNode()
		{
		return new DynamoLineNode();

work in progress for review #2

work in progress for review #2

Conversation

mjkkirschner commented May 29, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mjkkirschner May 30, 2020 • edited Loading

Choose a reason for hiding this comment

aparajit-pratap commented May 30, 2020

mjkkirschner commented May 30, 2020 • edited Loading

aparajit-pratap commented May 31, 2020

aparajit-pratap commented May 31, 2020 • edited Loading

mjkkirschner commented May 31, 2020

mjkkirschner commented May 31, 2020 • edited Loading

mjkkirschner commented May 31, 2020

aparajit-pratap commented May 31, 2020

mjkkirschner commented May 31, 2020 • edited Loading

mjkkirschner commented Jun 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aparajit-pratap commented Jun 1, 2020

aparajit-pratap commented Jun 1, 2020 • edited Loading

mjkkirschner commented Jun 1, 2020

mjkkirschner commented Jun 1, 2020 • edited Loading

mjkkirschner commented May 29, 2020 •

edited

Loading

mjkkirschner May 30, 2020 •

edited

Loading

mjkkirschner commented May 30, 2020 •

edited

Loading

aparajit-pratap commented May 31, 2020 •

edited

Loading

mjkkirschner commented May 31, 2020 •

edited

Loading

mjkkirschner commented May 31, 2020 •

edited

Loading

mjkkirschner commented Jun 1, 2020 •

edited

Loading

aparajit-pratap commented Jun 1, 2020 •

edited

Loading

mjkkirschner commented Jun 1, 2020 •

edited

Loading