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

Update VTK from slicer-v9.2.20230607-1ff325c54 to a recent version #7488

Open
jcfr opened this issue Dec 18, 2023 · 0 comments
Open

Update VTK from slicer-v9.2.20230607-1ff325c54 to a recent version #7488

jcfr opened this issue Dec 18, 2023 · 0 comments
Milestone

Comments

@jcfr
Copy link
Member

jcfr commented Dec 18, 2023

Is your feature request related to a problem? Please describe.

The version currently integrated in Slicer corresponds to the branch Slicer/VTK@slicer-v9.2.20230607-1ff325c54-21 and is based of Kitware/VTK@1ff325c from June 2023.

While selected fixes and improvement have been integrated through the pull requests listed below, we are missing out on additional fixes and improvements as well as increasing the maintenance burden as time goes.

The last VTK update was done through this pull request:

Describe the solution you'd like

We should create a new branch names like slicer-v9.3.2024MMDD-abcdefghi with only a few Slicer specific changes.

⚠️ While working on #7487, a performance regression related to vtkRender::GetZ() was identified, we should make sure to review its status when working on updating VTK:

Describe alternatives you've considered

NA

Additional context

Here are the pull requests backporting changes (from the most recent to the oldest) since the last update through #7010:

Below a comment originally posted by @lassoan in #7487 (comment)

The change in Kitware/VTK@cff3d9a60 is not very useful, as it is extremely expensive to instantiate a new hardware picker for every position. This is also something that anyone can easily do outside the renderer and actually do that picker more efficiently (e.g., cache the Z buffer content). I think the original behavior of vtkRenderer::GetZ should be restored, and its documentation should be amended with a note that the method should be called at the right time (when the Z buffer is not cleared).

The proposed GetZ implementation might be considered to be added to the vtkRenderer as some convenience function if Paulo Carvalho found it useful, but it should be reviewed, because for example creating a new hardware selector from scratch for every point position looks terribly wasteful, especially when the renderer already has a cached selector (and maybe vtkRenderer::PickProp could be enough). I think overall what happened is not Paulo's fault but that VTK developers did not review his proposed change for a really long time and then just rushed a merge without anyone really thinking about the implications of this change. Probably the change looked insignificant because nobody realized that the method was heavily used in vtkWorldPointPicker and vtkSelectVisiblePoints (and it worked correctly there because the method was used at the right time).

Footnotes

  1. https://github.com/Slicer/VTK/tree/slicer-v9.2.20230607-1ff325c54-2

@jcfr jcfr added this to the Slicer 5.7 milestone Dec 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant