Component tock() method for post scene rendering operations. #1564

wizgrav · 2016-06-18T13:29:12Z

Description:

Enables post scene rendering operations in aframe.

Changes proposed:

Extends a-scene element with a .renderTarget(to be set by a component/system on demand) to enable the rendering of the scene in a texture.
Adds a tock() method on components/systems where post scene rendering logic can be performed.
An example showcasing the functionality is included.

https://wizgrav.github.io/aframe-clubber/demo/

dmarcos · 2016-06-18T14:31:23Z

vendor/VREffect.js

@@ -232,7 +232,7 @@ THREE.VREffect = function ( renderer, onError ) {
 	var cameraR = new THREE.PerspectiveCamera();
 	cameraR.layers.enable( 2 );

-	this.render = function ( scene, camera ) {
+	this.render = function ( scene, camera, renderTarget, forceClear ) {


We should not be changing the code in the vendor folder. If there's something to be changed in VREffect we should submit a PR to three.js https://github.com/mrdoob/three.js/blob/master/examples/js/effects/VREffect.js

I agree and the change is both trivial and makes VREffect more consistent with the rest of three. I'll try submiting a PR to threejs as well but I suggest we keep the patched version for now.

The vendor directory must not have any patched code. It's messy to maintain and easy to forget that we have ad-hoc changes. This PR assumes new functionality on the VR Effect and Three.js might not agree on the approach. This would force us to redo this patch or maintain a fork of VREffect. Let's share the changes on a PR on three.js to get their feedback before merging this.

Ok I'm getting on it

dmarcos · 2016-06-18T14:35:10Z

This code is much cleaner that the previous attempt. @fernandojsg, What do you think?

wizgrav · 2016-06-18T15:02:20Z

@dmarcos implementing the final compositor as a system does away with the need for ordered execution of behaviors so this PR provides all that's needed for post processing. This setup is more proper since there can be only one compositor active. I also included the example as there is no need anymore for further PRs for ordering as discussed with @ngokevin .

Ordered execution could be implemented with something complex like the graph approach or something as simple as using a boolean flag on components to switch between behaviors.unshift/push in a-scene's addBehavior, but that's now unrelated to post proc and can be decided at a later time

wizgrav · 2016-06-19T16:41:16Z

VREffect additions submitted to three.js dev mrdoob/three.js#9172

dmarcos · 2016-06-19T20:35:40Z

@wizgrav nice, thanks!

dmarcos · 2016-06-20T19:55:53Z

Nice it seems mrdoob accepted the patch

dmarcos · 2016-06-20T19:56:17Z

We can now copy and paste to the vendor directory

dmarcos · 2016-06-20T20:05:27Z

examples/test/post-processing/components/compositor.js

+  tock: function () {
+    var scene = this.sceneEl;
+    var rt = scene.renderTarget;
+    if (!scene.enablePostProcessing) return;


missing curly brackets around return

wizgrav · 2016-06-20T20:27:51Z

@dmarcos Yeah that was very responsive and kind of them actually. I'll include the latest VREffect from three/dev in this PR. Also, a guy from the slack channel asked me and so I've also made another example. The approach taken on that, while not as efficient as I'd like, is more akin to the effect composer from three.js. Post effect components are very easy to write as they're just wrappers over the various three shaders. They just provide a shaderMaterial to the compositor system for blending to screen and a namespace to update uniforms. Should I include that as well?

http://wizgrav.github.io/aframe/examples/test/post-processing-grain/

fernandojsg · 2016-06-20T23:41:10Z

I still didn't check the code, by now I'm trying to run the demos with a headset and on chromium they won't work as the video won't load so maybe the demo version could be something simpler using just geometry?
I've tried also to run it on Firefox nightly 50.0a1 with Oculus Rift and I just get rendered the right eye, the left eye is black.
Has someone successfully make them work on windows with headset?

fernandojsg · 2016-06-21T00:01:34Z

examples/test/post-processing/components/bloom.js

+
+    });
+    this.quadPost.material = this.brightMaterial;
+    var rtOptions = { minFilter: THREE.LinearFilter, magFilter: THREE.NearestFilter, format: THREE.RGBAFormat };


better use whole words for variables instead of abbreviations: renderTargetOptions

wizgrav · 2016-06-21T06:49:58Z

@fernandojsg I've updated the compositor to use VREffect instead of the vanilla WebGLRenderer for outputting to screen, can you check the examples now? Unfortunately I don't have an hmd available currently. I hacked a bit on webvr-polyfill and vr-mode-ui to enable vr on desktop and got an image that looked proper but failed to make the view rotate. I'll check on a mobile later on

UPDATE: I have the suspicion that vrHMD.submitFrame(); in VREffect is the culprit here

wizgrav · 2016-06-21T09:40:16Z

@fernandojsg I patched VREffect for hmd frame submission to be performed manually. I haven't updated this PR with the changes, which will also have to be propagated to three.js. Please check the live examples to see if it fixes the issue.

http://wizgrav.github.io/aframe/examples/test/post-processing/
http://wizgrav.github.io/aframe/examples/test/post-processing-grain/

fernandojsg · 2016-06-21T10:44:30Z

@wizgrav I've just tried the new version and it's still painting just one eye. I'll try to debug it this afternoon!

dmarcos · 2017-05-09T00:33:59Z

src/core/scene/a-scene.js

        this.animationFrameID = effect.requestAnimationFrame(this.render);
-        effect.render(this.object3D, this.camera);
+
+        window.performance.mark('render-iteration-started');


Can we move this post processing code to a different PR?

You mean the timings with performance.mark? I so, sure these should be done in a timing PR

I eas refering to the lines below tha check for the posproceesing component and do renderTarget stuff

so should I just leave in the renderTarget creation in setupRenderer and the tock definition in component? the render to texture is straightforward, there's no other way to do it. If you're weary about the renderTarget size checking and resize, unfortunately that's necessary with the current way VREffect works(doesn't hint when it changes the canvas size when going in/out of VR)

I see the tock infrastructure as a way to introduce logic after rendering the scene. It is useful beyond postprocessing. I would make a separate PR that introduces all the postprocessing pieces on top of the tock work that is postprocessing agnostic

I would start by just doing (as analog to the tick):

effect.render(this.object3D, camera, null); if (this.isPlaying) { this.tock(this.time, delta): }

`

I can't readily think of any logic besides post processing that would need to happen after the scene rendering. Having tick for scene updates and tock for post proc is clean, if tock where to be generic it would lead to issues like having to check inside components if post processing is actually enabled for the current frame. Without these checks the final compositor would overwrite the screen with redundant scene renderings or just black. The effect components would also do their, costly, work but the results would not be used

dmarcos · 2017-05-09T00:34:54Z

Thanks for the patience. I would like to keep the tock related logic and move the postprocessing logic to a different PR. Can we do that?

wizgrav · 2017-05-09T07:16:22Z

@dmarcos I'm not sure I understand. You mean keeping the tock infrastructure in but not actually calling it in render()?

dmarcos · 2017-05-11T17:19:21Z

@wizgrav A use case for example we needed this for (and we hacked temporarly) is the ability to capture an animated gif of a running scene.

dmarcos · 2017-05-11T17:23:30Z

@wizgrav we would call tock in the render function the same way we call tick. This is what the render method would look like (renderTarget is null by default)

render: {
      value: function () {
        var effect = this.effect;
        var camera = this.camera;
        var delta = this.clock.getDelta() * 1000;
        this.time = this.clock.elapsedTime * 1000;

        if (this.isPlaying) { this.tick(this.time, delta); }

        this.animationFrameID = effect.requestAnimationFrame(this.render);
        effect.render(this.object3D, camera, renderTarget);

        if (this.isPlaying) { this.tock(this.time, delta); }

        this.effect.submitFrame();
        window.performance.mark('render-iteration-finished');
}

wizgrav · 2017-05-11T18:08:02Z

@dmarcos I understand why you want to keep something so core as simple and generic as possible, I'm thinking more from a convenience standpoint. The video recording is a good example

Maybe we could add a boolean property to the component, to denote that it shouldn't execute when the renderTarget is not enabled for the current frame. Then the "postprocessing" attribute check would just toggle the renderTarget on/off. What do you think of this scheme?

I insist on landing this because I feel that this feature has been discussed for a way long time (in my sense of time) it's been a year and I'd prefer to come up with a solution in this PR so the feature lands, it will give the projects coming out using the framework, a big increase in quality.

dmarcos · 2017-05-11T18:16:52Z

I would not check for the postprocessing attribute inside a-scene. The postprocessing logic should be decoupled from the scene. The postprocessing component could instantiate the renderTarget for the scene and you can move the logic to the tick method (that happens before render) so nothing specific to postProcessing remains in the render loop. We might want to consider a separate scene component called renderTarget that instantiates the renderTarget for those components that require one.

wizgrav · 2017-05-11T18:26:05Z

The scene.renderTarget setting could also be done by the final compositor system itself utilizing both tick and tock. It sounds ok

wizgrav · 2017-05-12T06:25:26Z

@dmarcos removed renderTarget handling from scene, that can now be set by a component/system, example here https://wizgrav.github.io/aframe-clubber/demo/

One minor thing I found was that system update is not called when changing attributes from the dom inspector, only with setAttribute(). I used to override setEntityAttribute to do that but took it out cause it caused a test to fail. This is unrelated to tock though and could be fixed in another PR.

dmarcos · 2017-05-12T06:59:16Z

Thanks a lot for the patience. Longest running PR in aframe history. It need a rebase.

wizgrav · 2017-05-13T11:40:13Z

@dmarcos rebased and also proposed mrdoob/three.js#9848 to threejs. It's as minimal as it can be and If accepted, it will solve all issues concerning post processing in VR(proper godrays and such).

wizgrav · 2017-05-15T07:56:07Z

@dmarcos just saw that ArrayCamera has landed in three dev and that is indeed the best way to handle it, so I'll wait for when AFrame utilizes WebVRCamera to get the twin projection matrices after the scene renders. In the meantime tock is ready and sufficient for most effects as we are now I think we're ok as we are now even for view dependant effects. ArrayCamera will give a nice boost though so I'm looking forward to it

dmarcos · 2017-05-15T14:31:16Z

Yeah! Thanks very much for sticking with it.

dmarcos · 2017-05-15T14:33:59Z

@wizgrav one last thing. I see changes on VREffect. Are those coming from the three repo? Do we need then!

wizgrav · 2017-05-15T14:44:14Z

@dmarcos yes I made the checks for layer.leftBounds and layer.rightBounds less strict as @johnnyshankman reported that they were undefined instead of null which the original code checked against. I hadn't experienced that myself tbh so I can take it out and if encountered again we can open a new issue. Though I don't expect side effects even if we leave it in

wizgrav · 2017-05-15T19:10:28Z

@dmarcos reverted VREffect. Also updated the tock related tests

dmarcos · 2017-05-15T19:17:59Z

Amazing! it only took a year to land this 😄 @wizgrav It's fantastic you were able to bring it back home

wizgrav changed the title ~~Added tock() method on components for post processing support.~~ Component tock() method for post processing support. Jun 18, 2016

dmarcos reviewed Jun 18, 2016
View reviewed changes

dmarcos reviewed Jun 20, 2016
View reviewed changes

fernandojsg reviewed Jun 21, 2016
View reviewed changes

dmarcos reviewed May 9, 2017

View reviewed changes

dmarcos added needs changes and removed needs review labels May 11, 2017

wizgrav added 2 commits May 12, 2017 09:06

refactor

c98be07

fix index.js

6741ae5

fix tock isPlaying

32fd697

dmarcos added needs rebase and removed needs changes needs answers / feedback labels May 12, 2017

fixes

1b01430

wizgrav changed the title ~~Component tock() method for post processing support.~~ Component tock() method for post scene rendering operations. May 12, 2017

wizgrav added 2 commits May 15, 2017 21:56

reverted VREffect

bbb12ed

updated tests

e007b56

dmarcos merged commit 71d4ded into aframevr:master May 15, 2017

Component tock() method for post scene rendering operations. #1564

Component tock() method for post scene rendering operations. #1564

Conversation

wizgrav commented Jun 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmarcos commented Jun 18, 2016

wizgrav commented Jun 18, 2016 • edited Loading

wizgrav commented Jun 19, 2016 • edited Loading

dmarcos commented Jun 19, 2016

dmarcos commented Jun 20, 2016

dmarcos commented Jun 20, 2016

Choose a reason for hiding this comment

wizgrav commented Jun 20, 2016

fernandojsg commented Jun 20, 2016

Choose a reason for hiding this comment

wizgrav commented Jun 21, 2016 • edited Loading

wizgrav commented Jun 21, 2016 • edited Loading

fernandojsg commented Jun 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wizgrav May 11, 2017 • edited Loading

Choose a reason for hiding this comment

dmarcos commented May 9, 2017

wizgrav commented May 9, 2017

dmarcos commented May 11, 2017

dmarcos commented May 11, 2017

wizgrav commented May 11, 2017 • edited Loading

dmarcos commented May 11, 2017

wizgrav commented May 11, 2017 • edited Loading

wizgrav commented May 12, 2017

dmarcos commented May 12, 2017

wizgrav commented May 13, 2017

wizgrav commented May 15, 2017 • edited Loading

dmarcos commented May 15, 2017

dmarcos commented May 15, 2017

wizgrav commented May 15, 2017 • edited Loading

wizgrav commented May 15, 2017

dmarcos commented May 15, 2017

wizgrav commented Jun 18, 2016 •

edited

Loading

wizgrav commented Jun 18, 2016 •

edited

Loading

wizgrav commented Jun 19, 2016 •

edited

Loading

wizgrav commented Jun 21, 2016 •

edited

Loading

wizgrav commented Jun 21, 2016 •

edited

Loading

wizgrav May 11, 2017 •

edited

Loading

wizgrav commented May 11, 2017 •

edited

Loading

wizgrav commented May 11, 2017 •

edited

Loading

wizgrav commented May 15, 2017 •

edited

Loading

wizgrav commented May 15, 2017 •

edited

Loading