_bindBuffer unnecessarily copies typed arrays

[rbtcollins]

Most appropriate sub-area of p5.js?

• Accessibility
• Color
• Core/Environment/Rendering
• Data
• DOM
• Events
• Image
• IO
• Math
• Typography
• Utilities
• WebGL
• Build Process
• Unit Testing
• Localization Tools
• Friendly Errors
• Other (specify if possible)

p5.js version

main

Web browser and version

No response

Operating System

No response

Steps to reproduce this

I documented the behaviour in this PR that got closed because of some process you prefer. #5737

I don't have a lot of spare time, and while I'll happily do value-adding things to allow this to be merged, I have no plans to become a long term contributor, so someone with full awareness of the hoops you've got present might want to take this over.

[welcome]

Welcome! 👋 Thanks for opening your first issue here! And to ensure the community is able to respond to your issue, please make sure to fill out the inputs in the issue forms. Thank you!

[davepagurek]

Thanks for opening this! I can look through the code at some point in the near future and see if there are cases where we could be storing values in a typed array to improve performance (possibly in p5.Geometry?)

In the mean time, it looks like your change will be necessary to accommodate this. I'll give it a review in a little bit after having taken a closer look at what our current buffers look like to get a fuller picture.

[rbtcollins]

Yeah, so a bit more background and context. Firstly the patch probably wants to be tested with more than just one type of typed array, but I wasn't sure what ones are relevant to p5, and the helper that exists in the file is quite broken and slow (5 or so checks, and doesn't | them together. I think what I wrote should work for all typed arrays.

Secondly, what I was doing was animating medium numbers of identical in orientation and texture boxes - about 20K of them - which p5 really didn't like. Doing that with instancing would be great, but as p5 doesn't do that implicitly or explicitly I needed to poke around a bit under the hood.

So what I did to make it all hang together is create a geometry object, populate it with all the vertices, faces, lines for stokes, and then draw that once to populate internal p5 caches, then for subsequent frames save and restore the lineVertices and vertices buffer map attribute, thereby avoiding the friction of vertex (vertices) and nested array (lineVertices) translation : instead I did a one-time conversion to a typed array, and then edited in-place. You could probably keep the same vertex orientated API use JS setters and getters, but eliminating shape-conversions when your time budget per frame is 16ms and 8ms can be spent just doing 50 rects.

So I would then iterate over the high level objects I'm working on, which were backed by this geometry, and mutate the coordinates, mark the two buffers involved dirty, and voila, done. At least until I needed a different number of boxes. Heh.

Along the way I noticed a few things.

The 'slow path' in flatten is very slow and not needed - the reason its needed is claimed to be stack exhaustion, but I think in actual use the nesting depth is only and always 1, and the children are homogeneous, so going directly to a fixed size typed array will be computationally cheaper, and combined with the _bindBuffer fix should be a nice improvement on its own.

_flatten -> _bindBuffer, both called by _prepareBuffer, double handle the same data: if conversion is required, better to do it just once.

as mentioned above, the internal representation for geometries is not GPU friendly, which means that p5 pays a performance tax on every frame. Even without instanced rendering, this can be avoided if the underlying representation is tuned. e.g. the Vector in a geometry vertices list should be 3 floats in a Float32Array, and the Vector object store the base index of the first one and a reference to the array, permitting getters and setters to work. Or something - I don't plan to design or implement, but it seems very possible to keep the friendly education API but still perform well.

[davepagurek]

Sorry it took so long to get back to this!

Right now, if you make a p5.Geometry once and never update it, performance should currently be good, as it won't update buffer data. So this would be an improvement just for when you need to update data. Do we think this is something p5 should support? We do have some internals set up to allow it (there is a dirty flag that one can set to mark that buffers need to be updated, but I'm pretty sure this isn't publicly documented yet) but it's not user-facing.

