Skip to content

Conversation

@Mugen87
Copy link
Collaborator

@Mugen87 Mugen87 commented Jan 11, 2019

This PR enables the usage of Uniform Buffer Objects for ShaderMaterial and RawShaderMaterial with WebGL 2. Users who build their own materials can use UBOs in order to manage global uniforms like camera or lighting related data more efficiently.

WebGLUniformBlocks encapsulates most parts of the UBO related code. The changes to WebGLRenderer are straightforward. At some point, upcoming build-in materials might also use WebGLUniformBlocks.

Related: #13700

@mrdoob
Copy link
Owner

mrdoob commented Mar 2, 2019

Looking good! How about UniformsGroup as naming instead?

@mrdoob mrdoob added this to the rXX milestone Mar 2, 2019
@Mugen87

This comment has been minimized.

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Mar 5, 2019

BTW: Thanks to @Alejandro_Insua from the forum for already testing this feature 👍

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 29, 2019

How about UniformsGroup as naming instead?

I've realized that we already have UniformsUtils so UniformsGroup is indeed a more consistent name.

@mrdoob mrdoob requested review from fernandojsg and takahirox May 1, 2019 04:40
@mrdoob mrdoob modified the milestones: rXX, r105 May 1, 2019
// the following two properties will be used for partial buffer updates

uniform.__data = new Float32Array( info.storage / Float32Array.BYTES_PER_ELEMENT );
uniform.__offset = offset;
Copy link
Collaborator

@takahirox takahirox May 8, 2019

Choose a reason for hiding this comment

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

Honestly I don't think spoiling uniform with __xxx properties is a clean approach. But I think it's ok so far. Let's clean up later if necessary.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

May I ask why you don't like this approach?^^

Copy link
Collaborator

Choose a reason for hiding this comment

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

For example, those properties are unnecessarily exposed to other modules which can access uniform (tho the property names begin with __ indicating it's private).

@fernandojsg
Copy link
Collaborator

Nice job @Mugen87 with this one. One probably obvious question that probably must be addressed in another PR instead of this one, is what about adding uniform support to the standard materials as most of the devs will be using them instead of custom ones, so we could really get the benefits of UBO in all the apps around

@takahirox
Copy link
Collaborator

He mentioned

At some point, upcoming build-in materials might also use WebGLUniformBlocks.

I'm making a test evaluating the performance if built-in material supports UBO. I'll share the result later.

@fernandojsg
Copy link
Collaborator

Ops I totally misread that part :D

@Mugen87
Copy link
Collaborator Author

Mugen87 commented May 8, 2019

is what about adding uniform support to the standard materials as most of the devs will be using them instead of custom ones, so we could really get the benefits of UBO in all the apps around

You are right, that's of course the long-term goal^^. But let's do it step by step. This PR should be (hopefully) a good basis for further enhancements. And we enable devs like @Alejandro_Insua to use UBOs in their custom shaders.


return this;

},
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you think some users may want to change the name runtime?

If I understand correctly name is required. And if we can think it's immutable, what do you think of taking name argument as constructor parameter and removing .setName()?

var group = new THREE.UniformsGroup( 'ViewData' );

Copy link
Collaborator Author

@Mugen87 Mugen87 May 8, 2019

Choose a reason for hiding this comment

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

Do you think some users may want to change the name runtime?

Maybe, although that would actually not make sense. It's like adding uniforms at runtime which would also be an invalid operation. Still, I think the current interface is cleaner than defining everything as ctor parameters.

I consider UBOs as an advanced WebGL feature so the API does not need to be foolproof from my point of view^^

Copy link
Collaborator

@takahirox takahirox left a comment

Choose a reason for hiding this comment

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

I commented some stuffs but I don't think they are big deals. Maybe we can enhance, optimize, and clean up later if needed. This PR is looking good as initial UBO support.

@takahirox
Copy link
Collaborator

I'm evaluating the performance. One feedback so far. Calling gl.getUniformBlockIndex() and gl.uniformBlockBinding() for every object every frame seems high cost. With the current implementation using UBO is slower here than using regular Uniforms.

@mrdoob mrdoob added this to the r117 milestone Apr 30, 2020
@mrdoob mrdoob modified the milestones: r117, r118 May 27, 2020
@mrdoob mrdoob modified the milestones: r118, r119 Jun 24, 2020
@mrdoob mrdoob modified the milestones: r119, r120 Jul 29, 2020
@mrdoob mrdoob modified the milestones: r120, r121 Aug 25, 2020
@mrdoob mrdoob modified the milestones: r121, r122 Sep 29, 2020
@mrdoob mrdoob modified the milestones: r122, r123 Oct 28, 2020
@mrdoob mrdoob modified the milestones: r123, r124 Nov 25, 2020
@mrdoob mrdoob modified the milestones: r124, r125 Dec 24, 2020
@mrdoob mrdoob modified the milestones: r125, r126 Jan 27, 2021
@mrdoob mrdoob modified the milestones: r126, r127 Feb 23, 2021
@mrdoob mrdoob modified the milestones: r127, r128 Mar 30, 2021
@takahirox
Copy link
Collaborator

Hi, we Mozilla Hubs wants UBO in Three.js. If the conflicts are resolved, can we get this PR merged? Or is there anything blocking this PR other than the conflicts?

@Mugen87
Copy link
Collaborator Author

Mugen87 commented Apr 1, 2021

@takahirox I've created a new PR (#21558) since a lot of things have changed over two years.

@Mugen87 Mugen87 closed this Apr 1, 2021
@Mugen87 Mugen87 removed this from the r128 milestone Apr 1, 2021
@Mugen87 Mugen87 removed the WebGL2 label Apr 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants