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

Graph based behavior execution scheduling and post processing support #1437

Closed
wants to merge 1 commit into from
Closed

Conversation

wizgrav
Copy link
Contributor

@wizgrav wizgrav commented May 4, 2016

Following the discussions with @ngokevin @dmarcos @donmccurdy and @fernandojsg :

Enable post processing by extending components with a .tock() method which runs after the scene renders.

A-scene maintains a .renderTarget and switches rendering to that in the presence of at least one tock behavior.

A graph based mechanism is employed to allow for explicitly definable execution ordering for both tick and tock behaviors.

Components can define two way connections in the graph using the properties .precedents and .following, similar to .dependencies.

Sorting to define the execution order takes place only in registerComponent() and batching is done efficiently in a-scene using caches.

The graph based mechanism works fast and comfortable for the developer. Cyclic dependencies in the graph are errors and throw exceptions.

An example utilizing most of the functionality added by this PR at http://wizgrav.github.io/aframe/examples/test/post-processing/index.html

@donmccurdy
Copy link
Member

This is really neat! Nice example.

As far as the API, it feels strange having DOM events hooked into the render loop, firing every 8-16ms. I'm not sure exactly what to recommend though. Other ideas:

// 1.
sceneEl.setPostProcessingCallback(function () { /* ... */ });

// 2.
sceneEl.addPostProcessingCallback(function () { /* ... */ });

// 3.
AFRAME.registerComponent('my-component', {
  prerender: function () { /* called only if my-component is attached to sceneEl */ },
  postrender: function () { /* called only if my-component is attached to sceneEl */ },
});

@wizgrav
Copy link
Contributor Author

wizgrav commented May 5, 2016

@donmccurdy you proposals #1/#2 were also my original thoughts. I went for emit because it seemed to be more consistent with the patterns used in the library. I'm almost certain that performance shouldn't degrade with dom events. Of course this needs investigation but 8/16ms is still a good chunk and we have to take into account that postprocessing will have a big cost itself so the events will probably be a negligible fraction of that

@dmarcos
Copy link
Member

dmarcos commented May 5, 2016

Thanks for tackling this. I don't think emitting events from the render loop is a very good idea. It adds overhead (and most of the time you won't even use it) and compromises performance. Having a way to do postprocessing effects would be indeed very cool. I like more @donmccurdy approach of providing two methods at the component level. You can currently insert your logic in the render loop via the tick function of components and systems. It would be nice to have a way to throw warnings or stats on console if any those user defined functions in the render loop are too slow.

* @param {number} time - Scene tick time.
* @param {number} timeDelta - Time it took to render the scene.
*/
tick: undefined,
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need Systems to have a hook into post-render callback yet.

@ngokevin
Copy link
Member

ngokevin commented May 8, 2016

Can you remove the dist bumps?

@@ -0,0 +1,117 @@
<!DOCTYPE html>
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this example to examples/test/post-processing and rename the titles/metatags to match?

@ngokevin
Copy link
Member

ngokevin commented May 8, 2016

Hmm, after going over the PR, I'm not sure about piggybacking off the component API. It assumes postRender might have a wider use case than just post-processing. And having to add special cases for pause/play is not great since post-processing shouldn't have a concept of pause/play. Maybe we need a tighter API for register post-processing effects. Thinking about it...

@wizgrav
Copy link
Contributor Author

wizgrav commented May 8, 2016

based on @dmarcos comments I added a post method on components. Behaviors are split in tick and post types and handled separately. Performance has indeed improved noticeably(from 50% -> 40% on my box for the example) so this seems like the optimal path to take.

I also added a postPriority property on components. This is intended to facilitate common post proc setups where several components will produce their own renderTargets containing bloom, godrays, ssao etc and then a composer component can pick these up and blend them to screen. Prioritization happens only on post behaviors(which should only be a handful) and sorting happens only once per frame if there was a change in the post pipeline.

Finally an event is emitted when the sorting happens to indicate that the pipeline has changed. This can be picked up by the composer components so they can reconfigure themselves. I'm not sure about this one maybe a simple timestamp could work. What are your thoughts on this, and the whole pr in general?

@wizgrav
Copy link
Contributor Author

wizgrav commented May 8, 2016

@ngokevin sorry for the mess with dist and such, I was having great pain with npm and setting up the build in general. I'll fix everything

@wizgrav
Copy link
Contributor Author

wizgrav commented May 8, 2016

I also included the tweaked VREffect inside lib/three.js until it is fixed in three.js itself. I see no reason why they wouldn't do the tweak themselves. The VREffect render() should match the signature of the base WebGLRenderer since it's a wrapper of it. Another note of interest is the use of depth textures. I think it recently landed on three and fortunately they implemented it as a property on the renderTarget so when aframe upgrades to the latest three.js all that will be needed to utilize depth textures will be

