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

WebGLRenderer is not extendable #19305

Closed
taphos opened this issue May 5, 2020 · 22 comments
Closed

WebGLRenderer is not extendable #19305

taphos opened this issue May 5, 2020 · 22 comments
Labels

Comments

@taphos
Copy link

taphos commented May 5, 2020

Description of the problem

It is impossible to extend functionality of WebGLRenderer without changing library code or copy-pasting thousands of lines of code.

For example, I would like to change a single line of code in one of renderer helpers, i.e. WebGLRendererLists.
Currently my best choice is to copy WebGLRendererLists, change one line, copy WebGLRenderer and change single line of renderLists initialization. This basically makes library non-upgradable. Most projects, I have seen, dont bother to copy, they just change code in place and never upgrade threejs.

Suggestion

WebGLRendererParameters could include custom helpers, same as it includes WebGLRenderingContext object. This would make it possible to replace helpers without creating a copy of WebGLRenderer.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 5, 2020

Related comment: #19304 (comment)

@taphos
Copy link
Author

taphos commented May 5, 2020

@Mugen87 I agree that exposing internal helpers to public API would make them unchangeable. Do you think we could do some kind of gray area? i.e. make it possible to swap helpers in WebGLRenderer but "on your own risk", without guarantee that it will work same way in future versions.
As a threejs user I would prefer this over copy-pasting thousands lines of code.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 5, 2020

I personally would not go down this way and keep the renderer internals closed. I'm okay with having interfaces like Object3D.onBeforeRender() or WebGLRenderer.setOpaqueSort(), WebGLRenderer.setTransparentSort() that allow you to influence how the renderer performs its work in a controlled way. But exchanging entire components (like WebGLGeometries) is not something I would support in a 3D engine. Not even as a half-private/public API. If you really need to change the renderer that much, it makes much more sense from my point of view when developers fork the project and then apply their changes.

However, this design decision is up to @mrdoob.

@Mugen87 Mugen87 added the Design label May 5, 2020
@taphos
Copy link
Author

taphos commented May 5, 2020

Forking requires alot of management, most teams not willing to do. This leads to most threejs based projects to be stuck with some r57 prehistoric versions, making it hard for one developer to participate in multiple projects. Making functionality extendable atleast semi-publicly would help the community.

@taphos
Copy link
Author

taphos commented May 5, 2020

For the same reason many modern languages provide some kind of reflection API, which allows to access library internals. JavaScript allows for some parts to be completely "sealed", not accessible in any way, but doing so do no good for library users.

@pailhead
Copy link
Contributor

pailhead commented May 6, 2020

You are about to hit something that is set in stone :)

Mugen outlined the two basic opposing directions. I lie somewhere in between. These hooks mentioned are not very consistent, the same javascript code you write in onBeforeRender() basically won't work in an onBeforeCompile, yet some other onBeforeSomething is missing altogether.

I still like these hooks, i'd like there to be more of them, and i'd like some of them to be less buggy. However, just to better explain the situation even onBeforeRender took about a year and half a crusade to be accepted :)

I think the best way for the time being is to actually modify the code. To explain this situation better, if you look at some ancient version, you'll see that all of these renderer methods were accessible either on the instance or the prototype. So with ancient javascript you could "just" replace these at runtime. Since, methods and state has been made private, and having typescript on top of everything may complicate things a little bit.

Having said this, i think PRs that break the renderer even further are probably going to be an easier sell than a large attempt to make the renderer customizable. This may come naturally later. Ie. more areas need to be identified that would allow for more hooks.

I feel compassion for this:

Forking requires alot of management, most teams not willing to do.

It's sometimes not even the management, but the issue of trust/risk. It's much easier to ship some code with a hook, than modify a library.

@taphos
Copy link
Author

taphos commented May 6, 2020

@pailhead

ancient javascript you could "just" replace these at runtime. Since, methods and state has been made private

Do you know the reason for making them private? To disallow library users to depend on them?

@pailhead
Copy link
Contributor

pailhead commented May 6, 2020

I think encapsulation coupled with the language we have to use. It's not easy to make safe javascript.
I'd be nervous if i had to ship code that does something like:

function onLoad(){
  THREE.WebGLRenderer.prototype.foo = myFoo
}

But for experimenting, some temporary hacks and such, it'd be nice. Likewise, myFoo would make me less nervous, but if anyone could just override it with anything, that makes me nervous. JavaScript allows for exactly that :(

Now, something like this:

const componentOverrides = { WebGLTextures: MyWebGLTextures }
const myRenderer = new WebGLRenderer(options, componentOverrides )

I think that this is a neat pattern, but would probably work better with some things and not much sense with others. Something as complex as WebGL____ classes, i don't think you'd have much fun plugging those in. But ive encountered things where this would make sense. Shader chunks are something that'd be nice to have the ability to override on a per material level. More low level objects like render buffers, are locked by three, it'd be nice if one could have the option to manage them from outside though.

@taphos
Copy link
Author

taphos commented May 6, 2020

. THREE.WebGLRenderer.prototype.foo = myFoo

Agree, this is a strange feature of JS. Though es6+ class declaration allows to do the same, means soon all modern js code will allow such hacks. Probably it does not make sense to fight the language. (I mean to hide private functions, using tricks)

const componentOverrides = { WebGLTextures: MyWebGLTextures }
const myRenderer = new WebGLRenderer(options, componentOverrides )

This would open up alot of possibilities and algorithms to be implemented without forking. Currently some cool features can not be added to project examples, just because they require library code change.

@pailhead
Copy link
Contributor

pailhead commented May 6, 2020

es6 declaration is just syntactic sugar, the prototype is still accessible. I wouldn't exactly refer to these patterns as tricks, they are features of the language.

The overrides are worth discussing though, in general. It'd be amazing if certain things could be overridden .

@DefinitelyMaybe
Copy link
Contributor

Probably it does not make sense to fight the language.

right?

@NicolasRannou
Copy link
Contributor

Maybe not relevant at all but maybe you use proxy to replace a specific method if the change is well localized?
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy

@taphos
Copy link
Author

taphos commented May 7, 2020

@NicolasRannou thanx for the link. Unfortunately Proxy is not enough.

@mrdoob
Copy link
Owner

mrdoob commented May 8, 2020

This leads to most threejs based projects to be stuck with some r57 prehistoric versions, making it hard for one developer to participate in multiple projects.

Do you mind elaborating on that statement? What projects have you seen that are stuck in prehistoric versions?

@mrdoob
Copy link
Owner

mrdoob commented May 8, 2020

Do you know the reason for making them private? To disallow library users to depend on them?

Yes, it's easier to not break user code that way. I've personally seen more people having good experiences upgrading to new versions than not. But I guess that's not what you have seen.

@DefinitelyMaybe
Copy link
Contributor

ah. good to hear.

perhaps y'all are hanging out for support of private fields across browser's?

@pailhead
Copy link
Contributor

pailhead commented May 8, 2020

Do you mind elaborating on that statement? What projects have you seen that are stuck in prehistoric versions?

I think autodesk's forge is on 78 or something like that.

@taphos
Copy link
Author

taphos commented May 11, 2020

@mrdoob

Do you mind elaborating on that statement? What projects have you seen that are stuck in prehistoric versions?

Autodesk web viewer is using r71, I think
Trimble web viewer was stuck at r95 before I joined. It was not easy to make threejs upgradable.

@pailhead
Copy link
Contributor

I think that this would be great to collect some data for. The products I worked on were also stuck at older versions.

@taphos
Copy link
Author

taphos commented May 12, 2020

@mrdoob As far as I see it, there are 2 types of projects using threejs

  1. simple ones, which are mostly ok with builtin materials and existing features of the library - are easy to upgrade because they use public API only (or doing very simple customization, using available callbacks and shader materials). Probably those are mostly simple web games and student projects.
  2. projects which require rendering pipeline and material customization are forced to change threejs code, as functionality is strictly sealed and inaccessible otherwise.

The strategy of sealing functionality might help projects of type 1. not to "shoot them selves in the foot", but at the same time it is doing the opposite for projects of type 2. making customization and library upgrades harder.

@taphos
Copy link
Author

taphos commented Jun 4, 2020

Just upgraded from r115 to r117. Was pain in the ass, as I have a copy of WebGLRenderer with modifications, which is not compatible with r117 renderer anymore :(
I dont know how should I deal with this.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 4, 2020

Closing in favor of #19554.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants
@mrdoob @NicolasRannou @pailhead @DefinitelyMaybe @taphos @Mugen87 and others