Discard attribute typed arrays for buffered geometries that are not modified after initial rendering #9512

Merged
merged 25 commits into from Nov 5, 2016

Projects

None yet

5 participants

@aardgoose
Contributor

More of an RFC, this reduces the memory consumption by JS hugely in my project where I have hi res (LIDAR data) tiled height maps . ( > 150MB ) which should allow it run on more limited hardware with sufficient GPU memory.

@jbaicoianu jbaicoianu commented on an outdated diff Aug 15, 2016
src/renderers/webgl/WebGLObjects.js
}
function updateBuffer( attributeProperties, data, bufferType ) {
+ if ( data.discard ) {
+
+ console.warm( "THREE.WebGLObjects: attempting to update discarded attribute array" );
@jbaicoianu
jbaicoianu Aug 15, 2016 Contributor

Guessing you probably mean console.warn() here

@aardgoose
Contributor

fixed thanks.

@mrdoob
Owner
mrdoob commented Aug 15, 2016

Could you elaborate a little bit more on what this PR does?

@aardgoose
Contributor

Certainly,

When I am using a geometry with attributes that don't change after being set up, the TypedArray objects in the associated BufferGeometry, can be discarded after the array contents have been sent to the GPU. Calling BufferGeometry.setDiscardBuffers() before the first render marks all the attributes as discardable. The large TypedArrays are replaced with length 1 arrays, to allow the main renderer to use the BYTES_PER_ELEMENT and object type without further changes to the code.

The result is greatly reduced browser memory consumption, which was causing me browser crashes especially on mobile devices.

@bhouston
Contributor
bhouston commented Sep 2, 2016

Neat. :)

@tentone
Contributor
tentone commented Sep 3, 2016

Is this PR going to be merged? It would be nice :)

@mrdoob
Owner
mrdoob commented Sep 7, 2016

How about adding a .onUpload() function to BufferAttribute and then you can delete the typedarrays on the app side?

@jbaicoianu
Contributor

I like that idea @mrdoob. If this also came with a way to optionally force a buffer to upload instantly rather than waiting until the object is rendered, the two features together would add a lot of flexibility.

@aardgoose
Contributor

That looks a good way of doing this. I was toying with making the discard operation attribute name specific so I could discard just the position attributes for large geometries who's vertices never change, but not other attributes eg colour which do. This would allow that case quite elegantly.

Passing the attribute name to the callback would be required. I think the callback should be able to be passed in from a Geometry object, rather than just the BufferGeometry, it would fit my use case anyway.

A discard() method on the BufferAttribute objects would avoid the app callback requiring knowledge of the BufferAttribute / Renderer internals.

I always thought my first imp was a rather blunt instrument approach.

How does this sound? i'll give it a go.

@aardgoose
Contributor

OK, onUpload callback mechanism added, with discard() method for BufferAttributes. If this looks OK, I can document. I have not added anything to Geometry yet.

@mrdoob mrdoob and 1 other commented on an outdated diff Sep 7, 2016
src/core/BufferAttribute.js
@@ -37,7 +39,7 @@ BufferAttribute.prototype = {
get count() {
- return this.array.length / this.itemSize;
+ return ( this.discardedLength > 0 ? this.discardedLength : this.array.length ) / this.itemSize;
@mrdoob
mrdoob Sep 7, 2016 Owner

Hmm, this feels like a hack...

@aardgoose
aardgoose Sep 8, 2016 Contributor

true, actually the original calculation is pointless unless I am missing something subtle, the Typed Array is fixed length, so we know the item count at the time of construction. Calculate once there as itemCount and use for that value in the getter. Preserving the getter to prevent accidental changing I assume?

The same applies to InterleavedBufferAttributes count as far as I can see. Not having used them myself.

@mrdoob
mrdoob Sep 9, 2016 Owner

true, actually the original calculation is pointless unless I am missing something subtle

Yep, I think you're right.

@mrdoob
Owner
mrdoob commented Sep 7, 2016

I don't understand why the onUpdate() needs to go in BufferGeometry instead of in BufferAttribute.

@aardgoose
Contributor

I put that method of assigning the callback function to each attribute in a geometry for convenience really.
I think most user's* would want to discard all buffers for an unchanging geometry, so this makes that case simple. rather than have to remember all the attribute names for a geometry, especially when using loaded / predefined geometries. There is nothing stopping someone assigning a callback to a single BufferAttribute.

  • based on a sample of n = 1.
@mrdoob
Owner
mrdoob commented Sep 9, 2016

I would prefer to keep this focused and only do the BufferAttribute.

One could always do...

for ( var name in geometry.attributes ) {
    geometry.attributes[ name ].dispose();
}
@aardgoose
Contributor
aardgoose commented Sep 10, 2016 edited

OK i'll skip the Geometry function.

Your example should really be, but essentially yes.

for ( var name in geometry.attributes ) {
    geometry.attributes[ name ].onUploadCallback = function () { this.discard );
}

I'll respin on top of the BufferAttribute count simplification.

@aardgoose
Contributor

This now applies on your current dev branch cleanly

@mrdoob

Getting there!

src/renderers/webgl/WebGLObjects.js
@@ -87,6 +87,12 @@ function WebGLObjects( gl, properties, info ) {
attributeProperties.version = data.version;
+ if ( data.onUploadCallback ) {
+
+ data.onUploadCallback.call( data, name );
@mrdoob
mrdoob Sep 15, 2016 Owner

Why not just data.onUploadCallback()?

src/core/BufferAttribute.js
+
+ var oldArray = this.array;
+
+ this.array = new oldArray.constructor( 1 ); // create dummy minimal length TypedArray
@mrdoob
mrdoob Sep 15, 2016 Owner

Hmm... This feels hacky...

@aardgoose
aardgoose Sep 16, 2016 Contributor

I'm trying to avoid changes to the WebGLRenderer, which uses instanceof buffer.array and buffer.array.BYTES_PER_ELEMENT in setupVertexAttributes(). bytesPerElement could be made a property of the attribute, but I'm not sure what the best way to handle the instanceof case, apart from an intermediate type value which seems equally clumsy.

Your suggestions?

@mrdoob
mrdoob Sep 27, 2016 Owner

discard() may confuse users. One could think that discard() is going to remove the data from the GPU (as that's how the other discard()s work).

How about moving this code to your app instead? So you would do...

attribute.onUpload( function () {

    this.array = new ( this.array.constructor )( 1 );

} );
@mrdoob
mrdoob Sep 27, 2016 Owner

Or maybe we can call it discardArray()...?

@aardgoose
aardgoose Sep 27, 2016 Contributor

A rename fine, but the only discards I can see are in shaders, are you thinking of dispose()?
I'll rename anyway.

@mrdoob
mrdoob Sep 27, 2016 Owner

Ah yes... dispose(). Should it be disposeArray()?

@aardgoose
aardgoose Oct 3, 2016 Contributor

I've looked at the issue of keeping the minimal typed array again, and tried moving the

"if ( array instanceof * ) then type = gl.{TYPE) "
...
sequence from the webGLRenderer to webGLObjects' internal createBuffer() function. In createBuffer the appropriate gl.(type) and bytesPerElement values are added to the existing attribute properties object. This way the "if" statements only run once, and the webGLRenderer now has no need to access an attribute's .array property.

The disposeArray() can now be just replaced by .array = null in the callback, or used in disposeArray() if you prefer to hide the mechanism,

How does this sound?

@@ -87,6 +87,12 @@ function WebGLObjects( gl, properties, info ) {
attributeProperties.version = data.version;
+ if ( data.onUploadCallback ) {
@mrdoob
mrdoob Nov 5, 2016 Owner

if ( data.onUploadCallback !== null ) {

@mrdoob
mrdoob Nov 5, 2016 Owner

Actually, I'll do some changes.

@mrdoob mrdoob merged commit 629195b into mrdoob:dev Nov 5, 2016
@mrdoob
Owner
mrdoob commented Nov 5, 2016

Thanks!

@mrdoob
Owner
mrdoob commented Nov 5, 2016 edited

Ok, so I renamed onUploadCallback to onUpload. 91802d6

I have also removed the name thing. I know it is handy, but I would prefer to keep this in the application layer for now. The simpler we keep the WebGL* stuff the better.

@aardgoose aardgoose deleted the aardgoose:discard branch Nov 6, 2016
@mrdoob mrdoob added a commit that referenced this pull request Nov 9, 2016
@sam-g-steel @mrdoob sam-g-steel + Abelnation RectAreaLight with ltc approximation (#10041)
* sketch of spherical distributions with linearly transformed cosine

* adding prelim example sketches for cosine dists

* makeSkew method added

* add polygon light sandbox example to repo for reference

* formatting

* notes files

* adding skeleton files for area point light

* debug directional light integrated into area light example page

* initial AreaLightHelper functional in example

* makeShape functions for Polygon and add as choices to example

* annotating TODO's with name. partial work on AreaLight shader components

* adding TODOs and placeholders for all places where AreaLight code needs to be added

* fix typo

* RectAreaLight shading works in preliminary fashion

* updates to example

* Preliminary RectAreaLight implementation (no shadows, no distance/decay)

* Integrate RectAreaLight with MeshStandardMaterial

* moving rectarealight brdf data to an example file.  rest of implementation left in place

* TubeBufferGeometry: Removed invisible char (#9943)

* Remove reference to THREE in IcosahedronGeometry.js (#9945)

Fix broken references to THREE namespace in geometries.

* Updated builds.

* Updated package.json.

* Resolved some issues from the first merge

* Added demo from #9234
Noticed that the code from @abelnation is not his latest...
more merging to come!!!

* Fixed issues from merge

* Fixed issues from merge...
webgl_lights_arealight improved
RectAreaLightHelper update bugs fixed

* Removing built js files that cause conflicts

* Use FileLoader.setMimeType() from MMDLoader (#9990)

* MMDPhysics improvement (#9989)

* MMDPhysics improvement

* Add property defined check

* Shoe physic bodies in mmd example by default.

* Updated builds.

* Improved documentation for constants / Materials (#9993)

* Improved documentation for Materials / Material (#9994)

* Added defaults to docs / perspectiveCamera (#10007)

* added constanst / animation (#10005)

* Fixed error in <head> for Docs / AnimationAction, AnimationClip, and AnimationMixer (#10004)

* Added default values for zoom, near and far properties of docs / orthographic camera (#10006)

* Improved documentation for Constants / Textures (#10001)

* Improved documentation for Materials / Material

* Moved Texture Combine Operations to constanst / materials

* updated Basic, Lambert and Phong .combine property to point to Material constant page

* Improved documentation for constants / textures

* Added Encoding constants to Textures constants page

* Ccdik solver optimization (#10010)

* Optimize CCDIKSolver

* Remove lines I should have not commit

* Remove lines I should have not committed

* added missing toJSON method (#10020)

* Added missing toJson method (#10019)

* added missing toJSON method (#10018)

* created documentation for VideoTexture (#10016)

* Created doc for CanvasTexture (#10015)

* Add CCDIKHelper (#9996)

* Add CCDIKHelper

* Clean up MMDPhysics.js

* Fix typo

* Update OBJLoader.html (#10009)

Spelling correction.

* Fix typo in comment (#10021)

* Improved documentation for docs / Texture (#10012)

* Improved documentation for docs / Texture

* Removed duplicate needsUpate

* Improved docs for Clock (#10008)

* Created new document page Constants / Renderer (#10002)

* Created constants / renderer

* renamed Renderer.html to WebGLRenderer.html

* Improved documentation for CompressedTexture (#10014)

* AudioContext: Added getContext() and setContext().

* Updated builds.

* Simplified AudioContext.

* Updated builds.

* BufferAttribute.onUpload() clean up.

* Renamed docs /constants / WebGLRenderer to Renderer (#10028)

* fixes #10026 (#10027)

* capture bufferAttribute.array properties at first upload (#9972)

* save typed array info in attribute properties

* use saved attribute properties

* remove unused variable

* Discard attribute typed arrays for buffered geometries that are not modified after initial rendering (#9512)

* add setDiscardBuffer method to BufferGeometry

* added discard support to BufferAttribute

* add mechanism for discard of BufferAttribute TypedArrays

* use more elegant method for creating dummy typed array.

* fix typo

* Update BufferGeometry.js

fix brain fade

* rework to use callbacks (phase 1)

* rework part 2

* remove build file

* support setting onUploadCallback from Geometry

* remove repeated calculation from renderer

* remove now redundant getter

* remove geoemtry interface

* document discard mechanism.

* merge fixes

* restore return.this

* drop unneeded call()

* rename discard() method to disposeArray()

* Improved documentation for WebGLRenderer (#10030)

* added missing methods

* Finished add methods

* redid changed to WebGLRenderer.html

* removed unused texture.sourceFile property (#10024)

* glTFLoader: Removed hack. See #10024.

* Updated builds.

* add link to project wiki in README.md (#9987)

* Added deprecated msg/fixed link (#10025)

* Created documentation for DepthTexture (#10017)

* Created documentation for DepthTexture

* pulled upstream

* added missing comma to docs/list.js

* Deprecated UniformsUtils. See #8016.

* Updated builds.

* MeshBasicMaterial: Add support for lightMap (#9975)

* Updated builds.
346f38c
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment