-
-
Notifications
You must be signed in to change notification settings - Fork 183
Changes in Pass and RenderPass #467
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
base: master
Are you sure you want to change the base?
Conversation
…ets`, Add render function in RenderPass and make properties optional.
| constructor(scene?: Scene, camera?: Camera, overrideMaterial?: Material, clearColor?: Color, clearAlpha?: number); | ||
| scene?: Scene; | ||
| camera?: Camera; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you provide an example or show in the code where scene or camera can be optional/undefined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One common way is to pass undefined in the constructor and assign the camera, scene before rendering. This way passes can be initialized before the scene/camera init/load.
But if its confusing I can remove this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm just concerned about the properties becoming optional since that would cause any access of those properties to assert that they're not undefined or check for it. I would feel better if there were some official three.js examples that show this pattern.
| _: WebGLMultipleRenderTargets | WebGLRenderTarget | null, | ||
| writeBuffer?: WebGLMultipleRenderTargets | WebGLRenderTarget, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the parameters be readBuffer and writeBuffer (in that order)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In three.js RenderPass.js, the first parameter is ignored and the second parameter is used as writeBuffer.
The autocomplete always creates confusion since the params are not write, read but ignored, write
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So the variables are misnamed in the three.js source code?
| writeBuffer: WebGLMultipleRenderTargets | WebGLRenderTarget | null, | ||
| readBuffer?: WebGLMultipleRenderTargets | WebGLRenderTarget, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the better fix would be to have WebGLMultipleRenderTargets extend WebGLRenderTarget like it does in the three.js source code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but since the type of texture is an object in WebGLRenderTarget and an array in WebGLMultipleRenderTargets, its not possible to inherit and change type of the property in typescript. This seemed to be the simplest solution for Passes atleast.
One way is to make texture to be Texture|Texture[] in WebGLRenderTarget but that might break a lot of projects and require special type casting at many places.
Another way is to make an interface like IWebGLRenderTarget and make both implement it.
I am open to other ideas, would be good to fix type handling of WebGLMultipleRenderTargets.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, I've opened #519 to resolve this.
Changes in Pass and RenderPass
What
Allow
WebGLMultipleRenderTargetsinPass.renderfor read and write buffers. This is required becauseWebGLMultipleRenderTargetsdoes not inherit from WebGLRenderTarget in this types repo(but it does in three.js)Allow
readBuffer,deltaTime,maskActiveto be optional in Pass.Add render function in RenderPass, which is slightly different from Pass.
Checklist
master, next goesdev)