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

WebGLMultipleRenderTargets: Add new class. #85

Closed
wants to merge 2 commits into from

Conversation

Mugen87
Copy link
Contributor

@Mugen87 Mugen87 commented May 27, 2021

Why

A new type of render target was added with mrdoob/three.js#16390.

What

Adds type declaration file for WebGLMultipleRenderTargets.

Checklist

  • Added myself to contributors table
  • Ready to be merged

joshuaellis
joshuaellis previously approved these changes May 27, 2021
Copy link
Member

@joshuaellis joshuaellis left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! ⭐

@Mugen87
Copy link
Contributor Author

Mugen87 commented May 28, 2021

Seems the linter reports an error. Do you have an idea what is going wrong?

@joshuaellis
Copy link
Member

WebGLMultipleRenderTargets extends WebGLRenderTarget so TS expects the property texture to be the same type...

@Methuselah96 Do you have an ideas how we might resolve this? 🤔 I always find it odd that you can override methods but not properties.

Although @Mugen87, looking at the THREE code, wouldn't calling WebGLMultipleRenderTargets.setTexture fail because i'd either have to supply an array which would break:

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

see here.

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

@Mugen87
Copy link
Contributor Author

Mugen87 commented May 28, 2021

It seems setTexture() should not be used in context of WebGLMultipleRenderTargets since it maintains multiple textures in a special way. If you call the method with a single texture, the code will crash.

@joshuaellis
Copy link
Member

Yeah, so this afaik wouldn't be 'allowed' in typescript... So we may need to copy the class WebGLRenderTarget and remove certain bits as opposed to extending from... 🤔

@Mugen87
Copy link
Contributor Author

Mugen87 commented May 29, 2021

Closing this PR. I'm afraid the mentioned problems exceed my TS knowledge^^. I hope somebody else can tackle this feature.

BTW: It was considered in mrdoob/three.js#16390 (comment) to not extend the class from WebGLRenderTarget. Maybe it's a good idea to highlight the typing issues at the three.js repo. Seems the class design could be cleaner if the inheritance is changed.

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

Successfully merging this pull request may close these issues.

2 participants