Network: Cleanup and refactoring PhysicsEngine #3507
Conversation
lib/network/modules/PhysicsEngine.js
Outdated
| this.body.emitter.on("_dataChanged", () => { | ||
| // update shortcut lists | ||
| // Nodes an/or edges have been added or removed, update shortcut lists. |
There was a problem hiding this comment.
OK. Can you hold off merging until @mbroad has reviewed, please?
There was a problem hiding this comment.
Of course, that's exactly why I waited
lib/network/modules/PhysicsEngine.js
Outdated
| this.updatePhysicsData(); | ||
| }); | ||
|
|
||
| // debug: show forces | ||
| // this.body.emitter.on("afterDrawing", (ctx) => {this._drawForces(ctx);}); | ||
| //this.body.emitter.on("afterDrawing", (ctx) => {this._drawForces(ctx);}); |
There was a problem hiding this comment.
Why remove space after comment indicator
There was a problem hiding this comment.
No reason whatsoever. My fingers were probably thinking for me. I actually prefer the space.
Ah yes, I enabled that to test the call, then I added the //'s back again.
| * @param {Node} node | ||
| * @private | ||
| */ | ||
| _getForceContributions(parentBranch, node) { |
There was a problem hiding this comment.
A function beginning with _get that has no return value...
Now I know this isn't your fault, as you're being consistent with _getForceContribution, but this seems wrong.
There was a problem hiding this comment.
OK. Well, there is a force contribution to be gotten, only it's not returned but added to param parentBranch. I can't think of any better names right now.
|
Yeah, tough one to unit test on, because everything should remain as is. I consider the examples in the unit tests running to be a good indication of fitness - actually all tests that run physics. I did a visual check by running OK, sooooo....think about a possible unit test here. The premise is The drawback I see to that idea is that it's most likely a single-shot test. If anything changes in physics, you can throw the test away. |
|
I just meant testing the inputs, outputs, and side effects of individual functions in isolation. |
|
Actually, I think a full test is easier to do at this point. This would be an indication that the whole physics thing works as before. |
|
It's actually quite instructive to enable the debug routine for
Apparently, the optimization here is to subdivide the area and deal with the forces in each area separately. I can sort of intuit that this works. |
* Initial refactoring * Small refactoring BarnesHutSolver * Disabled debug routine, empty lines removed * Put back space in comment * Typo
Some work done in preparation for #3468, while examining the code for better understanding. The logic remains unchanged.
Note that this does not solve #3468, it's just a preliminary cleanup.