Implement vtkQuad

[WesleyTheGeolien]

Context

This MR aims to implement the vtkQuad class as suggested by @finetjul in #2508

Changes

Created vtkQuad class. Type script updated but unsure on how to change documentation?

• Documentation and TypeScript definitions were updated to match those changes

PR and Code Checklist

• semantic-release commit messages
• Run npm run reformat to have correctly formatted code

Testing

Added unit tests but two are failing for the pcoords, I have copied the behaviour in the cxx library but the tests are from the vtkTriangle vtk.js class so they may be incorrect, I am not familiar enough with how these are calculated as to whether they are correct. Maybe @finetjul you could take a look?

• This change adds or fixes unit tests
• Tested environment:
  • vtk.js: latest
  • OS: Mac OS 12.4
  • Browser: Chrome (through karma)

[WesleyTheGeolien]

There is some small cleanup still todo around the outObj but wanted to check globally everything is inline

[WesleyTheGeolien]

No I was leaving it more in there as a stub in case there ever was any static methods to export. Ca easily remove though.

…

On 15 Jul 2022, at 17:21, Forrest Li ***@***.***> wrote:  @floryst commented on this pull request. In Sources/Common/DataModel/Quad/index.js: > @@ -0,0 +1,265 @@ +import macro from 'vtk.js/Sources/macros'; +import vtkCell from 'vtk.js/Sources/Common/DataModel/Cell'; +import * as vtkMath from 'vtk.js/Sources/Common/Core/Math'; +import { CellType } from 'vtk.js/Sources/Common/DataModel/CellTypes/Constants'; +import vtkTriangle from 'vtk.js/Sources/Common/DataModel/Triangle'; +import vtkPoints from 'vtk.js/Sources/Common/Core/Points'; + +// ---------------------------------------------------------------------------- +// Static API +// ---------------------------------------------------------------------------- + +export const STATIC = {}; If there is no exportable static API, then you can leave this out. Were you planning on adding some static methods? — Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

[floryst]

Let's remove the STATIC export then. No need to have this explicit placeholder if we don't have an intent on populating it any time soon.

[WesleyTheGeolien]

Could someone please take a look at the pCoords in the tests that are failing? I am unsure what the "correct" values of these should be.

in testQuad.js:

• line 49 I am getting [1,1,0] instead of [0,1,0]
• line 69 I am getting [1,1,0] instead of [0,0,0]

[WesleyTheGeolien]

Hey guys, wondering if someone could give me a hand with the pCoords so this can land?

[WesleyTheGeolien]

Thanks @finetjul, I've taken into account your remarks. The pCoords test however are still off

[DavidBerger98]

Thanks @finetjul, I've taken into account your remarks. The pCoords test however are still off

On line 49 [1,1,0] is the correct value. So that should be the expected one.

The assertion on line 69 is not needed, since there was a noIntersection and thus pcoords was never recomputed. You can thus also remove the assertion on line 68.

[WesleyTheGeolien]

Thanks @DavidBerger98 have taken into account your changes and the tests are now passing.

I believe this should be ready to merge now?

[finetjul]

I believe you can now add support for vtkQuad cell picking. See here

If you can squash all the commits into a unique commit, it would be great.

[WesleyTheGeolien]

I believe you can now add support for vtkQuad cell picking. See here

If you can squash all the commits into a unique commit, it would be great.

@finetjul I believe I have already implemented this and added the tests: https://github.com/Kitware/vtk-js/pull/2515/files#diff-dcd608a8f684922521c178b9a2965eccf3cadbedbfc27d680d70fe788e11b912

Just need to squash all commits I guess do I do this manually or do you guys do this on merge with something like: https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/incorporating-changes-from-a-pull-request/about-pull-request-merges#squash-and-merge-your-pull-request-commits

[finetjul]

I believe I have already implemented this and added the tests

My bad, for some reasons I forgot about it. You are correct, it's all fine !

Just need to squash all commits I guess do I do this manually or do you guys do this on merge

We can do it automatically like you suggest but we prefer to do a merge commit. It's also better to squash manually because you have more controls on the commit title and body message.

[WesleyTheGeolien]

@finetjul Right that should have squashed all commits.

[finetjul]

Thanks @WesleyTheGeolien