[render_vtk] Add gflag for show_window #20143
Conversation
jwnimmer-tri
added
priority: backlog
status: single reviewer ok
There was a problem hiding this comment.
I've been contemplating doing this for a while. 
However, as I spend a fair amount of time in this code, I'll tell you a couple of other things I frequently toggle when I toggle this:
- I tend to put a sleep statement so that the image stays open for a while. I'd recommend a flag with some default value that only is engaged if a
show_fooistruebut allow the user to tweak it to suit their workflow. - I frequently find that I only want to show the rgb image and not the label image. So, I frequently create a custom
label_camerawhich is distinct from thecolor_camerain thatshow_windowis hard-coded to false. (I can also imagine simply making them distinct flagsshow_rgbandshow_label.)
Both of those impact the Render() method. If that was a bit garbled, I can push a commit and you can decide what you think. Let me know.
Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status:complete! all discussions resolved, LGTM from assignee SeanCurtis-TRI(platform)
There was a problem hiding this comment.
(Second thought...I'd do the same thing in the internal_render_engine_gl_test.cc for parity.)
Reviewable status:
There was a problem hiding this comment.
re: CI. I believe if you rebase this PR, the CI failure will go away.
Reviewable status:
jwnimmer-tri
force-pushed
the
vtk-test-show-window
branch
from
262c74f to
f6cb874
Compare
|
I've added the Thanks for the info on sleeping, but I'm not going to do it in this PR. At minimum, the user could set a breakpoint on what they want and the window would stay open. Also |
There was a problem hiding this comment.
Yes. maybe_pause_for_user.h is probably a better solution.
Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status:
|
Looks like the change actually did cause the unit test to be more fussy on macOS arm64. I'll add that test case to the skip-list as well. |
There was a problem hiding this comment.
That test is removed from the list in master. I don't think this PR is based on the most recent master.
Reviewable status:
There was a problem hiding this comment.
That is to say, I thought it was. I have a branch that has it and I think I had my wires crossed.
Reviewable status:
|
Jenkins always merges PRs up to the tip of master before running any builds. |
jwnimmer-tri
force-pushed
the
vtk-test-show-window
branch
from
f6cb874 to
4a7071f
Compare
There was a problem hiding this comment.
Yeah.. Dur.
Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: labeled "do not merge" (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
And now it's the MultiLights test.
Reviewable status: labeled "do not merge" (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
(This is probably making your argument that they should all be excluded by default and tests have to be explicitly included.)
Reviewable status: labeled "do not merge" (waiting on @jwnimmer-tri)
It's nice to be able to debug show_window without rebuilding. Adjust the vtk unit test rule for macOS tests to opt-in instead of opt-out. While pushing this commit through CI, we saw even more spurious failures.
jwnimmer-tri
force-pushed
the
vtk-test-show-window
branch
from
4a7071f to
1f7e966
Compare
There was a problem hiding this comment.
Yup, I've swapped it to opt-in now.
-(status: do not merge)
Reviewable status: labeled "do not merge" (waiting on @jwnimmer-tri)
There was a problem hiding this comment.
Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status:
It's nice to be able to debug
show_windowwithout rebuilding. This came up during #19945.+@SeanCurtis-TRI for both reviews, please.
This change is