-
Notifications
You must be signed in to change notification settings - Fork 48
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
minor tweaks to support headless testing #88
Conversation
Related to RobotLocomotion/drake#13038. |
1) Allows one to optionally pass a renderer to the Viewer constructor 2) Count the number of messages received 3) Use window.CCapture instead of CCapture, as recommended in https://stackoverflow.com/questions/48864251/how-to-run-ccapture-by-server-side-node-js (and it still works in the normal browser)
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.
Took a look and looks good for merge with a one nit, and with one caveat: the ccapture change is pretty mysterious to me, but I trust it fixed a problem for you, and it looks innocuous as long as it still runs and passes tests.
this.renderer.shadowMap.type = THREE.PCFSoftShadowMap; | ||
this.dom_element.appendChild(this.renderer.domElement); | ||
} else { | ||
this.renderer = renderer; |
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.
If it's possible, asserting renderer
is the right type here might prevent really confusing deferred errors later if someone shoves in a bad renderer type (e.g. accidentally invokes the constructor with a weird extra argument)?
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.
That's a good point, although I don't actually know what the right assertion would be, since javascript typing is kind of the wild west. I'm ok with leaving this open-ended for now.
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.
👍
By the way, I stalled on updating the |
https://stackoverflow.com/questions/48864251/how-to-run-ccapture-by-server-side-node-js
(and it still works in the normal browser)