feat(picker): Add volume picker to CellPicker by rodrigobasilio2022 · Pull Request #2944 · Kitware/vtk-js

rodrigobasilio2022 · 2023-10-26T14:51:29Z

Context

This PRs adds volume picker capabilities to Cellpicker Object. I recorded a video of the functionality working.

Gravando.2023-10-26.123920.mp4

Results

Changes

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

This change adds or fixes unit tests
Tested environment:
- vtk.js:
- OS:
- Browser:

floryst · 2023-10-26T15:09:49Z

CI is failing due to old node versions. I will update it.

rodrigobasilio2022 · 2023-10-26T15:20:17Z

Ok. Thanks. I tried to make fewer changes so you can review effectively. Do you want me to create a video?

floryst · 2023-10-26T15:28:39Z

If you have an example, that would be a great addition to demonstrate its capabilities.

One note I had skimming your changes is that the package.json changes are irrelevant to the functionality at hand. If the build works then I'm fine with the upgrades.

rodrigobasilio2022 · 2023-10-26T15:30:57Z

THey are irrelevant. I wrongly executed npm install --force, that s it. I will make a video an put in the comments

sedghi · 2023-10-26T17:39:39Z

is this PR related to #2885 ?

floryst · 2023-10-26T17:41:54Z

is this PR related to #2885 ?

That appears to be a separate effort that hasn't been updated.

finetjul

It's not clear why you had to modify package.json / package-lock.json

Please update CellPicker documentation accordingly.

Sources/Rendering/Core/CellPicker/index.js

Sources/Rendering/Core/Picker/index.js

rodrigobasilio2022 · 2023-10-26T21:32:38Z

Can you guys help me in git? I think I am doing a mess here. I want to delete the modifications I did in the package.json files

finetjul

Can you please also update index.d.ts files accordingly ?

Sources/Rendering/Core/CellPicker/index.js

finetjul · 2023-10-27T21:39:09Z

Can you guys help me in git? I think I am doing a mess here. I want to delete the modifications I did in the package.json files

You will be able to squash all your branch commits eventually (I personnally use git rebase -i to do so)

rodrigobasilio2022 · 2023-10-30T13:43:32Z

Added the example. Something i noticed is why you guys do not use the clipLine information in the following lines...

vtk-js/Sources/Rendering/Core/CellPicker/index.js

Line 180 in d61d2e4

