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

WebGPURenderer: Fix global references in Node.js #29919

Merged
merged 6 commits into from
Nov 24, 2024
Merged

WebGPURenderer: Fix global references in Node.js #29919

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
df963e1
Fix compilation in react
whatisor Nov 19, 2024
1cb2183
Cleanup fixes
whatisor Nov 19, 2024
0b09367
Fix space
whatisor Nov 19, 2024
78800ae
Fix space
whatisor Nov 19, 2024
a32f487
Update WebGPUBackend.js
Mugen87 Nov 19, 2024
07c95d6
Update WebGPUUtils.js
Mugen87 Nov 24, 2024
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
2 changes: 1 addition & 1 deletion src/renderers/webgpu/WebGPUBackend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class WebGPUBackend extends Backend {
powerPreference: parameters.powerPreference
};

const adapter = await navigator.gpu.requestAdapter( adapterOptions );
const adapter = ( typeof navigator !== 'undefined' ) ? await navigator.gpu.requestAdapter( adapterOptions ) : {};
Copy link
Collaborator

@Mugen87 Mugen87 Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@donmccurdy{} is intended as a workaround for node but I'm not sure {} is ideal here. Do you think this use case is handled appropriately or would you implement it in a different way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Base on that function code, it should be null, otherwise, next step is exceptional?
"adapter.features"

Copy link
Collaborator

@donmccurdy donmccurdy Nov 19, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@whatisor are you executing this code in the Node.js environment and seeing runtime errors, or is this a compile-time error (like from TypeScript or Webpack)? Not clear to me from #29916.

I'm not sure that backend.init( renderer ) should be making efforts to avoid runtime exceptions, if running in an environment where neither WebGL nor WebGPU exists. I would support fixes that avoid build-time errors, and run-time errors in module scope. But if you are initializing and using a renderer in Node.js, a runtime exception is to be expected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure that backend.init( renderer ) should be making efforts to avoid runtime exceptions, if running in an environment where neither WebGL nor WebGPU exists

I agree, I think we should be throwing an error here, it doesn't make much sense to me to proceed with an empty object as an adapter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@donmccurdy it is compile error.
We also need to fix three\examples\jsm\capabilities\WebGPU.js with global and await.
So, we should return null and (exception is next step as current)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

three\examples\jsm\capabilities\WebGPU.js

Capabilities should be optional, it should be possible to use WebGPURenderer without global await. #29218

Copy link
Contributor Author

@whatisor whatisor Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

can we do same thing with webgl to avoid await?
https://developer.mozilla.org/en-US/docs/Web/API/GPUCanvasContext

=> No, we still need to await for real adapter. I guess browser need to improve this part.


if ( adapter === null ) {

Expand Down
6 changes: 3 additions & 3 deletions src/renderers/webgpu/nodes/WGSLNodeBuilder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ import { GPUBufferBindingType, GPUStorageTextureAccess } from '../utils/WebGPUCo
import { NoColorSpace, FloatType } from '../../../constants.js';

// GPUShaderStage is not defined in browsers not supporting WebGPU
const GPUShaderStage = self.GPUShaderStage;
const GPUShaderStage = ( typeof self !== 'undefined' ) ? self.GPUShaderStage : { VERTEX: 1, FRAGMENT: 2, COMPUTE: 4 };

const gpuShaderStageLib = {
'vertex': GPUShaderStage ? GPUShaderStage.VERTEX : 1,
Expand Down Expand Up @@ -123,7 +123,7 @@ const wgslMethods = {

// WebGPU issue: does not support pow() with negative base on Windows

if ( /Windows/g.test( navigator.userAgent ) ) {
if ( typeof navigator !== 'undefined' && /Windows/g.test( navigator.userAgent ) ) {

wgslPolyfill.pow_float = new CodeNode( 'fn tsl_pow_float( a : f32, b : f32 ) -> f32 { return select( -pow( -a, b ), pow( a, b ), a > 0.0 ); }' );
wgslPolyfill.pow_vec2 = new CodeNode( 'fn tsl_pow_vec2( a : vec2f, b : vec2f ) -> vec2f { return vec2f( tsl_pow_float( a.x, b.x ), tsl_pow_float( a.y, b.y ) ); }', [ wgslPolyfill.pow_float ] );
Expand All @@ -141,7 +141,7 @@ if ( /Windows/g.test( navigator.userAgent ) ) {

let diagnostics = '';

if ( /Firefox|Deno/g.test( navigator.userAgent ) !== true ) {
if ( ( typeof navigator !== 'undefined' && /Firefox|Deno/g.test( navigator.userAgent ) ) !== true ) {

diagnostics += 'diagnostic( off, derivative_uniformity );\n';

Expand Down
14 changes: 11 additions & 3 deletions src/renderers/webgpu/utils/WebGPUUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,13 +113,21 @@ class WebGPUUtils {
// TODO: Remove this check when Quest 34.5 is out
// https://github.com/mrdoob/three.js/pull/29221/files#r1731833949

if ( navigator.userAgent.includes( 'Quest' ) ) {
if ( typeof navigator !== 'undefined' ) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we remove this? It is inside a function that will never be executed by the adapter return null in this case.

Copy link
Collaborator

@donmccurdy donmccurdy Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I guess the issue is that it's a compiler error, not a runtime error? I wish we knew what in the Next.js stack is doing compile-time checks on dependencies, and what its requirements are. Avoiding all references to browser globals is a pretty tall order for a large library relying heavily on Web APIs. :/

Copy link
Collaborator

Choose a reason for hiding this comment

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

The compiler throwing an error for a global variable inside a function seems strange to say the least, since a variable can be declared at the global level after the function declaration.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I'm not sure why... trying to reproduce the issue:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is this one:
https://react.dev/reference/react/StrictMode


return GPUTextureFormat.BGRA8Unorm;
if ( navigator.userAgent.includes( 'Quest' ) ) {

return GPUTextureFormat.BGRA8Unorm;

} else {

return navigator.gpu.getPreferredCanvasFormat();

}

} else {

return navigator.gpu.getPreferredCanvasFormat();
return GPUTextureFormat.BGRA8Unorm;

}

Expand Down