Skip to content
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

Delete redundancy ( Performance: Cut rendering time in half for the sample index.html ) #169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

edom18
Copy link

@edom18 edom18 commented Dec 30, 2016

I have found redundancy codes. I have deleted it.

@edom18
Copy link
Author

edom18 commented Jan 7, 2017

I have noticed an issue in Nightly. The first commit of my PR that deleted twice calling rendering method. So twice calling is very heavy for Nightly. After deleting it, Nightly is very fast running in VR mode.

@haeric
Copy link

haeric commented Jan 11, 2017

👍 to this PR. The description is sparse, but the change is important. The title of this PR could better be explained Performance: Cut rendering time in half.

effect.render(scene, camera); effectively renders the VREffect a second time, after the manager has already done it. This means that everyone using this boilerplate is effectively seeing double the rendering time, which originally made me seriously question the performance of WebVR on the Google Pixel.

One could also go the opposite route, and remove/deprecate the .render() from the WebVRManager. But it's more hassle to change the WebVRManager constructor, assuming we want to deprecate and not just flat out remove.

This change was tested on Pixel and Pixel XL:
Chrome Beta 56.0.2924.53
Chrome Dev 57.0.2977.0

@edom18 edom18 changed the title Delete redundancy Delete redundancy ( Performance: Cut rendering time in half for the sample index.html ) Jan 14, 2017
@edom18
Copy link
Author

edom18 commented Jan 14, 2017

Thank you for comment. I've rewrite the subject of PR :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants