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.

Sign up for GitHub

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

Jump to bottom

Add powerPreference preference #12533 #12753

Merged
merged 5 commits into from
Nov 29, 2017
Merged

Add powerPreference preference #12533 #12753

merged 5 commits into from
Nov 29, 2017

Conversation

luruke
Copy link
Contributor

@luruke luruke commented Nov 26, 2017

No description provided.

@luruke luruke mentioned this pull request Nov 26, 2017
Closed
@luruke
Copy link
Contributor Author

luruke commented Nov 26, 2017

I had to move webglcontextlost and webglcontextrestored events listener before the creation of the gl context.

According to the spec:

At WebGLRenderingContext creation time, this attribute will be ignored if no event listeners exist which would handle both "webglcontextlost" and "webglcontextrestored" events dispatched to the canvas element.

Honestly I had hard time to figure out any difference on my machine MacBook Pro (13-inch, 2016) - 10.13.1, Intel Iris Graphics 540 1536 MB.

I guess this parameter is taken into consideration for machine with multiple GPU?

Mugen87
Mugen87 requested changes Nov 26, 2017
View reviewed changes
src/renderers/WebGL2Renderer.js Outdated
_antialias = parameters.antialias !== undefined ? parameters.antialias : false,
_premultipliedAlpha = parameters.premultipliedAlpha !== undefined ? parameters.premultipliedAlpha : true,
_preserveDrawingBuffer = parameters.preserveDrawingBuffer !== undefined ? parameters.preserveDrawingBuffer : false,
_powerPreference = parameters.powerPreference !== undefined ? parameters.powerPreference : 'default';
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some formatting issues...

mrdoob
mrdoob reviewed Nov 27, 2017
View reviewed changes
src/renderers/WebGL2Renderer.js Outdated
_antialias = parameters.antialias !== undefined ? parameters.antialias : false,
_premultipliedAlpha = parameters.premultipliedAlpha !== undefined ? parameters.premultipliedAlpha : true,
_preserveDrawingBuffer = parameters.preserveDrawingBuffer !== undefined ? parameters.preserveDrawingBuffer : false;
_alpha = parameters.alpha !== undefined ? parameters.alpha : false,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like there is a mix of spaces and tabs for indentation in these lines...

mrdoob
mrdoob reviewed Nov 27, 2017
View reviewed changes
src/renderers/WebGL2Renderer.js Outdated
@@ -36,9 +37,13 @@ function WebGL2Renderer( parameters ) {
stencil: _stencil,
antialias: _antialias,
premultipliedAlpha: _premultipliedAlpha,
preserveDrawingBuffer: _preserveDrawingBuffer
preserveDrawingBuffer: _preserveDrawingBuffer,
powerPreference: _powerPreference,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comma at the end is not needed.

mrdoob
mrdoob reviewed Nov 27, 2017
View reviewed changes
src/renderers/WebGLRenderer.js Outdated
@@ -199,9 +200,13 @@ function WebGLRenderer( parameters ) {
stencil: _stencil,
antialias: _antialias,
premultipliedAlpha: _premultipliedAlpha,
preserveDrawingBuffer: _preserveDrawingBuffer
preserveDrawingBuffer: _preserveDrawingBuffer,
powerPreference: _powerPreference,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comma at the end is not needed.

@mrdoob
Copy link
Owner

mrdoob commented Nov 27, 2017

I had to move webglcontextlost and webglcontextrestored events listener before the creation of the gl context.

According to the spec:

At WebGLRenderingContext creation time, this attribute will be ignored if no event listeners exist which would handle both "webglcontextlost" and "webglcontextrestored" events dispatched to the canvas element.

I don't understand... Do you mind giving more context?

@luruke
Copy link
Contributor Author

luruke commented Nov 27, 2017

@mrdoob sorry for those silly indentations issues.

From the spec: https://www.khronos.org/registry/webgl/specs/latest/1.0/#5.2.1
What I understand is that you must handle webglcontextlost and webglcontextrestored before the creation of the context, otherwise the powerPreference is ignored.

A related issue:
KhronosGroup/WebGL#2343

@luruke
Copy link
Contributor Author

luruke commented Nov 28, 2017

@mrdoob @Mugen87
You can find more info here:
https://www.khronos.org/webgl/public-mailing-list/public_webgl/1703/msg00021.php

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 28, 2017 •
edited
Loading

see KhronosGroup/WebGL#2343

And from your previous post:

You MUST have a registered event handler for the webglcontextlost and webglcontextrestored events if you want the user agent to respect your request for high-performance.

@luruke So yes, i think the implementation is correct.

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 28, 2017 •
edited
Loading

Um, maybe we should add a comment. Something like:
// event listeners must be registered before WebGL context is created, see #12753
Otherwise it's easy to overlooked this dependency...

@mrdoob
Copy link
Owner

mrdoob commented Nov 29, 2017

Perfect!

@mrdoob mrdoob merged commit ed7cafe into mrdoob:dev Nov 29, 2017
@mrdoob
Copy link
Owner

mrdoob commented Nov 29, 2017

Thanks!

leosingleton added a commit to leosingleton/fim that referenced this pull request Jul 20, 2019
@leosingleton
Registering event handlers before getting context
ca7ea6f 
According to this bug in three.js, it must be done in this order. Doing
the same here to avoid potential issues:

mrdoob/three.js#12753
@vanruesc vanruesc mentioned this pull request Sep 24, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrdoob mrdoob mrdoob left review comments

@Mugen87 Mugen87 Mugen87 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@luruke @mrdoob @Mugen87