-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Conversation
❤️ Thanks for looking into this! Fixes #3425 |
@@ -393,7 +393,7 @@ class InteractionHandler { | |||
let diffY = pointer.y - this.drag.pointer.y; | |||
|
|||
this.body.view.translation = {x:this.drag.translation.x + diffX, y:this.drag.translation.y + diffY}; | |||
this.body.emitter.emit('_redraw'); | |||
this.body.emitter.emit('_requestRedraw'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's it???? This resolves the IE issue??? Wow....
Esteemed comitter, your changes and fixes are totally and uniformly excellent. I am exceedingly impressed. There's only one thing that prevents me from immediately approving, and that is that the PR appears to be building on an old version of the code. Would you mind merging with latest develop and see what changes are needed? Sincerely, thank you for your efforts. Edit: I am totally aware that this is my code paranoia speaking. Please humor me. |
I found the problem: it's that I'm an idiot. I was comparing to a branch that hasn't been merged yet (#3486). That means that I consider your changes to be perfect, I can't find a fault in them. It also means that one of us will have to do merge+conflicts soon. Probably me, because I'm hereby approving this. Wrt previous comment: No, keep the PR as is. It's good, I was on a tangent. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In one word: gorgeous. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These changes look good to me.
It does make me wonder if there are other areas that would benefit from emitting _requestRedraw
rather than just _redraw
.
@mbroad That's a fair question. I'll scan the code to see if it makes sense to convert to |
* Label performance improvements * Label cleanup * Fix drag performance in IE * Fixed broken images still trying to draw * Fixed merge and lint issues
So the main issue with #3425 is onDrag was calling the synchronous _redraw instead of the aysnc _requestRedraw like the other interaction events. This didn't seem to be issue in Chrome but in IE it destroyed frame rate when dragging the view around.
The reason it didn't not cause an issue when physic simulation was still running is because the _redraw handler in CanvasRenderer does not actually call redraw if rendering is active.
The other changes to Label.js improve performance across the board based on profiling in IE and Chrome, which is where I started with the issue. I also cleaned up some code that was simply not correct and made it a little better for possible further optimizations in the future.