Feature/particles view update #375

eneim · 2017-02-21T15:18:42Z

IMPORTANT:

~~I commented out some lines in SplashActivity to make the sample, please read it and un-comment for normal behaviour.~~
I'm using AS RC-1 so please manually change gradle plugin back to 2.2.3, or I will do it after the review.

Issue

Current ParticlesAnimationView is beautifully implemented. But it comes with some trade off as follow:
- Redundant Object allocation inside onDraw method: createLines(particles) uses Stream which will allocate new Iterators each calls. Furthermore it unnecessarily generates new Lines.
- "Random" should be used once only to ensure the randomness of number. See its doc for more detail.
- Current implementation of createLines(particles) will draw each line twice, which is not so good.
- Calling drawLines to draw a single line is not so good too.
- Private classes can be Private static class, to prevent leak (not now, just in case).

Overview: what I did

Make private class (Particle, Line) static.
Re-implement Line class, fix some step in Particle class that would allocate new objects in onDraw().
Cache necessary objects as ParticlesAnimationView's member, and just update them if need (see Random instance, List instance for example).
- This fix reduce the number of Line instance from few thousands (数万 lines in some cases) to 780 (= C(40, 2)) (see my comment).
Improve onDraw and so on. Stream is no longer used here ...
isShouldLinked is actually shouldBeLinked I think :D.

Discussion: so it worth the change?

