[renderer] Add scene component for renderer config. by donmccurdy · Pull Request #3424 · aframevr/aframe

donmccurdy · 2018-03-09T04:34:36Z

Resolves #666.

<a-scene renderer="antialias: true;
                   sortObjects: true;
                   gammaOutput: true;">

donmccurdy · 2018-03-09T04:42:35Z

src/core/scene/a-scene.js

+ * Determines state of various renderer properties.
+ * This could be done by a component, but it's important
+ * to know the values when the renderer is created, so
+ * that materials don't need to be recompiled.


Correction: we only need to know antialias up front.

ngokevin · 2018-03-09T04:51:49Z

I would try using renderer="foo: bar" syntax, but not thru the component flow? Perhaps just parse it with AFRAME.utils.styleParser.parse.

donmccurdy · 2018-03-09T07:32:59Z

Would you want to roll antialias into that, or leave it as-is? Other than that a “real” component would be fine.

ngokevin · 2018-03-14T18:55:40Z

Yeah, we can roll anti-alias into that, and deprecate the current antialias="true".

donmccurdy · 2018-03-19T03:11:28Z

Ok, implemented renderer="antialias: true; gammaOutput: true" here. Will add docs + test if the gist looks good.

ngokevin · 2018-03-22T20:26:48Z

Looks good. There's no problem with the component setting up too late?

ngokevin

noice

ngokevin · 2018-03-22T20:27:36Z

src/components/scene/renderer.js

+
+    // Default not AA for mobile.
+    // NOTE: This default is also applied in a-scene.js.
+    var attrString = HTMLElement.prototype.getAttribute.call(el, this.name);


We've been doing vars-on-top (declarations only). Though tempted to just adopt prettier :P

ngokevin · 2018-03-22T20:29:28Z

src/components/scene/renderer.js

+    if (needsShaderUpdate) {
+      warn('Modifying renderer properties at runtime requires shader update and may drop frames.');
+      sceneEl.object3D.traverse(function (node) {
+        if (node.isMesh) {


can do an early return and keep an indent if (!node.isMesh) { return; }

ngokevin · 2018-03-22T20:29:47Z

src/components/scene/renderer.js

+    renderer.physicallyCorrectLights = data.physicallyCorrectLights;
+  },
+  update: function (prevData) {
+    if (!Object.keys(prevData).length) { return; }


I guess this check isn't even really needed?

It's needed, update() is called during initialization and we don't want to set .needsUpdate=true unnecessarily.

ngokevin · 2018-03-22T20:30:47Z

src/components/scene/renderer.js

+    // NOTE: This default is also applied in a-scene.js.
+    var attrString = HTMLElement.prototype.getAttribute.call(el, this.name);
+    if (attrString && !attrString.match(/($|\W)antialias\W/)) {
+      el.setAttribute(this.name, {antialias: !el.isMobile});


Can do setAttribute('renderer', 'antialias', !el.isMobile);

Don't need this.name above either, can just be renderer

dmarcos · 2018-03-24T19:48:17Z

Default components are gone on #3490. We need to adapt this PR

donmccurdy · 2018-03-24T19:53:01Z

There's no problem with the component setting up too late?

Well, the renderer.antialias handling is a bit of a hack because init() runs too late. Otherwise it's fine, we can set the other properties before any shaders have compiled.

Having trouble doing a git push from the cafe, so the last commit is still missing and my "Done" comments were premature.

@dmarcos — without default components, the only issue I see is that I can't detect whether the renderer component has been added before or after scene initialization. sceneEl.hasLoaded seems to be true when init() runs either way. I don't want to set .needsUpdate on all of the materials if the renderer hasn't compiled them yet... is there another way to see if the first render has already happened?

donmccurdy · 2018-03-24T22:34:45Z

Resolved the initialization issue by checking sceneEl.time > 0. Added tests and docs. I think this is ready to go except one thing — are we OK deprecating antialias="true" on an 0.8.X release? I've kept it functional here but the warning is still logged.

dmarcos · 2018-03-24T22:44:21Z

Besides the warning messages. Should we reflect in the docs that antialias cannot be changed after initialization?

donmccurdy · 2018-03-24T23:19:00Z

Done (and rebased).

dmarcos · 2018-03-25T16:53:14Z

Better not to introduce API changes on a minor version. We can deprecate <a-scene antialias="true"> later

dmarcos · 2018-03-25T16:53:25Z

Thank you!

donmccurdy · 2018-03-26T14:08:10Z

Better not to introduce API changes on a minor version. We can deprecate later

Ok, note this PR does log a warning about antialias (but keeps it working). We'll want to remove that before 0.8.2 then?

donmccurdy commented Mar 9, 2018

View reviewed changes

donmccurdy mentioned this pull request Mar 9, 2018

Resolve alpha depth issues automatically #666

Closed

donmccurdy mentioned this pull request Mar 19, 2018

Update three.js version to r88+ #3263

Closed

donmccurdy added this to the 0.8.X milestone Mar 21, 2018

ngokevin reviewed Mar 22, 2018

View reviewed changes

dmarcos added the needs rebase label Mar 24, 2018

donmccurdy added 7 commits March 24, 2018 16:17

[a-scene] Expose additional renderer properties as scene components.

570b8b2

[a-scene][renderer] Add 'renderer' component.

1012b79

[antialias] Deprecate antialias component.

c9d27a1

[renderer] Updates from feedback.

96f5a61

[renderer] Add docs and tests.

c0534c2

[renderer] Restore 'antialias' old behavior for now.

d989d92

[docs] Note that antialias cannot be changed after scene init.

168a4ad

donmccurdy changed the title ~~[a-scene] Expose additional renderer properties as scene components.~~ [renderer] Add scene component for renderer config. Mar 24, 2018

dmarcos merged commit bb16c01 into aframevr:master Mar 25, 2018

diarmidmackenzie mentioned this pull request Jul 8, 2023

A-Frame sort order flexibillity can be improved #5332

Closed

Uh oh!

Conversation

donmccurdy commented Mar 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngokevin commented Mar 9, 2018

Uh oh!

donmccurdy commented Mar 9, 2018

Uh oh!

ngokevin commented Mar 14, 2018

Uh oh!

donmccurdy commented Mar 19, 2018

Uh oh!

ngokevin commented Mar 22, 2018

Uh oh!

ngokevin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmarcos commented Mar 24, 2018

Uh oh!

donmccurdy commented Mar 24, 2018

Uh oh!

donmccurdy commented Mar 24, 2018

Uh oh!

dmarcos commented Mar 24, 2018

Uh oh!

donmccurdy commented Mar 24, 2018

Uh oh!

dmarcos commented Mar 25, 2018

Uh oh!

dmarcos commented Mar 25, 2018

Uh oh!

donmccurdy commented Mar 26, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

donmccurdy commented Mar 9, 2018 •

edited

Loading