Issue 7307117: Add animated line chart sample

Issue 7307117: Add animated line chart sample (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
11 years, 7 months ago by bsalomon

Modified:
11 years, 7 months ago

Reviewers:
robertphillips

CC:
skia-review_googlegroups.com

Base URL:
http://skia.googlecode.com/svn/trunk/

Visibility:
Public.

More Reviews

Description

Add animated line chart sample R=robertphillips@google.com Committed: https://code.google.com/p/skia/source/detail?r=7727

Patch Set 4 : #

Total comments: 24

Created: 11 years, 7 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+193 lines, -0 lines)			Patch
	M	gyp/SampleApp.gyp	View		1 chunk	+1 line, -0 lines	0 comments	Download
	A	samplecode/SampleChart.cpp	View	1 2 3	1 chunk	+192 lines, -0 lines	24 comments	Download

Messages

Total messages: 3

Expand All Messages | Collapse All Messages

robertphillips

LGTM + questions/suggestions https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp File samplecode/SampleChart.cpp (right): https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#newcode19 samplecode/SampleChart.cpp:19: static SkMWCRandom gRandom; rm linePath & ...

11 years, 7 months ago (2013-02-13 21:05:17 UTC) #2

bsalomon

11 years, 7 months ago (2013-02-13 21:22:53 UTC) #3

Message was sent while issue was closed.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp
File samplecode/SampleChart.cpp (right):

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:19: static SkMWCRandom gRandom;
On 2013/02/13 21:05:17, robertphillips wrote:
> rm linePath & fillPath?

Done.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:29: // plot. The fill path is bounded by below by the
prevData plot points or a horizontal line at
On 2013/02/13 21:05:17, robertphillips wrote:
> prevData -> bottomData?

Done.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:31: // The plots are animated by rotating the data
points by leftShift.
On 2013/02/13 21:05:17, robertphillips wrote:
> <SkScalar>_&_ topData?

Nice catch.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:32: void gen_paths(const SkTDArray<SkScalar> topData,
On 2013/02/13 21:05:17, robertphillips wrote:
> spacing?

Done.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:40: plot->incReserve(topData.count());
On 2013/02/13 21:05:17, robertphillips wrote:
> shouldn't this be 2*topData.count usually and +2 when bottomData is NULL?

good point, done

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:41: fill->incReserve(topData.count() + 2);
On 2013/02/13 21:05:17, robertphillips wrote:
> Why not fill->isEmpty too?
I'll remove the assert... it was there to test my understanding of isEmpty(). I
don't think it's necessary.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:43: 
On 2013/02/13 21:05:17, robertphillips wrote:
> %=?

Done.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:49: for (int i = 0; i < shiftToEndCount; ++i) {
On 2013/02/13 21:05:17, robertphillips wrote:
> Why not just do the moveTos first outside of the loop?

Done.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:90: 
On 2013/02/13 21:05:17, robertphillips wrote:
> comment like // mimics vector test? stresses out gpu path drawing?

Done.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:106: 
On 2013/02/13 21:05:17, robertphillips wrote:
> override?

Done.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:118: if (sizeChanged) {
On 2013/02/13 21:05:17, robertphillips wrote:
> already did the getDeviceSize step above

Done.

https://codereview.appspot.com/7307117/diff/6001/samplecode/SampleChart.cpp#n...
samplecode/SampleChart.cpp:163: 
On 2013/02/13 21:05:17, robertphillips wrote:
> // Make the fills partially transparent

Done.

Expand All Messages | Collapse All Messages