const clipLine = clipLineWithPlane(

finetjul · 2023-10-30T14:55:59Z

Added the example. Something i noticed is why you guys do not use the clipLine information in the following lines...

vtk-js/Sources/Rendering/Core/CellPicker/index.js

Line 180 in d61d2e4

const clipLine = clipLineWithPlane(

Not sure what do you mean, we do use clipLine afterwards

rodrigobasilio2022 · 2023-10-30T15:05:10Z

The t1 and t2 updated values that restricts the ray

finetjul · 2023-10-30T15:07:49Z

The t1 and t2 updated values that restricts the ray

I see, looks like a bug indeed. t1 and t2 should be extracted from the returned value...

Sources/Rendering/Core/CellPicker/index.js

Sources/Rendering/Core/Picker/index.js

Examples/Volume/VolumePicker/controlPanel.html

finetjul

LGTM

floryst · 2023-11-02T19:47:26Z

CI is red because this PR has some tests still failing. Please run npm test locally to identify the issue.

rodrigobasilio2022 · 2023-11-02T20:20:00Z

I run the npm run test command and this was the final output

My machine is a Windows 11

rodrigobasilio2022 · 2023-11-02T20:27:17Z

I executed the command again. In fact was executed yarn run test, and that was the output

finetjul · 2023-11-03T07:48:57Z

It looks like a test is hanging (that's why you do not see any failure)

rodrigobasilio2022 · 2023-11-03T07:54:32Z

How do I solve this problem?

rodrigobasilio2022 · 2023-11-03T14:51:54Z

Found the problem. It was a point picker function which not correspond with the changes in vtkPicker

rodrigobasilio2022 · 2023-11-03T19:24:10Z

@finetjul I nned to do something else? I saw the PR was pulled out from merge queue, despite no conflict with the base branch

floryst · 2023-11-03T19:46:27Z

Can you rebase locally onto an updated master to see if there are any conflicts? Github recently had operational issues, so I don't know if there is an actual conflict or if the merge queues are currently broken.

rodrigobasilio2022 · 2023-11-03T20:33:58Z

@floryst Made the change. It had one commit out from the base branch, despite no warnings in this PR. Please try again...

rodrigobasilio2022 · 2023-11-04T07:16:22Z

@floryst @finetjul Now i think i got it right

finetjul

LGTM

rodrigobasilio2022 · 2023-11-05T14:23:56Z

@finetjul I think the branch is ok right. It already passed theses tests?

github-actions · 2023-11-05T21:11:45Z

🎉 This PR is included in version 29.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

floryst requested a review from finetjul October 26, 2023 15:09

finetjul reviewed Oct 26, 2023

View reviewed changes

finetjul reviewed Oct 27, 2023

View reviewed changes

Sources/Rendering/Core/CellPicker/index.js Show resolved Hide resolved

rodrigobasilio2022 requested a review from finetjul October 31, 2023 17:02

finetjul reviewed Oct 31, 2023

View reviewed changes

Sources/Rendering/Core/CellPicker/index.js Outdated Show resolved Hide resolved

Sources/Rendering/Core/CellPicker/index.js Outdated Show resolved Hide resolved

Sources/Rendering/Core/Picker/index.js Outdated Show resolved Hide resolved

finetjul reviewed Nov 1, 2023

View reviewed changes

Examples/Volume/VolumePicker/controlPanel.html Show resolved Hide resolved

rodrigobasilio2022 requested a review from finetjul November 1, 2023 20:26

finetjul approved these changes Nov 2, 2023

View reviewed changes

rodrigobasilio2022 requested a review from finetjul November 3, 2023 16:18

finetjul added this pull request to the merge queue Nov 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 3, 2023

floryst added this pull request to the merge queue Nov 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 3, 2023

floryst added this pull request to the merge queue Nov 3, 2023

github-merge-queue bot removed this pull request from the merge queue due to a conflict with the base branch Nov 3, 2023

rodrigobasilio2022 and others added 12 commits November 4, 2023 04:03

feat(picker): Add volume picker to CellPicker

ba6f843

feat(sort): Add sorting in pickedPositions

021549a

refactor(function): Refactor function parameters

5c8ec1d

refactor(file): Restore package-lock.json

5dc2ab3

refactor(file): Delete unwanted package edition

b48af92

refactor(package): Restore package-lock.json

e5cc1d6

feat(volumePicker): add volume picker example

f6b12b3

refactor(import): Remove unused import

20afba4

refactor(code): remove unused variables

f8ea289

refactor(code): getOpacity call in cellpicker

ff27558

refactor(code): add slider in VolumePicker example

d9a9829

refactor(picker): Refactor point picker function

c93a020

rodrigobasilio2022 force-pushed the feat/VolumePicker branch from c55057d to c93a020 Compare November 4, 2023 07:04

finetjul approved these changes Nov 5, 2023

View reviewed changes

finetjul added this pull request to the merge queue Nov 5, 2023

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 5, 2023

finetjul added this pull request to the merge queue Nov 5, 2023

Merged via the queue into Kitware:master with commit c03e86e Nov 5, 2023

github-actions bot added the released Automated label label Nov 5, 2023

rodrigobasilio2022 deleted the feat/VolumePicker branch January 30, 2026 17:23

Uh oh!

Conversation

rodrigobasilio2022 commented Oct 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Results

Changes

PR and Code Checklist

Testing

Uh oh!

floryst commented Oct 26, 2023

Uh oh!

rodrigobasilio2022 commented Oct 26, 2023

Uh oh!

floryst commented Oct 26, 2023

Uh oh!

rodrigobasilio2022 commented Oct 26, 2023

Uh oh!

sedghi commented Oct 26, 2023

Uh oh!

floryst commented Oct 26, 2023

Uh oh!

finetjul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rodrigobasilio2022 commented Oct 26, 2023

Uh oh!

finetjul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

finetjul commented Oct 27, 2023

Uh oh!

rodrigobasilio2022 commented Oct 30, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

finetjul commented Oct 30, 2023

Uh oh!

rodrigobasilio2022 commented Oct 30, 2023

Uh oh!

finetjul commented Oct 30, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

finetjul left a comment

Choose a reason for hiding this comment

Uh oh!

floryst commented Nov 2, 2023

Uh oh!

rodrigobasilio2022 commented Nov 2, 2023

Uh oh!

rodrigobasilio2022 commented Nov 2, 2023

Uh oh!

finetjul commented Nov 3, 2023

Uh oh!

rodrigobasilio2022 commented Nov 3, 2023

Uh oh!

rodrigobasilio2022 commented Nov 3, 2023

Uh oh!

rodrigobasilio2022 commented Nov 3, 2023

Uh oh!

floryst commented Nov 3, 2023

Uh oh!

rodrigobasilio2022 commented Nov 3, 2023

Uh oh!

rodrigobasilio2022 commented Nov 4, 2023

Uh oh!

finetjul left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rodrigobasilio2022 commented Nov 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rodrigobasilio2022 commented Oct 26, 2023 •

edited

Loading

rodrigobasilio2022 commented Oct 30, 2023 •

edited

Loading

rodrigobasilio2022 commented Nov 5, 2023 •

edited

Loading