That said, it looks like we'd be able to allow it and make it more efficient with minimal changes and without breaking sketches, especially because updating geometry isn't documented. If we restrict updates to geometry data so that they must keep the same topology as before (same number of vertices, same faces), we could convert the buffer data to a typed array after the geometry is initially created and allow people to update those efficiently. As a follow-up, we could optionally provide a p5.Vector-like API as @rbtcollins mentioned where each vector is backed by data in the typed array, but even without that, we'd be unblocking people from e.g. writing a gltf animated mesh importer library, as that would need to update geometry each frame.

@stalgiag what do you think? This is probably also related to #5393 and figuring out level of complexity p5 wants to support, since this is definitely more of an advanced feature.

[stalgiag]

When thinking about accessibility with the GL Renderer, I think about people who are coding 3D graphics for the first time and I also think of people who are contributing to a 3D graphics library for the first time. As you said @davepagurek, this PR doesn't break anything but it does add code that wouldn't make sense to a contributor unless it was understood that we support modification of p5.Geometry objects. If we want to do that, then I think we should commit fully and consider the design of the interface and document it well.

It appears to me that contributors are interested in this functionality and there is some organic momentum to support it. This pathway offers the potential for the WEBGL renderer to become a more uniquely situated tool with a low friction approach for those wanting to work with custom geometry + shaders. The benefit in accessibility with this lies more with inspiring examples that may come as output.

As long as those first two groups (new 3D coders and new 3D contributors) are considered, I will back any approach. I do think that it is better to support something with transparency and clarity than to leave it as a common workaround that the source code doesn't expect. But most importantly, @davepagurek I think you are the best situated to determine the aims for this phase of the WEBGL renderer so I'll trust your call.

[davepagurek]

So the goal for this would be to:

• ideally not break existing sketches using geometry
• store geometry data in a format that can be updated more efficiently
• add a method for libraries and users to update that data

A little while ago, @xrcyz helped collect a number of examples in the wild of p5.Geometry, which I've been looking at to research existing usage patterns:

• sayo
  • https://openprocessing.org/sketch/1028245
  • https://openprocessing.org/sketch/944829
• sayama
  • https://openprocessing.org/sketch/861764
  • https://openprocessing.org/sketch/872176
• mathFoxLab
  • https://openprocessing.org/sketch/1136878
  • https://openprocessing.org/sketch/1145281
• Kusakari
  • https://openprocessing.org/sketch/1431881
• Paul Wheeler
  • https://openprocessing.org/sketch/1353108
• Blechdavier
  • https://openprocessing.org/sketch/1219380
• Horikawa
  • https://openprocessing.org/sketch/8891971
• xrcyz
  • https://openprocessing.org/sketch/1342385
• me
  • https://openprocessing.org/sketch/1616318
  • https://openprocessing.org/sketch/1590159

Here are the patterns I see for different stages of the p5.Geometry lifecycle:

• Creating:
  • new p5.Geometry(w, h, cb) where cb sets up the verts/faces
  • g = new p5.Geometry(w, h) and then updating g.vertices etc on following lines
• Drawing:
  • model(g)
  • this._renderer.createBuffers(gId, g) + this._renderer.drawBuffers(gId) or drawBuffersScaled
• Freeing/updating (no one seems to update data in-place, but rather recreate the geometry):
  • this._renderer._freeBuffers(gid)
  • Manually calling renderer.createBuffers(gid, g) again

Initially I only had the first form of creating p5.Geometry in mind. If we can assume the initial data is set up entirely by the callback, we can support more efficient updates by libraries using something like this:

class p5.Geometry {
  constructor(w, h, cb) {
    // constructs vertices, edges, faces, etc
    cb();
    
    // store data in a more efficient format
    this.vertexBuffer = Float32Array.from(p5.RendererGL.prototype._flatten(this.vertexBuffer));
    // repeat for other buffers
  }

  // You can then update the values in `this.vertexBuffer` and call this method to update the geoemtry in a reasonably
  // efficient manner. This would be how e.g. a library adding support for gltf animation imports would update each frame
  updateVertices() {
    this.dirtyFlags.vertexBuffer = true;
  }
}

• Pros:
  • Allows us, in the future, to make more user-friendly ways to update data, e.g. by creating vector-like wrappers for the data in the typed array so that it can be edited in place without overhead
• Cons:
  • Would break some existing sketches

If we don't want to deprecate users' code that omits passing a callback and modifies data directly, we could opt instead for a lazy approach to making the typed arrays only on render. It likely would mean that in addition to the dirty flag from before (used by libraries and advanced users who edit the more efficient data structure), we would need load/update methods for the user-friendly array of p5.Vectors, similar to load/updatePixels:

class p5.Geometry {
  get vertexBufferData() {
    if (!this._vertexBufferData) {
      this._vertexBufferData = Float32Array.from(p5.RendererGL.prototype._flatten(this.vertexBuffer);
    }
    return this._vertexBufferData;
  }

  // Libraries and advanced users can call this after updating `this.vertexBufferData` directly
  updateVertexBufferData() {
    this.dirtyFlags.vertexBuffer = true;
  }

  // Users call this before editing `this.vertexBuffer`
  loadVertices() {
    if (this._vertexBufferData) {
      this.vertexBuffer = [];
      for (let i = 0; i < this._vertexBufferData.length; i += 3) {
        this.vertexBuffer.push(
          createVector(
            this._vertexBufferData[i],
            this._vertexBufferData[i + 1],
            this._vertexBufferData[i + 2]
          )
        );
      }
    }
  }

  // Users call this after editing `this.vertexBuffer`
  updateVertices() {
    // Delete the typed array; it will get lazily re-created when asked for
    delete this._vertexBufferData;
    // Mark the buffer as needing to be updated on the GPU
    this.updateVertexBufferData();
  }
}

• Pros:
  • Works with all existing code but allows for libraries to make fast updates
  • Would immediately let users update geometry using a load/update metaphor which should already be familiar
• Cons:
  • More moving parts, more bug surface area
  • There isn't a good way to optimize updates made by editing an array of vectors directly without fully supporting every JS array operation under the hood

I think if we're OK with the tradeoffs, my preference is to go with the former approach, as it leaves room for us to make friendly methods that are also efficient (e.g. we could support a syntax like this.vertex(i).add(someOtherVector) which edits data in-place, so it's efficient while still being pretty friendly.) I don't think we need those friendly methods initially, as I haven't found examples of in-place editing. I'd prefer that we don't commit to editing this.vertexBuffer directly as the way to update geometry just because that's how some users are creating their geometry, as committing to that makes it much harder to optimize performance. That said, breaking sketches is a big deal, and that might be more important, so I'm willing to accept doing things differently if we feel that's more important!

Also, both methods allows us, in the future, to make helper functions that make the cb in the constructor for you using an easier API (e.g. something similar to immediate mode drawing APIs), so I've left that out of this discussion.

[davepagurek]

Some updates on this:

• We now have p5.DataArray (currently used for automaticaly-generated line data), which stores information in a Float32Array without needing to flatten and convert every frame, but allowing the array to be variably sized
• With Add methods to construct p5.Geometry from other p5 drawing functions #6287, we now have methods of creating geometry via vertex(), which we could adapt to use p5.DataArray under the hood if we wanted

However, there are a few more cases where we currently still have programmers accessing these arrays directly, which we would need to replace if we want full freedom of a more optimized under-the-hood representation:

• in some cases it's still useful to create geometry by specifying vertices/faces/edges (e.g. as described in this tutorial) so we would have to come up with a new interface for this. Maybe beginShape(MESH) + a new API for face(v1, v2, v3) and edge(v1, v2)?
• while not a currently exposed feature, updating geometry is easier with direct array access. Maybe we could add a updateVertex(index, x, y, z) method that can edit the underlying array? + maybe similar methods for vertex colors + normals? These methods could also mark buffers as dirty so that they update GPU data the next time you use them.