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

feat(vtkscalarbaractor): adds generateTicks field to vtkScalarBarActor #2375

Merged
merged 8 commits into from May 16, 2022

Conversation

NGimbal
Copy link
Contributor

@NGimbal NGimbal commented Apr 19, 2022

Context

I'd like to enable customizing ticks in vtkScalarBarActor to do things like the following:
image

Discussed approach with @martinken on Discourse thread here.

Results

The approach I took was to create a new generateTicks field that is implemented similarly to autoLayout.

I ended up adding ticks and tickstrings setters / getters to the vtkScalarBarActorHelper publicAPI where those values had been private to the vtkScalarBarActorHelper model previously. The generateTicks function then has to set those values. This is very similar to the current implementation of autoLayout, although probably could be improved.

Here are results from my client app:
Screen Shot 2022-04-19 at 2 12 15 PM

Setting a new generateTicks function requires the user to update the vtkScalarBarActor annotations. This also seems to be true for other similar functions, setAxisLabel for example. This is how I'm doing that:

      const mapper = scalarBarActor.getMapper()
      if (mapper) {
        mapper.getLookupTable().resetAnnotations()
      }

Changes

  • Adds new generateTicks field to vtkScalarBarActor
  • Added test for new generateTicks initial value
  • 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

Adds new test testScalaBarSetGenerateTicks.js, tests against image testScalarBarSetGenerateTicks.png.

  • This change adds or fixes unit tests
  • Tested environment:
    • vtk.js: latest master
    • OS: macOS Monterey 12.3.1
    • Browser: Chrome Version 100.0.4896.127 (Official Build) (x86_64)

Funding

This contribution is funded by Pollination.

@martinken
Copy link
Member

martinken commented Apr 19, 2022

Very nice MR! I suspect the failing test is due to font differences. We need to add another valid image (see testScalarBar.js for how to have multiple baselines) To get the image that failed you can go to actions at the top of this page and then click on your failed workflow and then download the tests artifact (zip file) and open that in a browser, click on the hide/show browser tests and the results should show up. If you get stuck just holler or check this page https://docs.github.com/en/actions/managing-workflow-runs/downloading-workflow-artifacts

The only other minor thing I see is since we are making tickstrings public let's change the capitalization to be tickStrings and getTickStrings() (tickstrings was my mistake, but let's fix it now if that's OK)

@martinken
Copy link
Member

artifacts will look like

image

@NGimbal NGimbal marked this pull request as draft April 19, 2022 19:33
@NGimbal
Copy link
Contributor Author

NGimbal commented Apr 19, 2022

@martinken Thanks - I'll add the images from the failing tests as you describe (I was wondering where the multiple baseline images came from!) then convert back to a reviewable PR.

@NGimbal NGimbal marked this pull request as ready for review April 19, 2022 21:28
@finetjul finetjul requested a review from martinken April 20, 2022 20:44
Copy link
Member

@finetjul finetjul left a comment

Choose a reason for hiding this comment

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

LGTM

@boucaud
Copy link
Contributor

boucaud commented May 13, 2022

@finetjul @NGimbal what is the status on this ? This will affect a project I'm working on now and I would like to start the integration.

@NGimbal
Copy link
Contributor Author

NGimbal commented May 13, 2022

@boucaud - I think this is ready to merge pending @martinken's review.

@finetjul finetjul merged commit d4a9829 into Kitware:master May 16, 2022
@github-actions
Copy link

🎉 This PR is included in version 24.11.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot added the released Automated label label May 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
released Automated label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants