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.

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

Allow user to copy full buffer with gl.bufferData #9631

Merged
merged 10 commits into from Oct 22, 2016

Conversation

mattdesl
Copy link
Contributor

@mattdesl mattdesl commented Sep 6, 2016

Currently all GL buffers are updated with gl.bufferSubData — which makes sense most of the time for performance.

However, this means you need to use fixed-size buffers, and can't re-use a GL buffer with a larger or smaller array than before. This is a problem for geometry that needs to dynamically grow/shrink.

One solution is to dispose and re-create your THREE.Geometry instances every time they need to shrink/grow, but this can be a bit tedious for the end-user since they have to restructure their application logic to handle changing geometries.

Another solution is to have a flag (disabled by default) on BufferAttribute that tells the engine to update the entire contents of the array (using gl.bufferData) rather than only a sub-region. This way the same GL buffers can be re-used without deleting/re-creating them (causing needless noise in GL inspectors), and with the right abstractions the user will never need to change their application logic.

In practice this shows up with a library like three-bmfont-text since users may choose to add/remove glyphs (quads) at runtime. This PR will allow developers like myself more control over the GL buffers, allowing cleaner and more optimized abstractions without affecting the end-user's code.

Thanks! 😄

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2016

Shouldn't we just change the code so it looks like this instead?

        if ( data.dynamic === false || data.updateRange.count === - 1 ) {

            // Not using update ranges

            var usage = data.dynamic ? gl.DYNAMIC_DRAW : gl.STATIC_DRAW;
            gl.bufferData( bufferType, data.array, usage );

        } else ...

@mattdesl
Copy link
Contributor Author

mattdesl commented Sep 6, 2016

Yup, although there may be a performance impact on some GPUs when using bufferData by default, and most things built with ThreeJS will probably use fixed-length buffers.

I'm guessing that's why this uses bufferSubData instead of bufferData. So my PR forces the user to opt-in to receive this feature.

@mrdoob
Copy link
Owner

mrdoob commented Sep 6, 2016

Related:
https://www.opengl.org/discussion_boards/showthread.php/171684-BufferData-or-BufferSubData-VBO?p=1205534&viewfull=1#post1205534

So I guess what you really want is to dispose the current buffer?

@mattdesl
Copy link
Contributor Author

mattdesl commented Sep 6, 2016

Hmm nope, bufferData is used to allocate the array, which is needed if you have for e.g. a larger buffer than before. You do not need to dispose/delete any buffers to do this.

Example:

  • if you are re-positioning the vertices of a geometry at runtime you might want to use bufferSubData since the vertex count remains the same
  • if you are subdividing or decimating a geometry at runtime you might rather use bufferData since the vertex count will be different
  • if you no longer need the geometry then it might make sense to delete the buffers

@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2016

Hmm nope, bufferData is used to allocate the array, which is needed if you have for e.g. a larger buffer than before. You do not need to dispose/delete any buffers to do this.

Hmm, but bufferData is not going to reuse the previous allocated buffer, right? So this would become a "leak", no?

@mattdesl
Copy link
Contributor Author

mattdesl commented Sep 7, 2016

The bufferData call overwrites previous data (with new data, usage and size) so there is no leak. It replaces the old data.

http://stackoverflow.com/questions/20983416/if-you-call-glbufferdata-after-already-calling-it-on-a-buffer-is-there-a-memory

@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2016

@kenrussell any pointers on whether this is good practice? or whether we should be calling gl.deleteBuffer instead?

@kenrussell
Copy link

I think it's the best practice to not call gl.deleteBuffer and allocate a new one, but to optionally use gl.bufferData to reallocate the entire buffer object, as @mattdesl is suggesting. Deleting the old buffer and creating a new one does cause unnecessary churn.

@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2016

@kenrussell thanks!

@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2016

@kenrussell One more question actually... Is is slower to use gl.bufferData() instead of gl.bufferSubData() when updating a same-size buffer?

@kenrussell
Copy link

I think it's safest to continue to use bufferSubData to update same-sized buffers. There are arguments on both sides about it and I think it may actually be GPU-dependent whether it's faster or slower than bufferData. Chrome has some code which chooses the faster implementation in some cases. But Three.js seems to work just fine on a large range of devices at this point so I'd say if it isn't broken, don't fix it.

@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2016

@kenrussell thanks!

@mrdoob
Copy link
Owner

mrdoob commented Sep 7, 2016

Struggling to find a nice API for this...

@aardgoose
Copy link
Contributor

aardgoose commented Sep 9, 2016

There is a problem earlier in the process. A BufferAttribute's size is fixed at creation, when a Geometry vertices etc are expanded after GPU upload, The updates are processed by bufferAttribute.copy{x}( ) when updating the attached BufferGeometry.

This currently fails silently ( at least in Chrome). I hit this earlier this week, and resorted to copying into a new geometry,.

Maybe detect mismatch between Geometry attribute size and buffer attribute size at time of update. Then you could create a new TypedArray object of the correct size and at the same time trigger the use of gl.BufferData call (not something I have any insight into). This would mainly be contained in BufferAttribute.copy() methods with small mods to the renderer for the gl call variation.

Thoughts? Maybe totally wrong here, still learning.

This would be totally transparent and not even require a user facing API.

@mattdesl
Copy link
Contributor Author

mattdesl commented Sep 10, 2016

@aardgoose Where is the buffer size fixed? gl.bufferData modifies the buffer to the new size, but you can call it again to change the buffer size. BufferAttribute doesn't store the size currently, it's in a getter that computes the size based on the current array and itemSize.

If we wanted to make this fully hidden to the user (i.e. they have no control over bufferData vs bufferSubData) then you could use bufferData the first time, and then bufferSubData on subsequent updates unless there is a size mismatch with the previous update, in which case bufferData is used. This might get a bit complex and you may run into other edge cases.

I think the simplest approach is just to place this on the end-user. For the API — instead of a boolean, you could use a "strategy" flag:

var attr = new BufferAttribute(..);
attr.bufferStrategy = THREE.BufferDataStrategy;
// attr.bufferStrategy = THREE.BufferSubDataStrategy;

Or something like that.

@aardgoose
Copy link
Contributor

That is the issue - the BufferAttribute internal TypedArray is fixed at creation time ie: new" BufferAttribute();".

Typed arrays are fixed size, there isn't a method of expanding them.

The copy() methods used when the buffer attributes are changed simply silently ignore writes pass the end of the array so:

var a = new Uint8Array( [ 0, 1, 2, 3 ] ); // create length 4 array
a[ 6 ] = 3; // fails silently*, giving you the false sense that there are no issues there.

  • checked on IE, Edge Chrome and FF, so I assume this is standard behaviour.

The TypedArray.set() method is the only access method that seems to care and raises an exception.

So to have variable sized BufferAttributes, the .array typedArray needs to be change on demand, the expanded geometry currently never gets near the gl.* calls, it fails before then (silently).

@mattdesl
Copy link
Contributor Author

Typed arrays are fixed size, there isn't a method of expanding them.

Sure, but the user should be able to do this:

attrib.array = new Float32Array(...);
attrib.needsUpdate = true;

😄

@aardgoose
Copy link
Contributor

True, there is nothing to stop them doing that, but there is nothing that says that it should work, it's fiddling with what is really a private property of the object, or would be in another language. If it breaks something, bad luck. Look at Object3D etc when certain properties like position have been made un-replaceable to stop foot/gun incidents.

If you checked incoming length on the copy methods you could get automatic resizing after altering a Geometry() with no user changes, and if you really want to play directly with the BufferAttribute like that, surely it would be better to use the copyArray() method and adjust that to check for length changes, and you could get needsUpdate and any other flag setting for free. Simple for the user and insulates them from any changes in the future. Resizing, however done is going to be fairly costly, so having a proper API won't add much overhead.

Anyway, not my library or decision, I am just very happy that it exists and works well.

@mattdesl
Copy link
Contributor Author

mattdesl commented Sep 12, 2016

I didn't think .array is a private or read-only property, I've mutated it on some other projects in the past and it's in the docs.

I'm pretty OK with any change that lets AttributeBuffer grow/shrink dynamically. The documentation should be easy enough to fix. I can change my PR if we decide on the best way to go.

@WestLangley
Copy link
Collaborator

I'm pretty OK with any change that lets AttributeBuffer grow/shrink dynamically.

@mattdesl Can you allocate a sufficiently-large, fixed-sized buffer and use geometry.drawRange?

See http://stackoverflow.com/questions/31399856/drawing-a-line-with-three-js-dynamically/31411794#31411794.

@aardgoose
Copy link
Contributor

What I meant that I think it would be private in other languages, rather than actually is (I probably should have said that more explicitly)

I imagine most people use the standard method, at least most of Three.js objects do. If we have a method to allow new arrays of different sizes to be setup as suggested by mrdoob in the other PR, that allows the count field to be maintained, variable array size and the discard method can my later PR can co-exist happily, which is my personal concern. Plus it hides renderer nicely.

@mattdesl
Copy link
Contributor Author

@WestLangley yup, but it's a pain and eventually will break when your array is not "sufficiently large enough." It's not a good solution IMHO when gl.bufferData works just fine and is what other engines would use to grow/shrink vertex attributes.

@mrdoob
Copy link
Owner

mrdoob commented Sep 14, 2016

How about adding a setArray() method would then update count and also set a needsFullUpdate or maybe needsUpload to true?

@mattdesl
Copy link
Contributor Author

mattdesl commented Sep 23, 2016

I pushed a change to update the API with setArray and needsFullBuffer.


How about adding a setArray() method would then update count and also set a needsFullUpdate or maybe needsUpload to true?

Hmm, the only issue is that the method introduces a side-effect that users might not be expecting. For example, they might use setArray even though the new array is the same size, so it doesn't actually need to use bufferData. All of a sudden, the engine will be using a different strategy for buffer uploads.

We could check in setArray whether the new size is different than the old size, though it sounds like it's getting a bit fragile/complex. This is why I felt like it was best as a flag that advanced users can opt-in to.

We could call the boolean dynamicSize, and in the docs suggest users set it to true if they expect to have different sizes in setArray() calls. Otherwise all buffer attributes are expected to be fixed sized (as they are now).

@mattdesl
Copy link
Contributor Author

@mrdoob @aardgoose What do you guys think?

@aardgoose
Copy link
Contributor

I don't have any issues, I think you worries about side effects regarding strategies shouldn't be an issue, It is a new API so there are no prior expectations. Adding some documentation for setArray() that makes this behaviour explicit would help.

@mrdoob
Copy link
Owner

mrdoob commented Oct 12, 2016

Hmm... @kenrussell suggested that maybe a better API would be adding a setSize() method to BufferAttribute. So we can grow/shrink the array internally and set the necessary flags. Somehow seems more intuitive than doing setArray() and I think it also fits your needs @mattdesl?

@mattdesl
Copy link
Contributor Author

mattdesl commented Oct 13, 2016

@aardgoose I think you worries about side effects regarding strategies shouldn't be an issue, It is a new API so there are no prior expectations

I am talking about side-effects in the functional sense. Generally, it's not good for methods to introduce implicit side-effects, and it's better to force the user to explicitly mutate state.

Let's say setSize looks like this:

    setSize: function ( arrayLength ) {

        if ( typeof arrayLength !== 'number' ) {

            throw new TypeError('Must specify a number as arrayLength to setSize()');

        }

        var oldCount = this.count;
        this.count = arrayLength / this.itemSize;

        if ( !this.needsFullBuffer && this.count !== oldCount ) {

            this.needsFullBuffer = true;

        }

    }

Now take the following end-user code (which is not uncommon, since some users set the data some time after the constructor).

const attr = new THREE.BufferAttribute();
...
attr.itemSize = 2;
attr.array = new Float32Array(myData);
attr.setSize(attr.array.length);

The unexpected thing here is that now this attribute will always copy with bufferData().

