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

Should WebGLMultipleRenderTargets extend WebGLMultipleRenderTarget? #21930

Closed
joshuaellis opened this issue May 31, 2021 · 4 comments
Closed
Labels

Comments

@joshuaellis
Copy link
Contributor

Is your feature request related to a problem? Please describe.

Introduced here – #16390, WebGLMultipleRenderTargets extends WebGLMultipleRenderTarget which on the surface works, however when we came to create the .d.ts file – three-types/three-ts-types#85 we began to discuss how maybe this shouldn't be the case. Calling WebGLMultipleRenderTargets.setTexture would fail because you'd either have to supply an array which would break:

texture.image = {
	width: this.width,
	height: this.height,
	depth: this.depth
};

see:

Alternatively not passing an array of Textures, would break the WebGLMultipleRenderTargets.

Describe the solution you'd like

Whilst I appreciate how helpful it is to extend a lot of the features already written in WebGLMultipleRenderTarget, maybe there should be a utility/base class that they both inherit from, this would reduce duplication (which is why we extend classes in the first place) and equally avoid the edge cases described above that will impact users.

Additional context

For anyone interested, the .d.ts file will probably just duplicate the properties and methods included in WebGLMultipleRenderTarget

@Mugen87 Mugen87 added the Design label May 31, 2021
@joshuaellis
Copy link
Contributor Author

Would it be better if I mocked up a proposal in a PR? Or would we rather discuss the issue here first?

@Mugen87
Copy link
Collaborator

Mugen87 commented Jun 13, 2021

I guess it's easier to see the impact of a change with a PR.

@mbredif
Copy link
Contributor

mbredif commented Aug 6, 2021

Does #22089 solves this issue ?

@Mugen87
Copy link
Collaborator

Mugen87 commented Nov 21, 2022

Since #23498 removes WebGLRenderTarget.setTexture(), there is no inconsistency with WebGLMultipleRenderTargets.setTexture() anymore. So the inheritance itself should not cause any (type) issues anymore.

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

3 participants