-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
show_frame: Use workaround updateFrame
pending upstream merge
#11134
show_frame: Use workaround updateFrame
pending upstream merge
#11134
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+@jwnimmer-tri for both reviews, please.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing (waiting on @jwnimmer-tri)
59eb46d
to
8db3a4f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TBH None of this code is automatically tested so I'm hesitant to patch this without upstream merging the fix first, in case the fix isn't really good enough, or has other problems. And I'm not planning on rebuilding and running the demo where I saw this anytime soon.
Reviewed 1 of 1 files at r1.
Reviewable status: all discussions resolved, LGTM missing from assignee jwnimmer-tri, platform LGTM missing
SGTM. I think we should up the priority if another user runs into it, but I'm fine with this otherwise chilling pending upstream updates. |
(Or closing this for the time being, as long as it's easy to refer back to.) |
Sure. They would be in a good position to test it, in that case. |
Aye. Closing, and will post a tracer-comment in |
Upstream PR:
RobotLocomotion/director#621
For issue, see Slack discussion:
https://drakedevelopers.slack.com/archives/C43KX47A9/p1554239704004700
EDIT (2019/10/07):
Copy of Slack transcript:
This change is