Ideally, a graphics engine might only update the full bufferData() when it notices the data is a different length than before, and then subsequent calls with the same length can continue to use bufferSubData(). This is (a) more complex to implement, (b) most likely a micro optimization, and (c) may not be desirable to users who specifically want to target one method or the other (e.g. if they are targeting certain GPUs that excel over one or the other).

And, more generally, attr.setSize introduces a bit of redundancy. You can only change the size by changing the array field, and to avoid errors the size should always match the new array size. So the user will always have to write this:

attr.array = newArray;
attr.setSize(attr.array.length);

Instead of the following, which has the same effect:

attr.setArray(newArray);

Anyways... my opinion is still to force the needsFullUpdate on the end-user. Only a fraction of users will need to grow/shrink attributes, and it's easy enough for those users to just set the field manually. It also gives them the most control over how the data is uploaded (e.g. if they are targeting a specific GPU). So we can keep the PR as it is now, and suggest the following if the user wants to grow/shrink the attribute:

const attr = new THREE.BufferAttribute(data, 2);

// allow this attribute to grow/shrink
attr.needsFullBuffer = true;

...

// to grow/shrink the data
attr.setArray(newArray);

@mrdoob
Copy link
Owner

mrdoob commented Oct 15, 2016

More thinking about this...

Aren't we always calling bufferData() when this.version is 0? How about we set this.version to 0 inside attribute.setArray() and add the additional logic in WebGLObjects?

@mrdoob mrdoob mentioned this pull request Oct 15, 2016
3 tasks
@mattdesl
Copy link
Contributor Author

mattdesl commented Oct 15, 2016

I updated the PR, now setArray will reset the version to zero. The only thing is that if you do this, it will not work:

attr.setArray(newData);
attr.needsUpdate = true;

Because needsUpdate always increments the version field, and we are only testing against version === 0. Another option is to set an internal value in the buffer, like _needsCreateBuffer, and test against that in WebGLObjects. After updating the full buffer, WebGLObjects can set _needsCreateBuffer back to false.

@mrdoob
Copy link
Owner

mrdoob commented Oct 18, 2016

I updated the PR, now setArray will reset the version to zero. The only thing is that if you do this, it will not work:

attr.setArray(newData);
attr.needsUpdate = true;

Ugh. Yeah...

Hmm, I'm starting to think that the right thing to do here is to go the dumb (thus predicable) approach and just do this:

function updateBuffer( attributeProperties, data, bufferType ) {

    gl.bindBuffer( bufferType, attributeProperties.__webglBuffer );

    if ( data.dynamic === false ) {

        gl.bufferData( bufferType, data.array, gl.STATIC_DRAW );

    } else if ( data.updateRange.count === - 1 ) {

        // Not using update ranges

        gl.bufferSubData( bufferType, 0, data.array );

    } else if ( data.updateRange.count === 0 ) {

        console.error( 'THREE.WebGLObjects.updateBuffer: dynamic THREE.BufferAttribute marked as needsUpdate but updateRange.count is 0, ensure you are using set methods or updating manually.' );

    } else {

        gl.bufferSubData( bufferType, data.updateRange.offset * data.array.BYTES_PER_ELEMENT,
                            data.array.subarray( data.updateRange.offset, data.updateRange.offset + data.updateRange.count ) );

        data.updateRange.count = 0; // reset range

    }

    attributeProperties.version = data.version;

}

I realised that the use cases where this could affect performance are when users update the BufferAttribute which is actually not a common use case. Most of code just loads a geometry and uses bufferData() once anyway.

We also gain control on being able to use bufferData() or bufferSubData() with the dynamic flag.

@mattdesl
Copy link
Contributor Author

That sounds good to me! I'll update the PR tomorrow. 😊

@mattdesl
Copy link
Contributor Author

Ok @mrdoob I've updated this PR. 😄 Let me know! Thanks.

@mrdoob mrdoob merged commit 348b72e into mrdoob:dev Oct 22, 2016
@mrdoob
Copy link
Owner

mrdoob commented Oct 22, 2016

Thaaaanks!

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.

None yet

5 participants