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.

Sign up for GitHub

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

Jump to bottom

Changes in Pass and RenderPass #467

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
+27 −12
Open

Changes in Pass and RenderPass #467

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3d6493e
Optional type fixes in Pass, add support for `WebGLMultipleRenderTarg…
repalash May 25, 2023
87882c3
Run prettier
repalash May 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions types/three/examples/jsm/postprocessing/Pass.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { Material, WebGLRenderer, WebGLRenderTarget } from '../../../src/Three';
import { Material, WebGLMultipleRenderTargets, WebGLRenderer, WebGLRenderTarget } from '../../../src/Three';

export class Pass {
constructor();
Expand All @@ -12,10 +12,10 @@ export class Pass {
setSize(width: number, height: number): void;
render(
renderer: WebGLRenderer,
writeBuffer: WebGLRenderTarget,
readBuffer: WebGLRenderTarget,
deltaTime: number,
maskActive: boolean,
writeBuffer: WebGLMultipleRenderTargets | WebGLRenderTarget | null,
readBuffer?: WebGLMultipleRenderTargets | WebGLRenderTarget,
Comment on lines +15 to +16
Copy link
Contributor

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.

Copy link
Author

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.

Copy link
Contributor

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.

deltaTime?: number,
maskActive?: boolean,
): void;

dispose(): void;
Expand Down
29 changes: 22 additions & 7 deletions types/three/examples/jsm/postprocessing/RenderPass.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,13 +1,28 @@
import { Scene, Camera, Material, Color } from '../../../src/Three';

import {
Scene,
Camera,
Material,
Color,
WebGLMultipleRenderTargets,
WebGLRenderTarget,
WebGLRenderer,
} from '../../../src/Three';
import { Pass, FullScreenQuad } from './Pass';

export class RenderPass extends Pass {
constructor(scene: Scene, camera: Camera, overrideMaterial?: Material, clearColor?: Color, clearAlpha?: number);
scene: Scene;
camera: Camera;
overrideMaterial: Material;
clearColor: Color;
constructor(scene?: Scene, camera?: Camera, overrideMaterial?: Material, clearColor?: Color, clearAlpha?: number);
scene?: Scene;
camera?: Camera;
Comment on lines +13 to +15
Copy link
Contributor

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?

Copy link
Author

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.

Copy link
Contributor

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.

overrideMaterial?: Material;
clearColor?: Color;
clearAlpha: number;
clearDepth: boolean;

render(
renderer: WebGLRenderer,
_: WebGLMultipleRenderTargets | WebGLRenderTarget | null,
writeBuffer?: WebGLMultipleRenderTargets | WebGLRenderTarget,
Comment on lines +23 to +24
Copy link
Contributor

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

Copy link
Author

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

Copy link
Contributor

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?

deltaTime?: number,
maskActive?: boolean,
): void;
}