Current View is shown in few seconds, so there may be no need to do redundant work.
But ... some may want to use it as Cover background, or Title view etc, so improving it here can help.
See screenshot for more information (see number of GC calls, magenta (or blue?) in GPU log ...) (sorry don't know how to align 2 screenshot horizontally, please help).

Screenshot

Before	After

…her tune up.

konifar · 2017-02-21T15:30:13Z

app/src/main/java/io/github/droidkaigi/confsched2017/view/activity/SplashActivity.java

+        Takt.stock(getApplication())
+                .seat(Seat.BOTTOM_RIGHT)
+                .interval(250)
+                .listener(fps -> Log.w("DroidKaigi", "heartbeat() called with: fps = [" + fps + "]"))


[Android Lint] _{reported by reviewdog 🐶}
Error: Using 'Log' instead of 'Timber' [LogNotTimber]

…actually un-comment the normal way. some type and grammar fix

konifar · 2017-02-21T23:39:09Z

Thank you for contribution! Interesting!

eneim · 2017-02-22T00:04:13Z

Thanks. Let me fix the license and conflict in few minutes :D

eneim · 2017-02-22T00:37:08Z

FYI @konifar I would like to ask @roana0229 (author of this View) to review this if possible. My change may conflict with the expected behaviour in some unexpected ways.

konifar · 2017-02-23T04:49:57Z

app/build.gradle

@@ -226,6 +226,9 @@ dependencies {
    debugCompile 'com.tomoima.debot:debot:2.0.2'
    releaseCompile 'com.tomoima.debot:debot-no-op:2.0.2'

+    // fps, track the frame rate of animation view.
+    compile 'jp.wasabeef:takt:1.0.3'


konifar · 2017-02-23T04:51:05Z

app/src/main/java/io/github/droidkaigi/confsched2017/view/activity/SplashActivity.java

@@ -52,12 +54,24 @@ protected void onCreate(Bundle savedInstanceState) {
    protected void onStart() {
        super.onStart();
        loadSessionsForCache();
+
+        // Starting new Activity normally will not destroy this Activity, so set this up in start/stop cycle
+        // TODO remove it if you hate it ...


I like it. So let's remove this TODO comment 😉

Just push a commit to remove the TODO :D.

konifar · 2017-02-23T04:53:48Z

...src/main/java/io/github/droidkaigi/confsched2017/view/customview/ParticlesAnimationView.java

-
-    private class Line {
+    /**
+     * A Pair of 'Particles' whose centers can be 'linked to each other ... Called 'Line'.


Awesome! 👍

konifar · 2017-02-23T05:00:30Z

Looks good. Thanks 😃
@roana0229 Can you review this?

…eim/conference-app-2017 into feature/particles-view-update

eneim · 2017-02-23T08:18:53Z

@konifar my latest commit cause a failed build on circleci, checking it, turns out :app:testDevelopDebugUnitTest failed with one case. But running it on my machine passes all tests. Can you manually trigger a rebuild? If it keeps failing again I will try to figure out what happened.

konifar · 2017-02-23T08:32:16Z

Sorry, currently CI is not stable. I'll work on it tonight.

eneim · 2017-02-23T08:44:14Z

Thanks, just take your time :D

roana0229

Thank you for improving performance! Awesome 👍
I learned from this PR. 😄

roana0229 · 2017-02-23T12:01:06Z

...src/main/java/io/github/droidkaigi/confsched2017/view/customview/ParticlesAnimationView.java

-
-    private class Line {
+    /**
+     * A Pair of 'Particles' whose centers can be 'linked to each other ... Called 'Line'.


Awesome! 👍

roana0229 · 2017-02-23T15:22:59Z

...src/main/java/io/github/droidkaigi/confsched2017/view/customview/ParticlesAnimationView.java

-                            + (point[3] - point[1]) * (point[3] - point[1])
-            ));
+        void draw(Canvas canvas, Paint paint) {
+            if (!first.shouldBeLinked(LINK_HEXAGON_DISTANCE, second)) {


In my opinion, shouldBeLinked is not necessary if first particle compare second particle in draw.

@roana0229 not really get your full point, but do you mean to call shouldBeLinked from ParticlesAnimationView#onDraw instead of here? It will end up removing the need of using "Line" object (a Line is actually just a bridge between 2 particles :D).

Yesterday I thought about something like:

@Override protected void onDraw(Canvas canvas) { super.onDraw(canvas); for (int i = 0, size = particles.size(); i < size; i++) { particles.get(i).draw(canvas, paint); } // for (int i = 0, size = lines.size(); i < size; i++) { // lines.get(i).draw(canvas, paint); //} for (int i = 0; i < particles.size() - 1; i++) { for (int j = i + 1; j < particles.size(); j++) { if (particles.get(i).shouldBeLinked(LINK_HEXAGON_DISTANCE, particles.get(j))) { // TODO draw, use the code from Line } } } }

I think what you mean is the following:

// ParticlesAnimationView @Override protected void onDraw(Canvas canvas) { super.onDraw(canvas); for (int i = 0, size = particles.size(); i < size; i++) { particles.get(i).draw(canvas, paint); } for (int i = 0, size = lines.size(); i < size; i++) { if (lines.get(i).shouldDraw()) { lines.get(i).draw(canvas, paint); } } } // ParticlesAnimationView$Line ///// shouldDraw simply returns the comparison boolean shouldDraw() { return first.shouldBeLinked(LINK_HEXAGON_DISTANCE, second); }

Neither of above gain any huge impact (but the later looks clean and better, in fact :D). For now I would like to keep current version (this PR is pretty huge now) and maybe another PR from you if you really want to :p. So it is up to you ^^.

I thought that the following code was good.

For now I would like to keep current version (this PR is pretty huge now) and maybe another PR from you

OK! 😄

roana0229 · 2017-02-24T01:33:21Z

LGTM 👍

konifar · 2017-02-25T06:03:21Z

Thanks for improvement!

eneim added 2 commits February 21, 2017 23:44

Optimize Particle View in the matter of object allocation and some ot…

dc18009

…her tune up.

few comments

f4de230

KeithYokoma added the in progress label Feb 21, 2017

konifar reviewed Feb 21, 2017

View reviewed changes

also clear lines on windows focus change. use Timber instead of Log. …

bf2d196

…actually un-comment the normal way. some type and grammar fix

eneim added 2 commits February 22, 2017 09:11

add license info for takt

c8b05da

Merge branch 'master' into feature/particles-view-update

0676fdd

konifar reviewed Feb 23, 2017

View reviewed changes

eneim added 2 commits February 23, 2017 15:02

remove unused TODOs

703c7ab

Merge branch 'feature/particles-view-update' of https://github.com/en…

70cf5fe

…eim/conference-app-2017 into feature/particles-view-update

roana0229 approved these changes Feb 23, 2017

View reviewed changes

konifar merged commit 2f107da into DroidKaigi:master Feb 25, 2017

konifar removed the in progress label Feb 25, 2017

eneim deleted the feature/particles-view-update branch February 26, 2017 04:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/particles view update #375

Feature/particles view update #375

eneim commented Feb 21, 2017 •

edited

konifar Feb 21, 2017

konifar commented Feb 21, 2017

eneim commented Feb 22, 2017

eneim commented Feb 22, 2017 •

edited

konifar Feb 23, 2017

konifar Feb 23, 2017 •

edited

eneim Feb 23, 2017

konifar Feb 23, 2017

roana0229 Feb 23, 2017

konifar commented Feb 23, 2017

eneim commented Feb 23, 2017

konifar commented Feb 23, 2017

eneim commented Feb 23, 2017

roana0229 left a comment

roana0229 Feb 23, 2017

roana0229 Feb 23, 2017

eneim Feb 24, 2017 •

edited

roana0229 Feb 24, 2017

roana0229 commented Feb 24, 2017

konifar commented Feb 25, 2017

Feature/particles view update #375

Feature/particles view update #375

Conversation

eneim commented Feb 21, 2017 • edited

IMPORTANT:

Issue

Overview: what I did

Discussion: so it worth the change?

Screenshot

Choose a reason for hiding this comment

konifar commented Feb 21, 2017

eneim commented Feb 22, 2017

eneim commented Feb 22, 2017 • edited

Choose a reason for hiding this comment

konifar Feb 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

konifar commented Feb 23, 2017

eneim commented Feb 23, 2017

konifar commented Feb 23, 2017

eneim commented Feb 23, 2017

roana0229 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eneim Feb 24, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roana0229 commented Feb 24, 2017

konifar commented Feb 25, 2017

eneim commented Feb 21, 2017 •

edited

eneim commented Feb 22, 2017 •

edited

konifar Feb 23, 2017 •

edited

eneim Feb 24, 2017 •

edited