search

joewalnes / smoothie

Smoothie Charts: smooooooth JavaScript charts for realtime streaming data
http://smoothiecharts.org
Other
2.25k stars 232 forks source link

Some small improvements [patch] #40

Closed asbai closed 10 years ago

asbai commented 10 years ago

I had add some small improvements to this great library:

  1. [For IE9] remove the redandunt 'globally' lastTime variable, so we can create multiple charts without interference.
  2. improve the render method: only render in needed.

here is it: http://baiy.cn/tmp/smoothie.zip

please search "by BaiYang" to review the patch. :)

drewnoakes commented 10 years ago

Hi Bai,

I just took a quick look at this and it looks good. Your changes address two of the three TODO comments, which is great. I'll review this in more detail shortly and integrate your changes after some testing.

Drew.

drewnoakes commented 10 years ago

I found a small issue with the limit on the framerate when scrolling slowly. If the chart is dynamically resizing to fit new data points, then not enough frames are drawn to make the animation smooth.

You can test this in the builder: drag the scroll speed slider to the far right, then toggle fix maximum value.

asbai commented 10 years ago

Did you mean the scaling of the y-value not smoothly enough?

Cloud we detect this situation and boost the frame rate temporarily?

PS: It would be best if we could avoid using any O(N)+ algorithm to detect this. O(1) algorithm is the preferred of course. :-)

drewnoakes commented 10 years ago

@asbai yes that's right. The y-movement was quite jerky. I've committed some code that does exactly as you suggest: if the animation is occurring then it won't reduce the frame rate. Please take a look and see what you think.

Your fix to the requestAnimationFrame patch will follow in another commit once I've reviewed it.

asbai commented 10 years ago

@drewnoakes I have been read your code, and it seems very nice.

drewnoakes commented 10 years ago

I've merged your changes into master. Thanks again!

I thought some more about the issue with running multiple animation loops concurrently due to having more than one chart in the document. It seems to me that having a single animation loop that then renders all charts on the page at once might be worth trying. It may reduce the amount of composition done in the browser. Of course we'd need to test. As it stands, behaviour looks pretty good across all the browsers I've tested on.