DYN-7103, DYN-7127 Fix crash error in Instancing Rendering logic, Fix issue with code block rendering #15296
Conversation
There was a problem hiding this comment.
See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-7103
UI Smoke TestsTest: success. 2 passed, 0 failed. |
| for (int i = 0; i < l.Indices.Count; i++) | ||
| { | ||
| l.Indices[i] -= totalRemoved; | ||
| //Reset the Indices values for the indices that were after the removed region |
There was a problem hiding this comment.
Is this a correct statement : the value of l.Indices[ j ] is always smaller then the value of l.Indices[ j+1 ] ?
I'm thinking if there is a way to check the validity of Geometry3D at the end of this operation and throw if something is not right.
There was a problem hiding this comment.
Looked into this and will have to come back to it. There isn't a very predictable try / catch situation upward. I think we will have to come back with that.
There was a problem hiding this comment.
I added the assert in the tests but that is really what we want to eventually check at runtime. indices should not be less then 0 or greater then vertex count.
aparajit-pratap
left a comment
There was a problem hiding this comment.
I'm not familiar with this logic so I'll take your word for it. The only thing I would say is please add a test if possible.
Echo that |
|
If no one can understand the logic in this file, that is not good. It needs a diagram, better comments, something. |
aparajit-pratap
left a comment
There was a problem hiding this comment.
Just one comment about using the correct error message box. Please also add another test case for the code block node case.
… issue with code block rendering (DynamoDS#15296) (cherry picked from commit df3a852)
Purpose
This PR corrects an error in the instancing render logic that can cause a crash. Specifically the fix adjusts the logic that is used when the
HelixWatch3DViewModelis extracting regions of line vertices, colors, and indices associated with goemetry that will be rendered with instancing mechanism. The logic error occurred when the remaining indices are reset. Previous every indices was reset vs only those after the removed regions.sudo example
Indices List before remove [1,2,3,4,5,6,7,8]
Indices List after remove [1,2,7,8]
Indices List after reset of values old way [-3,-2,3,4] WRONG
Indices List after reset of values: new way [1,2,3,4] CORRECT
The PR also add additional asserts to validate that the logic does keep the indices with the correct bounds. If the asserts fail then the render skips on completing the render of the geometry and passes a message to the user
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
Fixed an issue where Dynamo would crash when rendering the background preview with the instancing turned on.
Reviewers
@BogdanZavu @mjkkirschner
FYIs
@jnealb @aparajit-pratap @pinzart @QilongTang