antialias by default on desktop too #2455
Conversation
src/core/scene/a-scene.js
Outdated
| @@ -344,10 +344,10 @@ module.exports = registerElement('a-scene', { | |||
| var canvas = this.canvas; | |||
| // Set at startup. To enable/disable antialias | |||
| // at runttime we would have to recreate the whole context | |||
| var antialias = this.getAttribute('antialias') === 'true'; | |||
| var antialias = this.getAttribute('antialias') !== 'false'; | |||
There was a problem hiding this comment.
We can enable antialias by default only on desktop
ngokevin
force-pushed
the
aa
branch
2 times, most recently
from
be7a778
to
9ac647f
Compare
|
Updated, with tests. Logic is (explicitly enabled or disabled) or (has native webvr or is not mobile) |
|
@dmarcos r? |
src/core/scene/a-scene.js
Outdated
| */ | ||
| function shouldAntiAlias (sceneEl) { | ||
| // Explicitly set. | ||
| if (sceneEl.getAttribute('antialias') === 'true') { return true; } |
There was a problem hiding this comment.
Can we do
if (sceneEl.getAttribute('antialias') !== null) { return sceneEl.getAttribute('antialias') === 'true'; }
src/core/scene/a-scene.js
Outdated
| if (sceneEl.getAttribute('antialias') === 'false') { return false; } | ||
|
|
||
| // Has WebVR or is desktop. | ||
| return window.hasNativeWebVRImplementation || !sceneEl.isMobile; |
There was a problem hiding this comment.
this will match Daydream, FYI
| canvas: canvas, | ||
| antialias: antialias || window.hasNativeWebVRImplementation, | ||
| canvas: this.canvas, | ||
| antialias: shouldAntiAlias(this), |
There was a problem hiding this comment.
could this instead by exposed as an antialias component on <a-scene>? that way folks can force enable/disable it if they want.
There was a problem hiding this comment.
antialias cannot be disabled once the context has been created. You would have to create a new one and resubmit all textures and buffers (http://stackoverflow.com/questions/27554969/dynamically-turn-on-off-antialiasing-and-shadows-in-webglrenderer).
There was a problem hiding this comment.
There's a code quality issue to make a renderer component to be configurable, antialias would be part of that.
There was a problem hiding this comment.
I still don't think it's could be modifiable at runtime. You would invalidate all the components that have submitted textures / buffers to the GPU.
There was a problem hiding this comment.
I pretty elaborated machinery could be implemented to make antialias dynamic but that's something you don't want to do in 99.9999999% of the cases. We can revisit in the future if there's a very good reason
There was a problem hiding this comment.
Yeah, I agree, it cannot be configurable at runtime. I just mean configurable at init time.
|
|
||
| /** | ||
| * Determines if renderer anti-aliasing should be enabled. | ||
| * Enabled by default if has native WebVR or is desktop. |
There was a problem hiding this comment.
can this be mentioned in the Docs? (especially if you go the antialias component route)
|
Thanks! |
Description:
Anti-aliasing is desirable for VR. If we're targeting room scale, enabling it makes sense as the default.