phetsims/neuron

ParticlesWebGLNode refactoring

Closed this issue · 3 comments

In the process of refactoring this for phetsims/scenery#308, I ran across a few things:

  1. updateParticleData() doesn't seem to need the 'self' variable, as there are no function closures.
  2. particleData could include Vector2s (seems faster, and can mutate with set() and matrix.multiplyVector2())
  3. Why continually cast elementData to a Uint16Array? Just make elementData a Uint16Array?
  4. gl.STATIC_DRAW is unsuitable for things that are changed every frame. gl.DYNAMIC_DRAW seems better?
  5. On that note, it seems elementData is being overwritten with the same values every frame, and would be suitable for STATIC_DRAW if we don't change it? (Just fill it in for the maximum number of particles supported?)
  6. Currently (and before refactoring), a ton of the vertex data is stored on the Node itself. Seems awkward (but correct), as each painter updates (all) the data on the Node before painting.
  7. I removed the context-loss listener when the painter is disposed (in case it happens on canvases used for screeenshots)

@jbphet, at your convenience, could you review my changes (e05cef1), and review the above comments?

I implemented several of the suggestions listed above, here are responses to individual issues that were enumerated:

  1. The self variable was removed and its usages were replaced with this.
  2. I went ahead and replaced xPos and yPos with a vector, though there wasn't and perceptible improvement in performance.
  3. elementData is now declared as a Uint16Array.
  4. I changed the usage of STATIC_DRAW to DYNAMIC_DRAW for the vertex data. I looked up these constants, and the definitions found on line do make it seem that DYNAMIC_DRAW is the value to use for data that changes, but I'm a little ignorant when it comes to WebGL details like this. Making the change didn't seem to have any obvious adverse effects on the sim, please double check and make sure that it looks correct.
  5. I moved initialization of element data to the constructor and left the related call alone, so it still uses STATIC_DRAW.
  6. I'm not sure if there's a suggestion here about where the vertex data should be stored. You said it was, "awkward (but correct)" and I'm not sure if there is something that should be done to make it less "awkward".
  7. Sounds good to me.

Assigning back to @jonathanolson to review my changes and the notes above.

I'm not sure if there's a suggestion here about where the vertex data should be stored. You said it was, "awkward (but correct)" and I'm not sure if there is something that should be done to make it less "awkward".

I would generally initially implement things such that the Painter / init/paint calls would NOT modify things on the Node itself, as it feels like a break in encapsulation (i.e. like two views of something updating view information that is stored on the model element). Each view (Painter) fully overwrites data that it changes, so it doesn't break anything.

That said, it's probably not worth refactoring (up to you). Everything else looks good, thanks!

I think I understand the concern, but I also agree that "it's probably not worth refactoring" since I don't expect this sim to get a lot of maintenance or reuse. I'll keep this in mind and will consult with @jonathanolson to work towards better encapsulation the next time I do one of these, which will likely be for the States of Matter simulation. Closing.