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

Exposing internals + onProject, onRender and onRenderShadow #20457

Closed
Fyrestar opened this issue Oct 5, 2020 · 15 comments
Closed

Exposing internals + onProject, onRender and onRenderShadow #20457

Fyrestar opened this issue Oct 5, 2020 · 15 comments

Comments

@Fyrestar
Copy link

Fyrestar commented Oct 5, 2020

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
Copy link

DusanOpenSpace commented Oct 5, 2020

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

Some related issues:
#19305
#15490 - onFigureOutTextureBuffers

@DusanOpenSpace
Copy link

DusanOpenSpace commented Oct 5, 2020

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):

#9738 (comment)

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

@Fyrestar
Copy link
Author

Fyrestar commented Oct 6, 2020

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
Copy link

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
Copy link
Author

Fyrestar commented Oct 12, 2020

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
Copy link
Contributor

Usnul commented Dec 5, 2020

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
Copy link

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
Copy link
Contributor

Usnul commented Dec 9, 2020

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
Copy link
Contributor

Usnul commented Dec 9, 2020

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
Copy link

Would you rename getContext so UNSAFE_getContext?

@Usnul
Copy link
Contributor

Usnul commented Dec 9, 2020

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
Copy link
Contributor

Usnul commented Dec 9, 2020

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
Copy link

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
Copy link

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
Copy link
Collaborator

Mugen87 commented Jan 10, 2024

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

Next to onBeforeRender() and onAfterRender(), onBeforeShadow() and onAfterShadow()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.

@Mugen87 Mugen87 closed this as completed Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants