mrdoob / three.js

JavaScript 3D Library.
https://threejs.org/
MIT License
101.87k stars 35.32k forks source link

Exposing internals + onProject, onRender and onRenderShadow #20457

Closed Fyrestar closed 8 months ago

Fyrestar commented 3 years ago

These 3 simple callbacks + exposing some internal renderer objects allow to implement a set of new fundamental features. The callbacks are for the Object3D class.

onProject

To override the culling routine, i made a spatial index "IndexedVolume" based on Object3D. By returning false in this callback THREE cancels the current projectObject call, the IndexedVolume will perform indexed hierarchical culling and then push the visible objects into the renderList, but projectObject can be called as well to let THREE take over again, furthermore i implemented auto-instancing, impostors and other in this routine.

Asides of custom culling implementations this can be used to render additional objects that aren't part of the scene hierarchy. Also a module based selection, having render objects in the main scene hierarchy but rendering in a different pass, working as a more flexible version to layers.

The onProject callback receives renderer, camera, frustum, renderList, groupOrder, sortObjects.

onRender

ImmediateRenderObject is nice but very limited, only allowing triangle mode and a specific set of attributes. onRender allows to implement a custom render call to renderBufferDirect and using + updating any geometries or other operations only possible at this stage, but not with onBeforeRender. It basically also works as ImmediateRenderObject but both gl buffers or THREE managed BufferGeometry can be used, but without being limited to the renderBufferImmediate imlementation.

I replaced isImmediateRenderObject, with isCustomRenderObject , but could be as well a else if case of course.

onRenderShadow

Basically like onProject but for shadows, receiving renderer, camera, frustum, shadowCamera, light, type, scope and returns if onRenderShadow returns false. It would be nice if any needed methods such as getDepthMaterial could be exposed as well, as it gives much more control to replace renderObject with a modified approach as shadow proxies might be used or a different depth materials management.

Internals

There are some internal objects such as frustum, textures, geometries, properties, objects that are as well private yet, i needed those for various cases in those callbacks, but especially for onRender. Regarding methods: in WebGLRenderer projectObject, setProgram as well as in WebGLShadowMap renderObject, i called the shadowMap callback version onRenderShadow.

These additions all together would be just a few lines but opening a vast amount of possibilities and more control, it would be great if they could find their way officially into the core.

Additional note: instead calling empty callbacks like onBeforeRender i implemented those as the following, declaring them on the prototype with null: if ( object.onProject instanceof Function && object.onProject( ... ) === false ) return;

DusanOpenSpace commented 3 years ago

The more the merrier! Not sure how you picked these three, but it would be nice to add more.

Some related issues: https://github.com/mrdoob/three.js/issues/19305 https://github.com/mrdoob/three.js/pull/15490 - onFigureOutTextureBuffers

DusanOpenSpace commented 3 years ago

Additional note: instead calling empty callbacks like onBeforeRender i implemented those as the following, declaring them on the prototype with null: if ( object.onProject instanceof Function && object.onProject( ... ) === false ) return;