this.renderTarget.depthTexture = new THREE.DepthTexture()

and of course resizing it along with the renderTarget itself

@ngokevin
Copy link
Member

ngokevin commented May 8, 2016

I don't like using the component API too much. We are adding a special handler and property that from what I can tell would only be used for post-processing, and then we have to touch play/pause code, and such.

Brainstorming, but here's an alternative API, which would be harder to implement, but would be a bit more explicit. Using the System API, this would be a bit similar to Don's Controls API:

AFRAME.registerPostProcessor('some-effect', {
  schema: {},
  vertexShader: 'vertexshaderstring',
  fragmentShader: 'fragmentshaderstring'
});

<a-scene postprocess="bloom, hdr, ssao" bloom="intensity: 0.5>

@wizgrav
Copy link
Contributor Author

wizgrav commented May 8, 2016

@ngokevin I'm not sure a postproc effect can be described like a shader, it's more of a setup. For instance bloom commonly uses a separable gaussian blur so it would need at least two passes. Good bloom would also entail downsampling several times, blurring, and blending again on the way up. I think components make for a natural expression even if meant to be attached on the scene only. But I don't know I haven't yet looked over all of aframe's source and patterns so you'd know best

@wizgrav
Copy link
Contributor Author

wizgrav commented May 8, 2016

@ngokevin another thing that should be taken into account while brainstorming is that customisation logic ends up in the composer usually. Postproc steps that provide intermediate renderTargets like bloom and ssao tend to be straightforward and so could be cast in place as reusable components. Only the composer needs special treatment, both for doing the composition of all the rest and also to apply the simple effects like vignette aberations, ascii etc At least for production one shouldn't use extra passes for these. Well maybe a final fxaa step if antialiasing is desired but this is straighforward as well.

The way I imagined it with postPriority would be for all steps that produce renderTargets from the scene's rendering would be left at their default zero priority. The composer would be at one and check for all post components with less priority than him and finally fxaa at two would check for the composer, again based on postPriority

@ngokevin
Copy link
Member

ngokevin commented May 8, 2016

Ah, right, thanks for the details. I should clarify the sample API I had would still make post-processing effects a component, but just a subset API of a component. Just because postRender + postRenderPriority would only be used by effects, seems to make sense to fence it off.

@wizgrav
Copy link
Contributor Author

wizgrav commented May 8, 2016

Yeah I understand your concerns. Defining post specific components seems to be the cleaner route then unless there would be a reason for post components to also run some logic at tick time. To be honest though I cannot think of such a case right now, I need to think about it

@wizgrav
Copy link
Contributor Author

wizgrav commented May 9, 2016

Ok so I tried to think of cases where it would be useful to have a component that taps into both tick and postRender and something did came up. I've updated the example which you can see on http://wizgrav.github.io/aframe/examples/test/post-processing/

The alpha channel of the render target can be used to pass information to the post processing chain, the most common case being masking of objects. In the example the pulse component sets up properties on its underlying mesh. The postproc component then traverses the scene and adjusts the opacity according to these properties. It also handles cleanup when it's removed from the scene.

Alternatively the pulse component could setup it's opacity by itself but it would need to know the state of post processing. It could do that by listening for something like the "post-modified" event.

I'm not arguing in favor of either approach, it would be convenient to have both options availble.

I would also like to mention something, and I know you're going to hate me @ngokevin , but it really needs to be said. It is inevitable that components will need to be extended. At some point, a component will need to access the results of computations performed on another component on another entity. A way to guarantee the order of execution will be needed. I'm sorry if I've missed something, I still study the source but I think there isn't something like that in currently.

Doing something like postRenderPriority may work for postproc because it's by nature limited in scale but for tick behaviors I don't think it's appropriate at all. The solution unity came up for this situation was to add a LateUpdate method which runs after all update methods. So aframe will also need a tock() to go along with tick(). Again excuse me if I'm missing something. This does not concern this PR, it's something else all together but it's relevant and I feel that it should be taken into account

@wizgrav
Copy link
Contributor Author

wizgrav commented May 9, 2016

Following the line of thought from the previous comment, if a tick()/tock() pair is indeed desirable, the api could get some symmetry with a postTick()/postTock() pair for the post processing, getting postRenderPriority out of the picture. Two lanes for the post processing should cover most cases. The third fxaa step I mentioned could be integrated in the compositor itself in postTock(). So that setup could also work

@wizgrav
Copy link
Contributor Author

wizgrav commented May 11, 2016

@ngokevin @dmarcos Added tock() on components as a straight equivalent of unity's LateUpdate. A similar pair is implemented for post render as postRenderTick, postRenderTock. These should be enough to guarantee the order of execution so postRenderPriority was removed. The post-modified event is still emitted as it is useful for reconfiguring the compositor when the post chain changes.

