Avoid Helix crash due to edge double values #11031
Conversation
This prevents the display of geometry located in positions considered invalid by Helix. This includes the special values PositiveInfinity, NegativeInfinity and NaN. Also NaN is checked as a special value when unmarhsalling data from an FFI. Special values PositiveInfinity and NegativeInfinity were already handled. This allows to generate a warning over the node with an appropriate message.
| if (!package.Points.Positions.Any(v => v.IsInvalid()) && | ||
| !package.Lines.Positions.Any(v => v.IsInvalid()) && | ||
| !package.Mesh.Positions.Any(v => v.IsInvalid())) | ||
| { |
There was a problem hiding this comment.
I wonder if this will cause a significant slowdown.
There was a problem hiding this comment.
it sure seems like it could - for an alternative, what about checking this during package creation, since at that point we have to iterate over every vertex anyway.
There was a problem hiding this comment.
The checks should be quick. The amount will grow with geometry, but should be a very low percentage in comparison.
@mjkkirschner What do you mean by package creation? To check it as soon as the geometry is added to the package?
There was a problem hiding this comment.
yes, exactly, since we already need to iterate over each vertex at that point. It may not be possible though if some render packages are created in LibG code.
There was a problem hiding this comment.
GetRenderPackagesFromMirrorData is the function I am thinking of.
There was a problem hiding this comment.
If I read this correctly, I think it should be good: https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoPerformance-CI/job/master/514/
There was a problem hiding this comment.
unfortunately, these tests don't tessellation or view interactions at the moment. 😢
There was a problem hiding this comment.
I thought @QilongTang added the ability to measure tessellation performance as well: #9651
There was a problem hiding this comment.
Did a couple of runs with the tests I added here. I would say the results were not conclusive:
With a sampling of 3, total time in seconds compiling assets for rendering:
First run:
Master: 60.68702
Branch: 63.96409
Second run:
Master: 62.36558
Branch: 58.82862
Theoretically, doing the check first or later should not make a difference. This is still o(n) based on the size of the packages collection. Also, not sure if it's a good idea to change GetRenderPackagesFromMirrorData for a Helix-specific check. I would say to leave it like this unless there are any objections.
| // Nodes should be set to warning status with an appropriate message | ||
| var nanNode = Model.CurrentWorkspace.Nodes.First(n => n.Name == "NaN"); | ||
| Assert.AreEqual(ElementState.Warning, nanNode.State); | ||
| StringAssert.Contains("'NaN' is being cast to 'Double'", nanNode.ToolTipText); |
There was a problem hiding this comment.
@mmisol are these warning messages coming from .net or are we creating them?
There was a problem hiding this comment.
They come from us, from CLRObjectMarshaller.
Purpose
This prevents the display of geometry located in positions considered
invalid by Helix. This includes the special values PositiveInfinity,
NegativeInfinity and NaN.
Also NaN is checked as a special value when unmarhsalling data from an
FFI. Special values PositiveInfinity and NegativeInfinity were already
handled. This allows to generate a warning over the node with an
appropriate message.
Here is a screenshot of how the message looks:
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@mjkkirschner @aparajit-pratap
FYIs
@DynamoDS/dynamo