When onBeforeRender was being implemented, there was this (not sure if it's relevant anymore):

https://github.com/mrdoob/three.js/pull/9738#issuecomment-248985168

I actually tested, it seems twice as fast to just call an empty function than to test for null.

Fyrestar commented 3 years ago

I did some tests though i forgot console gives different results than actual scripts. However the difference there was interesting, instanceof performed in chrome couple hundred times faster than null test or empty call. Otherwise a empty call often is faster but sometimes equal, that's a really odd js quirk.

Not sure how you picked these three, but it would be nice to add more.

Those already give most control of the render routine (like the examples i mentioned) in a flexible component way without internal changes.

The more the merrier!

Yes especially for exposing internals there are many objects that would make it much more flexible if they were public.

DusanOpenSpace commented 3 years ago

Interesting, so the null test is the biggest offender, but instanceof can be either insignificantly slower, or much much faster?

There are some internal objects such as frustum, textures, geometries, properties, objects that are as well private yet, i needed those for various cases in those callbacks, but especially for onRender.

I might be misinterpreting this, but it seems related to:

Those already give most control of the render routine (like the examples i mentioned) in a flexible component way without internal changes.

What i hear is that you would like to pass a few of these internal objects into the callbacks? If that is so, could it indicate that these three may not be enough / most flexible / give most control? It may be worth investigating this as a more generic pattern - the renderer has many internal objects, which ones would be worth exposing and when.

Fyrestar commented 3 years ago

Interesting, so the null test is the biggest offender, but instanceof can be either insignificantly slower, or much much faster? The null test seems logical, i'd guess a if ( object.onBeforeRender ) could be faster, but calling a empty function is much less obvious, especially since the empty ones don't have any parameters declared while parameters are passed. But that also shows how conditions can get costly in hot places, and THREE unfortunately still has a condition forest in the WebGLRenderer, which could be uncluttered and get faster by references and class specific implementations, such as material specific handling being on their prototype rather than if ( material.isMeshStandardMaterial || material.isMeshBasicMaterial ... ).

What i hear is that you would like to pass a few of these internal objects into the callbacks? If that is so, could it indicate that these three may not be enough / most flexible / give most control? It may be worth investigating this as a more generic pattern - the renderer has many internal objects, which ones would be worth exposing and when.

The mentioned internals make a lot sense to use outside as they are needed in a lot cases to make use of those callbacks, some internals make only sense internally, the mentioned ones really give most of control you could need. But there sure are more that would be reasonable to expose, i don't see a reason to keep them hidden honestly.

There would be one other callback that would be really helpful as well, i currently implemented it as a monkey patch plugin: https://discourse.threejs.org/t/chainable-onbeforecompile-uniforms-per-mesh/8905 (the uniforms per mesh part)

There is almost no project i don't need it, for some cases having uniform values stored on object/mesh level (or anywhere you need), so you can use 1 material instance for multiple objects, but use different uniform values per object/render call, without having to clone entire materials just for 1 different uniform. I currently implemented it through onBeforeRender which is okay as not always needed and not blocking it, but it would be better at the actual setup, to be implemented on the material class prototype or on the renderer.

Usnul commented 3 years ago

I believe that opening up the renderer would be a good idea generally, but I feel it would be hard to do well, as you're essentially making it part of the public (supported) API, which makes changing the renderer more difficult in the future without breaking compatibility in major ways.

Personally, I have similar issues as you, so something like this would help me too! I do think that what you're proposing is a bit hacky though, like injecting renderable primitives in onProject. Maybe it would be a good idea to have something like an "unsafe" or "protected" API set that would be exposed, but clearly documented as not being for the general public. I.e. "if you use this - you're on your own."

One more fear I have is the callback hell, as long as renderer invokes fixed number of callbacks per render cycle - that's simple, but if you have a callback per-object, it quickly starts to stray into performance-limiting territory.

DusanOpenSpace commented 3 years ago

Maybe it would be a good idea to have something like an "unsafe" or "protected" API set that would be exposed, but clearly documented as not being for the general public. I.e. "if you use this - you're on your own.

Can you elaborate on this? I've seen stuff like UNSAFE_dangerously_doFoo() and i'd be totally fine with that. But what does the "you're on your own" mean? Is one not on their own when they use another callback thats already present and not marked UNSAFE? Ie. how do you measure which callback is safer than others?

Usnul commented 3 years ago

that's easy, "UNSAFE" api has the power to completely destroy the state of your renderer, if you do something without understanding the implications - you will be stuck. If you do something that you think will work using UNSAFE api, but it does something unexpected (unspecified behavior) - that's fine. What I mean by "you're on your own" is - there is no support provided for this API at all and maintenance is not guaranteed, i.e. if the API completely changes or is removed in the next revision - you accept that.

Usnul commented 3 years ago

Personally, I think some users will understand and agree to this type of an api "contract" but majority will use it, abuse it, get frustrated and shout "wHy u n0 wrk?!11one" at the development community, and create a lot of wasteful threads on the forums.

DusanOpenSpace commented 3 years ago

Would you rename getContext so UNSAFE_getContext?

Usnul commented 3 years ago

As for examples of "safety" as such, three.js does a good job at resource management, it cleans up after itself. That's one type of safety. Another one would be state management, three.js will not try to render a material when uniforms have not been bound yet.

Usnul commented 3 years ago

Would you rename getContext so UNSAFE_getContext?

Ha, that's a matter of preference, but yes, that's an example of unsafe behaviour. I don't think it's too much though, since you can get access to the GL context without this API too. API classification can be hard, having a clear set of guidelines for such classification certainly helps. But this depends on each individual project and the level of API granularity.

DusanOpenSpace commented 3 years ago

I don't think it's too much though, since you can get access to the GL context without this API too.

Lol, good point, if you want to mess with everything, true you can get the context even without the API. Still i'm not convinced yet that this is a bad thing.

DusanOpenSpace commented 3 years ago

I think the other way to look at this, is there an API in three.js currently that could be deemed SAFE? Sure, some are safer than others, but all may be prone to sudden changes. I'd be more in favor mentioning in the docs that something is advanced, or unstable, what should be taken care of, what to keep an eye out for etc.

Mugen87 commented 8 months ago

This issue is a bit outdated since many things mentioned in the first post have changed.

Next to onBeforeRender() and onAfterRender(), onBeforeShadow() andonAfterShadow()is available sincer159`.

ImmediateRenderObject has been removed with r134.

In general, the policy is to expose internals of the renderer only when absolutely necessary. Most of the listed renderer classes are not designed to be for public use and can be changed or refactored at any time. Moreover, using this classes on app level will make it harder to move to WebGPURenderer.

Ideally, we identify common tasks developers want to do and provide dedicated APIs instead of allowing access to internals or raw WebGL logic.

If there is still the requirement for onProject(), I suggest to discuss this feature request in a new issue.