WebGLRenderer: Change scissor, viewport functions to use "round" instead of "floor"

[gkjohnson]

Fixed #27655

Description

Use "round" instead of "floor" to compute the scissor and viewport functions to prevent rounding errors. From the WebGL spec there doesn't seem to be any requirement that the max value be within the width of the canvas so no value clamp has been added.

[github-actions]

📦 Bundle size

Full ESM build, minified and gzipped.

Filesize dev	Filesize PR	Diff
676.2 kB (167.9 kB)	676.2 kB (167.9 kB)	+0 B

🌳 Bundle size after tree-shaking

Minimal build including a renderer, camera, empty scene, and dependencies.

Filesize dev	Filesize PR	Diff
456.8 kB (110.7 kB)	456.8 kB (110.8 kB)	+0 B

[gkjohnson]

Do we know if there's anything unique about the device pixel ratio in the CI checks? When generating a new screenshot locally for "webgl_effects_peppersghost" I'm seeing that there's no diff but in CI it's failing.

dev screenshot	artifact screenshot	delta screenshot

[mrdoob]

What I started doing for these cases is to download the screenshot that the CI generated and add it to the PR... 😇

You can get it here:
https://github.com/mrdoob/three.js/actions/runs/7814629999/artifacts/1227023113

[WestLangley]

#18326 may need to be revisited if this is merged. It was required because of the use of .floor().

[gkjohnson]

#18326 may need to be revisited if this is merged. It was required because of the use of .floor().

This isn't something I'd be able to test, unfortunately. cc @elalish

[elalish]

Well, it should be pretty obvious with a Pixel and this example: #18265 (comment)

I can't test just now either, but I will once I pull this into MV.

[gkjohnson]

Looking at the PR - it might be best to set the viewport settings on the render target rather than the renderer. Setting the viewport on the render target itself (here) allows you to set exact pixel values instead of dealing with these rounding issues and DPR.

The discrepancy in behavior when dealing with WebGLRenderTarget.viewport and WebGLRenderer.setViewport (and scissor) is fairly confusing and it might be worth looking into how to simplify this another time.

[donmccurdy]

From the WebGL spec there doesn't seem to be any requirement that the max value be within the width of the canvas so no value clamp has been added.

Examples like https://threejs.org/examples/#webgl_multiple_elements rely on being able to set scissor and viewports that extend partially offscreen, so +1 for not clamping this. 🙂

[Mugen87]

@gkjohnson Do you mind temporarily updating the build files in this PR? I can make a test with a Pixel 4a which has a device pixel ratio of 2.75. If there is an issue with PMREM, examples like webgl_materials_envmaps_exr or webgl_materials_physical_clearcoat should show the banding artifacts from #18265.

[gkjohnson]

In #18447 the _setViewport function in PMREMGenerator was already refactored to remove use of WebGLRenderer.setViewport in favor of "WebGLRenderTarget.viewport" so this issue shouldn't be a concern anymore.

[WestLangley]

In at least six other instances in the code, floor() is still used when applying pixelRatio. Was this an oversight, or was this intentional?

/ping @Mugen87
/ping @gkjohnson

[Mugen87]

To me that is an oversight. When looking at the code, the usage in setRenderTarget() should be updated to round() as well.

three.js/src/renderers/WebGLRenderer.js

Lines 2286 to 2287 in 0bc990f

	_currentViewport.copy( _viewport ).multiplyScalar( _pixelRatio ).floor();
	_currentScissor.copy( _scissor ).multiplyScalar( _pixelRatio ).floor();

Not sure about getDrawingBufferSize() though.

three.js/src/renderers/WebGLRenderer.js

Line 412 in 0bc990f

return target.set( _width * _pixelRatio, _height * _pixelRatio ).floor();

Depending on how we decide, the code in examples/jsm/renderers/common/Renderer.js should be updated accordingly.