Skip to content
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

Add vtk threshold point #3081

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

finetjul
Copy link
Member

@finetjul finetjul commented Jun 8, 2024

Context

Add vtkThresholdPoints filter.

Results

A new filter with an example are added.

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: master
    • OS: Windows
    • Browser: Chrome

@finetjul finetjul requested a review from floryst June 8, 2024 18:22
outData[0] = output;

if (model.criterias.length === 0) {
output.shallowCopy(input);
Copy link
Member Author

@finetjul finetjul Jun 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@floryst @jourdain I was surprised to have to shallow copy.
If I simply do outputData[0] = input, then the shader may not refresh.
This no-refresh happens when the threshold filter has previously generated a new ouput polydata and now it simply returns an old polydata. There must be some kind of time stamp issue. I'm not sure if that should be fixed instead in the mapper. WDYT ?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably b/c input.getMTime() is older than output.getMTime() at that point, so there's probably a shouldUpdate flag set to false in the vtk algorithm macro. outputData[0] = input would probably work if input.modified() was called as well, but that's probably undesirable, so shallowCopy seems fine to me.

@floryst
Copy link
Collaborator

floryst commented Jun 10, 2024

Are you planning on adding typescript definitions as well?

@finetjul
Copy link
Member Author

Are you planning on adding typescript definitions as well?

I was not planning to because I do not use typescript. Shall we make it mandatory for new contributions ?

@floryst
Copy link
Collaborator

floryst commented Jun 10, 2024

I was not planning to because I do not use typescript. Shall we make it mandatory for new contributions ?

It's a good idea to do so nowadays, especially since vtk.js is ingested by typescript projects.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants