Closed jonathanolson closed 8 years ago
I implemented several of the suggestions listed above, here are responses to individual issues that were enumerated:
self
variable was removed and its usages were replaced with this
.xPos
and yPos
with a vector, though there wasn't and perceptible improvement in performance.elementData
is now declared as a Uint16Array
.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.STATIC_DRAW
.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.
In the process of refactoring this for https://github.com/phetsims/scenery/issues/308, I ran across a few things:
@jbphet, at your convenience, could you review my changes (https://github.com/phetsims/neuron/commit/e05cef1fc66a589780d7d9d06cb5c60e4337e4eb), and review the above comments?