Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Canvas heatmap #557

Merged
merged 15 commits into from
Apr 18, 2016
Merged

Canvas heatmap #557

merged 15 commits into from
Apr 18, 2016

Conversation

aashish24
Copy link
Member

No description provided.

@aashish24 aashish24 changed the title Canvas heatmap [WIP] Canvas heatmap Apr 13, 2016
@aashish24
Copy link
Member Author

I am still working on it. This is my todo list:

  • Squash commits
  • Look at performance issues
  • Check on documentation

@aashish24 aashish24 force-pushed the canvas_heatmap branch 2 times, most recently from ac1daf0 to 262e089 Compare April 13, 2016 16:53
@jbeezley
Copy link
Contributor

Out of curiosity, is there a reason we wouldn't just use a standalone library for this? For example, heatcanvas has a similar implementation conceptually, but it also supports web workers and dynamically updating the heatmap.

@aashish24
Copy link
Member Author

Out of curiosity, is there a reason we wouldn't just use a standalone library for this? For example, heatcanvas has a similar implementation conceptually,

We looked into various heatmap implementations out there (I have to check the link you have poster though) and found various sort of issues (in-correct rendering, api mis-match) etc (I don't remember all the details but @dcjohnston might have it documented somewhere). The code is small enough that @dcjohnston and I didn't feel need to use the third party library.

@aashish24 aashish24 force-pushed the canvas_heatmap branch 3 times, most recently from bd9c8bf to 7a0b67a Compare April 14, 2016 03:12
@aashish24 aashish24 force-pushed the canvas_heatmap branch 3 times, most recently from 9faac80 to eb370db Compare April 14, 2016 03:19
@aashish24 aashish24 force-pushed the canvas_heatmap branch 2 times, most recently from 7feb601 to ea526fa Compare April 14, 2016 04:11
@aashish24 aashish24 changed the title [WIP] Canvas heatmap Canvas heatmap Apr 14, 2016
@aashish24 aashish24 force-pushed the canvas_heatmap branch 2 times, most recently from 02383e3 to 678ff11 Compare April 14, 2016 04:16
@aashish24
Copy link
Member Author

Its ready for review / suggestions.

@aashish24 aashish24 force-pushed the canvas_heatmap branch 2 times, most recently from 5520e56 to 8023c46 Compare April 14, 2016 05:10
@jbeezley
Copy link
Contributor

I added a commit to improve the performance. Even with this change though, I'm not sure the performance is good enough for this to be useful. Even with as few as 100 points the render call is exceeding the animation frame budget. At 9,000 points I'm getting about 12 fps.

I think for something like this to be useful in practice we need to debounce the render calls and apply css transforms to respond to navigation events. This is what is done in the standard leaflet example which handles 10,000 points without breaking a sweat.

@aashish24
Copy link
Member Author

I added a commit to improve the performance. Even with this change though, I'm not sure the performance is good enough for this to be useful. Even with as few as 100 points the render call is exceeding the animation frame budget. At 9,000 points I'm getting about 12 fps.

@jbeezley yes performance certainly has be to be improved. The test data that @dcjohnston has (I don't have the number of points on top of my head) it is large and the performance is poor. The leaflet example you posted, @dcjohnston has looked into it.

debounce the render calls and apply css transforms to respond to navigation events

Applying css transformations would certainly be useful (but I believe will only work in panning though since for zoom events you would have to aggregate). Not sure about debouncing the render calls. Can you explain some more?

@aashish24
Copy link
Member Author

On this note I have only looked into two of the three issues i posted above:

  • Squash commits
  • Look at performance issues
  • Check on documentation

Since I don't have an exact date when @dcjohnston will pick it up on performance.

@aashish24
Copy link
Member Author

The plugin is using simpleheat under the hood (https://github.com/mourner/simpleheat) which we have looked at as well (and have found some issues with it). On the performance note, we should be able to optimize the performance.

@jbeezley
Copy link
Contributor

The example in this PR has 9,555 points.

You can apply scale transform to simulate zooming. Yes, it is not the same thing as rerendering, but it makes for an acceptable visual substitute. The behavior that leaflet exhibits is that all animated navigations (zoom and pan) are simulated by CSS transforms. Once the animation is finished the debounced call to rerender the canvas is made.

We can certainly improve the performance some, but as you can see from the screenshot below, the time spent is dominated by native canvas functions drawImage and getImageData, which we won't be able to optimize. My takeaway is that this kind of rendering cannot be done inside the animation loop.
screen shot 2016-04-14 at 9 27 14 am

@aashish24
Copy link
Member Author

We can certainly improve the performance some, but as you can see from the screenshot below, the time spent is dominated by native canvas functions drawImage and getImageData, which we won't be able to optimize.

Yes, I agree.

Our ultimate goal is to use webgl for this since the performance there would be so much better (and I would like to target that sometime soon, we already have looked into it some). The canvas implementation was the first step in that direction (base class API etc, understanding the canvas performance).

* intensity must be a positive real number will be used to normalize all
* intensities with a dataset. If no value is given, then a it will
* be computed.
* @returns {geo.heatmapFetures}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

geo.heatmapFeature

@aashish24
Copy link
Member Author

Ref: #557 #559 #74

@aashish24
Copy link
Member Author

@jbeezley fixed issues you have found. Also, reported the performance issue #559 here.

@jbeezley
Copy link
Contributor

LGTM

@aashish24 aashish24 merged commit 1b56c2d into master Apr 18, 2016
@aashish24 aashish24 deleted the canvas_heatmap branch April 18, 2016 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants