-
Notifications
You must be signed in to change notification settings - Fork 455
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
added configurable parentElem and preventButton #77
Conversation
this.renderer = renderer; | ||
this.effect = effect; | ||
this.distorter = new CardboardDistorter(renderer); | ||
this.button = new ButtonManager(); | ||
this.button = !preventButton && new ButtonManager(); |
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.
I don't like this conditionally created ButtonManager. Let's keep it always created.
@borismus thank you for reviewing and considering my PR with your agreeable response. Changes I made are motivated by subjectiveness. For example, window is defined with innerWidth and document elements are defined with offsetWidth which in my experience is more consistently handled by browsers than outerWidth. In my full preference an element (not window) would always be used as the parent because this would be simpler than supporting both window or element. The parent element would always be accessed from the DOM with an id value to ensure the parent element is still present in the document at each time reference is needed. For those who would build their own system of video player controls, it would be best if the boilerplate ui were separated and more easily bypassed. |
Please address my inline comments, and rebase against latest changes. |
31a5501
to
ade4bcd
Compare
I rebased my branch named parentElem-configure, https://github.com/bumblehead/webvr-boilerplate/tree/parentElem-configure I removed the button related changes, but added a new change which I hope you will agree with. this.parentEl is params.parentEl or document.body (window is no longer used). |
Outdated |
Hi there :) |
Please rebase and make small focused changes. The original PR here had a lot of seemingly unrelated changes. |
Hi, I'm also interested in this issue and agree with what @bumblehead says about always using a dom element as the parent and separating out the UI. As mentioned by @pindiespace in immersive-web/webvr-polyfill#23 it sounds like a significant overhaul is needed. To second @abuisine, what is the plan? Perhaps if we can split it up into sub-tasks we can each take one on? |
This is in large part solved with 1.0 WebVR polyfill. |
Cool, do you mean on this branch? https://github.com/borismus/webvr-polyfill/tree/webvr-1.0 Should I be using the webvr-1.0 branch of webvr-boilerplate as well? Presumably I can specify a parent element somehow? This is the main roadblock I've run into for my 'virtex' viewer. I will be demonstrating this at the MoMA in a couple of weeks and would love to be able to show people webvr-boilerplate-enabled VR. |
The webvr branch was merged to master for a while, so no need for the other On Mon, Apr 25, 2016 at 8:15 AM Edward Silverton notifications@github.com
|
I've commented all uses of webvr-boilerplate, just using the polyfill now. Based on your example I've tried to recreate the setup in Viewport.ts. If I uncomment this line: https://github.com/edsilv/virtex/blob/webvr/src/Viewport.ts#L406 the camera location seems to be shifted to 0, 0, 0. Any idea what I'm doing wrong? |
to clarify, the camera should be offset like this: uncommenting
puts the camera into the center of the object |
That seems to be working now: http://edsilv.github.io/virtex/test/ Ignore the broken loading bar, it works, it just doesn't like gzipped content coming from github. This has external controls and can toggle between vr/normal mode. One hack I had to put in is this: https://github.com/edsilv/virtex/blob/gh-pages/test/js/VREffect.js#L72 (forced I need to figure out how to debug this on my android phone (I have a Google Cardboard). Any tips gratefully received :-) |
I recommend chrome:inspect. Why did you need the hack? Are you calling On Wed, Apr 27, 2016 at 1:57 AM Edward Silverton notifications@github.com
|
added configurable parentElem and preventButton
#68
#66
if defined,
parentElem
is used in place of window to obtain width and height.preventButton
prevents button construction.