feat(polydatacellnormals): compute cell normals #2436
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
finetjul
reviewed
May 31, 2022
| @@ -105,6 +153,13 @@ function vtkPolyDataNormals(publicAPI, model) { | |||
|
|
|||
| output.getPointData().setNormals(outputNormals); | |||
|
|
|||
| if (model.computeCellNormals) { | |||
| const outputCellNormalsData = | |||
| publicAPI.vtkPolyDataCellNormalsExecute(input); | |||
There was a problem hiding this comment.
it's too bad you are recomputing the normals that were computed when doing points normals.
Can you please modify vtkPolyDataNormalsExecute to optionally return cells normals ?
finetjul
reviewed
May 31, 2022
| CELL: 'cell', | ||
| POINT: ' point', | ||
| }; | ||
|
|
|
Can you please also update index.js.ts with the new set/get accessors ? |
finetjul
reviewed
May 31, 2022
| numberOfPoints = polysData[c]; | ||
|
|
||
| if (numberOfPoints < 3) { | ||
| continue; // eslint-disable-line |
finetjul
reviewed
May 31, 2022
| (v1[2] + v2[2] + v3[2]) / 3, | ||
| ]; | ||
|
|
||
| const line = vtkLineSource.newInstance({ |
There was a problem hiding this comment.
how about an arrow source instead ?
(alternatively, you can consider vtkGlyphMapper3D)
finetjul
reviewed
May 31, 2022
| (v1[0] + v2[0] + v3[0]) / 3, | ||
| (v1[1] + v2[1] + v3[1]) / 3, | ||
| (v1[2] + v2[2] + v3[2]) / 3, | ||
| ]; |
There was a problem hiding this comment.
Looks like a good candidate to be exposed publicly in vtkTriangle as a static function.
|
@emil-peters have you had a chance to look at the suggestions ? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Context
closes #2434
Gives the option through
setComputeCellNormalsto compute the cell normals for polyData using thevtkPolyDataNormalsfilter.Results
Changes
polyData.getCellData().getNormals().getData()vtkPolyDataNormalsfilter to compute the cell normals-> Question related to this: can you autogenerate TS-def files? Or should these be created manually?
PR and Code Checklist
npm run reformatto have correctly formatted codeTODO
I was thinking it might make sense to either change the
publicAPI.vtkPolyDataNormalsExecuteto accept only one parameter, which would bevtkPolyData.Or to merge
publicAPI.vtkPolyDataNormalsExecuteandpublicAPI.vtkPolyDataCellNormalsExecuteinto one method, as there is quite some duplication in both?I don't know what would be preferred.
Update the example to switch between cell and vertex normals