Skip to content
This repository has been archived by the owner on Jul 29, 2019. It is now read-only.

IE performance improvements #3425 #3489

Merged
merged 5 commits into from
Sep 29, 2017

Conversation

justinharrell
Copy link
Contributor

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.

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 26, 2017

❤️ 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');
Copy link
Contributor

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....

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 26, 2017

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.

@wimrijnders wimrijnders self-requested a review September 26, 2017 17:45
@justinharrell
Copy link
Contributor Author

Is that any better? The 0700f19 can be ignored if thats the issue, it was a merge artifact fixed up in 50db76f

@wimrijnders
Copy link
Contributor

wimrijnders commented Sep 26, 2017

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.

Copy link
Contributor

@wimrijnders wimrijnders left a 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!

Copy link

@mbroad mbroad left a 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.

@wimrijnders
Copy link
Contributor

@mbroad That's a fair question. I'll scan the code to see if it makes sense to convert to _requestRedraw() elsewhere.

@yotamberk yotamberk merged commit 3cac614 into almende:develop Sep 29, 2017
@justinharrell justinharrell deleted the label-performance branch September 29, 2017 16:24
primozs pushed a commit to primozs/vis that referenced this pull request Jan 3, 2019
* Label performance improvements

* Label cleanup

* Fix drag performance in IE

* Fixed broken images still trying to draw

* Fixed merge and lint issues
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants