Add RenderTarget|null typing + remove dead code

[kungfooman]

This fixes a few of type issues, since RenderTarget is null in many situations, and then it goes on and on to e.g. drawQuadWithShader which doesn't need a target, e.g. example with bloom:

I confirm I have read the contributing guidelines and signed the Contributor License Agreement.

[kungfooman]

We have in some functions strict "no-undefined-allowed" assertions:

function drawQuadWithShader(device, target, shader, rect, scissorRect) {
    // a valid target or a null target (framebuffer) are supported
    Debug.assert(target !== undefined);

But then we still have places where undefined creeps in again:

Should we try to keep it RenderTarget|null everywhere? From a clean type perspective, it makes most sense to me here 🤔

[slimbuck]

Should we try to keep it RenderTarget|null everywhere? From a clean type perspective, it makes most sense to me here 🤔

Probably a question for @mvaligursky

[mvaligursky]

Should we try to keep it RenderTarget|null everywhere? From a clean type perspective, it makes most sense to me here

undefined is valid in some cases, so we cannot force it to null in those cases.

[kungfooman]

undefined is valid in some cases, so we cannot force it to null in those cases.

Interesting, so I tried to find the these methods called with undefined as renderTarget in a running app (UI ScrollView example):

I found these two:

engine/src/platform/graphics/render-pass.js

Lines 188 to 221 in bbb7961

	/**
	* @param {import('../graphics/render-target.js').RenderTarget} renderTarget - The render
	* target to render into (output). This function should be called only for render passes which
	* use render target, or passes which render directly into the default framebuffer, in which
	* case a null or undefined render target is expected.
	*/
	init(renderTarget) {
	// null represents the default framebuffer
	this.renderTarget = renderTarget || null;
	// defaults depend on multisampling
	this.samples = Math.max(this.renderTarget ? this.renderTarget.samples : this.device.samples, 1);
	// allocate ops only when render target is used
	this.depthStencilOps = new DepthStencilAttachmentOps();
	const numColorOps = renderTarget ? renderTarget._colorBuffers?.length : 1;
	for (let i = 0; i < numColorOps; i++) {
	const colorOps = new ColorAttachmentOps();
	this.colorArrayOps[i] = colorOps;
	// if rendering to single-sampled buffer, this buffer needs to be stored
	if (this.samples === 1) {
	colorOps.store = true;
	colorOps.resolve = false;
	}
	// if render target needs mipmaps
	if (this.renderTarget?._colorBuffers?.[i].mipmaps) {
	colorOps.mipmaps = true;
	}
	}
	}

engine/src/framework/components/camera/component.js

Lines 905 to 917 in bbb7961

	/**
	* Prepare the camera for frame rendering.
	*
	* @param {import('../../../platform/graphics/render-target.js').RenderTarget} rt - Render
	* target to which rendering will be performed. Will affect camera's aspect ratio, if
	* aspectRatioMode is {@link ASPECT_AUTO}.
	* @ignore
	*/
	frameUpdate(rt) {
	if (this.aspectRatioMode === ASPECT_AUTO) {
	this.aspectRatio = this.calculateAspectRatio(rt);
	}
	}

If these are called correctly, I can just mark them as [renderTarget] (meaning |undefined).

I feel uncertain here though, in first code snippet, line 197:

this.renderTarget = renderTarget || null; 

So it could be called with undefined but will be immediately updated to null in case of undefined? Would be really nice to get the types right here, because they cause type invalidations in RuntimeTypeInspector.js every frame:

[mvaligursky]

So the RendePass.renderTarget is undefined if nothing is set to it, so that is the default uninitialized state.
if RenderPass.init is called, it can only update that member to a valid rt, or NULL (backbuffer), but never to uninitialized.

so only renderTarget needs to have all 3 types allowed, but init function only needs two.

[kungfooman]

if RenderPass.init is called, it can only update that member to a valid rt, or NULL (backbuffer), but never to uninitialized.

so only renderTarget needs to have all 3 types allowed, but init function only needs two.

Hm, we still call RenderPass#init with undefined aswell, shouldn't we then just do init(renderTarget = null) so the type system picks up that calling init with undefined is allowed?

(this happens in ForwardRenderer#addMainRenderPass)

[mvaligursky]

init(renderTarget = null)

perfect! Go for it.

[mvaligursky]

Nice one, thanks!