The example at http://wizgrav.github.io/aframe/examples/test/post-processing/ was commented and updated to reflect all functionality in the PR. Some crude bloom was added to highlight the post tick/tock pattern, you can toggle the effect by right clicking. Ascii is always on now.

@ngokevin
Copy link
Member

Thanks, will take a look when I can

@ngokevin
Copy link
Member

Can you remove the dist files from the PR?

<script src="../../../dist/aframe.js"></script>

<script>
AFRAME.registerComponent('pulse', {
Copy link
Member

Choose a reason for hiding this comment

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

Let's register these components in separate files: e.g., components/pulse-effect.js, components/envmap.js, components/bloom.js

@wizgrav
Copy link
Contributor Author

wizgrav commented May 11, 2016

Hmm, I split the example but I'm not sure I did the right thing with the dist files. I did a checkout on the last commit before I touched them. Can you suggest how could they get out of the PR completely?

There's also the issue with VREffect being included in lib/three.js which is not so good. I can try making a PR to three so they can fix it on their source but to be honest, requiring such a core part of aframe from an examples/ folder kind of makes me nervous. Since you control the revision of three included anyway, maybe you should consider including VREffect.js in aframe's source somewhere.

].join("\n"),

fragmentText: [
"uniform sampler2D srcTexture;",
Copy link
Member

Choose a reason for hiding this comment

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

nice, thanks

@ngokevin
Copy link
Member

  • dist/ files are out and look OK now.
  • Yeah, that's not great. But @dmarcos just pulled them into vendor/ for WebVR 1.0 API reasons. We still wouldn't want to touch them. Maybe we need that module to live in its own home?

@wizgrav
Copy link
Contributor Author

wizgrav commented May 31, 2016

Graphing is generally useful so I took it out into it's own package https://www.npmjs.com/package/precedence-maps and pull it as a dependency. The package also provides ordered stores so the code in a-scene is much cleaner now

@@ -110,7 +110,7 @@ helpers.getSkipCISuite()('a-scene (with renderer)', function () {

test('calls behaviors', function () {
var scene = this.el;
var Component = { el: { isPlaying: true }, tick: function () {} };
var Component = {el: { isPlaying: true }, tick: function () {}};
Copy link
Member

Choose a reason for hiding this comment

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

you're my type of guy

Copy link
Member

Choose a reason for hiding this comment

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

also around the isPlaying too

@ngokevin
Copy link
Member

Do you think it'd be a good idea to split this PR up into three parts so it'd be easier to review?

  • tock
  • execution ordering
  • post processing exaple

@wizgrav
Copy link
Contributor Author

wizgrav commented Jun 10, 2016

@ngokevin yes I was also thinking about splitting it. The only issue is that the PRs would conflict with each other. They both touch this.behaviors in a-scene.js which is split in tick and tock. We could do the ordering as the first PR for tick() and then do tock(). All the graphing logic now resides in the precedence-maps npm package so we could also do some commentary there. You can also take it under a-frame's umbrella if you want. It bases of toposort which seems like a legit package, so I don't see much worry there.

Should the example for tock be a PR on it's own?

@ngokevin
Copy link
Member

Is it possible to do tock addition first? I think that would be easiest. And then later, we can layer on the ordering?

@wizgrav
Copy link
Contributor Author

wizgrav commented Jun 10, 2016

it's certainly possible but mind that the example actually depends on the order guarantees. As the bloom component gets added/removed by bloom-toggler it's execution order will change. This means that the compositor component(post-process) which outputs to screen will blend the current scene's frame with the bloom texture from the previous frame. We can probably workaround that by also removing and readding the post-process component in order as well

@ngokevin
Copy link
Member

Yeah, I was suggesting that we save the post-processing example for last.

@wizgrav
Copy link
Contributor Author

wizgrav commented Jun 10, 2016

Ok I'll start off with a new PR for tock() then

@ngokevin
Copy link
Member

Cool. We can keep this one open to track the overall work.

@cvan
Copy link
Contributor

cvan commented Apr 18, 2017

@wizgrav did you want to wait until PR #1564 is merged before fixing this one up too?

@wizgrav
Copy link
Contributor Author

wizgrav commented Apr 18, 2017

@cvan the tock() method and the behavior ordering overlap on some parts. Should I merge both in this one? Otherwise I think the tock PR would need to go first

@wizgrav
Copy link
Contributor Author

wizgrav commented May 7, 2017

@cvan #1564 is rebased and clear for merge

@cvan
Copy link
Contributor

cvan commented May 8, 2017

@wizgrav thanks. @ngokevin and @dmarcos are best to review #1564. thanks; excited to see this get merged 👍

@ngokevin
Copy link
Member

ngokevin commented Nov 2, 2017

I think we can close. Not much demand for ordering past tick/tock and it seems aframe-effects was able to get by.

@ngokevin ngokevin closed this Nov 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants