Skip to content

ENH: Allow virtual func to be called during MRML(ThreeD|Slice)View pimpl init#7325

Merged
jcfr merged 2 commits intoSlicer:mainfrom
jcfr:allow-virtual-func-in-MRMLSliceViewPrivate-and-qMRMLThreeDViewPrivate-init
Oct 31, 2023
Merged

ENH: Allow virtual func to be called during MRML(ThreeD|Slice)View pimpl init#7325
jcfr merged 2 commits intoSlicer:mainfrom
jcfr:allow-virtual-func-in-MRMLSliceViewPrivate-and-qMRMLThreeDViewPrivate-init

Conversation

@jcfr
Copy link
Member

@jcfr jcfr commented Oct 30, 2023

The CTK update adds protected constructor to ctkVTKRenderView and ctkVTKSliceView for associating a derived pimpl.

This is required to have the complete virtual function chain executed when virtual function are called in the qMRMLThreeDViewPrivate::init() or qMRMLSliceViewPrivate::init() function.

jcfr added 2 commits October 30, 2023 16:21
@jcfr
ENH: Update CTK to allow improved ctkVTK(Render|Slice)View pimpl deri…
34580f6 
…vation

Add protected constructor to ctkVTKRenderView and ctkVTKSliceView for
associating a derived pimpl.

This is required to have the complete virtual function chain executed
when virtual function are called in the pimpl init() function.

See commontk/CTK@4888deb...f53820a

List of CTK changes:

$ git shortlog 4888deb1d..f53820a4e --no-merges
Jean-Christophe Fillion-Robin (2):
      STYLE: Add missing override keyword to ctkVTK(Render|Slice)View classes
      ENH: Ensure virtual functions can be called from derived ctkVTK view pimpl
@jcfr
ENH: Allow virtual func to be called during MRML(ThreeD|Slice)View pi…
368ad56 
…mpl init
@jcfr jcfr requested review from lassoan and pieper October 30, 2023 20:26
@jcfr jcfr mentioned this pull request Oct 30, 2023
Merged
pieper
pieper approved these changes Oct 30, 2023
View reviewed changes
Copy link
Member

@pieper pieper left a comment

Choose a reason for hiding this comment

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

I'm not following closely but if this works for you I'm fine with it.

lassoan
lassoan approved these changes Oct 30, 2023
View reviewed changes
Copy link
Contributor

@lassoan lassoan left a comment

Choose a reason for hiding this comment

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

Same here. Looks good overall.

@jcfr jcfr enabled auto-merge (rebase) October 30, 2023 20:51
@jcfr jcfr disabled auto-merge October 30, 2023 23:00
@jcfr
Copy link
Member Author

jcfr commented Oct 30, 2023 •
edited
Loading

This pull request updates CTK in the first commit and update the classes qMRMLThreeDView and qMRMLSliceView in a second commit, that said the check1 attempting to detect SuperBuild changes does not properly see that the first commit is updating External_CTK.cmake.

For this reason, later this evening and before our nightly build starts, I will bypass out usual check and move forward with the integration.

This can be see the build log2 associated with the CI (build) workflow (also copied below for future reference).

CI (Build) Complete list of updated files 3
image image

I think we should also look for updates our check with something like this:

      - name: 'Build Slicer'
        id: slicer-build
-        if: steps.changes.outputs.paths-to-include == 'true'
+        if: steps.changes.outputs.paths-to-include == 'true' and steps.changes-superbuild.outputs.paths-to-include == 'false'
        uses: ./.github/actions/slicer-build

cc: @jamesobutler

Footnotes

  1. https://github.com/Slicer/Slicer/blob/09a0e50583f85ad80e09f8fbc2a0c115a1bb0f79/.github/workflows/ci.yml#L24-L38 ↩

  2. https://github.com/Slicer/Slicer/actions/runs/6698340032/job/18200163646?pr=7325#step:3:33 ↩

  3. https://github.com/Slicer/Slicer/pull/7325/files ↩

@jcfr jcfr merged commit 6b3228b into Slicer:main Oct 31, 2023
@jcfr jcfr deleted the allow-virtual-func-in-MRMLSliceViewPrivate-and-qMRMLThreeDViewPrivate-init branch October 31, 2023 00:04
@jcfr jcfr added the backport:5.x Identify pull request expected to be backported to the current 5.x release branch. label Nov 5, 2023
@jcfr jcfr removed the backport:5.x Identify pull request expected to be backported to the current 5.x release branch. label Nov 15, 2023
This was referenced Dec 21, 2023
Merged
Closed
@jcfr jcfr mentioned this pull request Dec 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pieper pieper pieper approved these changes

@lassoan lassoan lassoan approved these changes

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jcfr @pieper @lassoan