New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement vtkQuad #2515
Implement vtkQuad #2515
Conversation
There is some small cleanup still todo around the outObj but wanted to check globally everything is inline |
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.
|
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. |
Could someone please take a look at the in testQuad.js:
|
Hey guys, wondering if someone could give me a hand with the pCoords so this can land? |
Thanks @finetjul, I've taken into account your remarks. The |
On line 49 The assertion on line 69 is not needed, since there was a noIntersection and thus |
Thanks @DavidBerger98 have taken into account your changes and the tests are now passing. I believe this should be ready to merge now? |
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 |
My bad, for some reasons I forgot about it. You are correct, it's all fine !
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. |
c9d65ad
to
eac6167
Compare
@finetjul Right that should have squashed all commits. |
Thanks @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?
PR and Code Checklist
npm run reformat
to have correctly formatted codeTesting
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?