Material / Material3 Linear and Circular Progress Indicators Recompose instead of Redraw on Progress updates [295616656]

Fixed

Bug

Status Update

No update yet.

Description

jb...@google.com

created issue #1

Aug 12, 2023 02:51AM

Similar issues were discovered in Lottie several months ago, but also prevalent in `CircularProgressIndicator` and `LinearProgressIndicator` APIs. Indeterminate progress doesn't have the same issue.

When there are animated updates to progress for UIs like "Instagram Stories" or "Google Photos" which are frequent this becomes a problem.

Ideally, these APIs should have a parameter for `progressProvider: () -> Float` instead of just `progress: Float`.

This actively impacts some card types on Play's Apps and Games Home pages.

Comments

jb...@google.com <jb...@google.com> #2Aug 12, 2023 02:52AM

Project: platform/frameworks/support
Branch: androidx-main

commit a330c0d3bcdd41326f37968a60e6084ad4a2e32c
Author: Chet Haase <chet@google.com>
Date: Wed Jul 05 07:26:46 2023

Convert APIs using PointF to use Float instead

PointF is a convenient mechanism for passing around x.y values
representing 2D points. But there are downsides, including:
- Converting to PointF: You may not have the data in PointF form
to begin with, so using an API which takes PointF requires converting
the data to that form (including allocating a PointF object every time)
- Mutability: Point structures can be mutated internally, causing
unpredictability in what that mutation means. Should the library
react to those changes? Ignore them? Do defensive copies (requiring
even more allocations)? Using primitive types like Float make the
behavior more obvious (by making the data inherently immutable).
- Allocations: Whenever we use object types, there are necessarily
allocations on the Java heap for them. This puts pressure on the GC
at both allocation and collection time. Given the amount of points
being passed around (especially at morph creation time, when curves
are being split and created), this causes a lot of PointF objects to
be allocated (even temporarily). Using Float avoids that problem.

Also fixed bug with unclosed paths causing discontinuity at the
start/end point.

Bug: 276466399
Bug: 290254314
Test: integration and unit tests pass
Relnote: PointF parameters changed to Float pairs
Change-Id: Id4705d27c7be31b26ade8186b99fffe2e2f8450e

M graphics/graphics-shapes/api/current.txt
M graphics/graphics-shapes/api/restricted_current.txt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicShapeTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/CubicTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonMeasureTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/PolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/RoundedPolygonTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/ShapesTest.kt
M graphics/graphics-shapes/src/androidTest/java/androidx/graphics/shapes/TestUtils.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Cubic.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/CubicShape.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FeatureMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/FloatMapping.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Morph.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/PolygonMeasure.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/RoundedPolygon.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Shapes.kt
M graphics/graphics-shapes/src/main/java/androidx/graphics/shapes/Utils.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/DebugDraw.kt
M graphics/integration-tests/testapp-compose/src/main/java/androidx/graphics/shapes/testcompose/ShapeEditor.kt
M graphics/integration-tests/testapp/src/main/java/androidx/graphics/shapes/test/MaterialShapes.kt

https://android-review.googlesource.com/2649119

jb...@google.com <jb...@google.com> #3Aug 21, 2023 04:29PM

PointF was the main (possibly only) mutability issue, marking this as fixed

co...@google.com <co...@google.com> Aug 21, 2023 06:01PM

Assigned to ma...@google.com.

ys...@google.com <ys...@google.com> #4Aug 22, 2023 08:59AM

The following release(s) address this bug.It is possible this bug has only been partially addressed:

androidx.graphics:graphics-shapes:1.0.0-alpha04

ae...@google.com <ae...@google.com> #5Aug 30, 2023 12:19AM

Indeed we could change the progressSemantics API to be lambda-based like ScrollAxisRange, but I'm reluctant to add to our API for this. A number of properties are likely getting updated at high frequency by some application or other. Each one will add API clutter and add to the number of decisions developers have to make to optimize performance.

The semantics block is already a lambda, so we have been talking about being able to make it run separately from composition when one of its inputs changes, much like layout. Ralston is already starting to work on a performance-oriented rearchitecture to semantics and we can discuss how this might fit into that plan.

le...@google.com <le...@google.com> #6Aug 30, 2023 03:44AM

The semantics block is already executed outside of composition. That change was made and landed in 1.5.

We honestly don't have to create a new API for this. Modifier.progressSemantics is just a convenience API around the already public progressBarRangeInfo API. Material3 could easily just use a plain Modifier.semantics call with the value resolved inside the lambda and this semantics property set and it would avoid composition and fix this issue.

jb...@google.com <jb...@google.com> #7Aug 30, 2023 03:51AM

+1 to Leland's approach. That is aligned with my thinking here.

ys...@google.com <ys...@google.com> #8Aug 31, 2023 03:47PM

I'm annoyed I missed this option, really nice and clean.

ma...@google.com <ma...@google.com> Sep 26, 2023 10:47PM

Accepted by ma...@google.com.

ap...@google.com <ap...@google.com> #9Sep 28, 2023 07:05PM

Marked as fixed.

Project: platform/frameworks/support
Branch: androidx-main

commit 9b4bfdee9439d9bb9145e61137427221e46511d9
Author: Max Alfonso-Ying <maxying@google.com>
Date: Tue Sep 26 21:34:49 2023

Improve progress indicator perf

- Use lambda for progress to defer all work to draw phase
instead of recomposing
- Pre-allocate IncreaseSemanticsBounds modifier instead
of recreating it.

Fixes:

b/295616656

Test: ProgressIndicatorTest and ProgressIndicatorBenchmark.
Local device shows improvements of 80% for `changeProgress`.
Relnote: "Added new overloads of `LinearProgressIndicator` and
`CircularProgressIndicator` that take `progress` as a lambda.
These should be more performant than the previous versions."
Change-Id: I824e6ba4d57e713ad47f97f25a41c330b3439eb0

M compose/material3/benchmark/src/androidTest/java/androidx/compose/material3/benchmark/ProgressIndicatorBenchmark.kt
M compose/material3/material3/api/current.txt
M compose/material3/material3/api/restricted_current.txt
M compose/material3/material3/samples/src/main/java/androidx/compose/material3/samples/ProgressIndicatorSamples.kt
M compose/material3/material3/src/androidInstrumentedTest/kotlin/androidx/compose/material3/ProgressIndicatorScreenshotTest.kt
M compose/material3/material3/src/androidInstrumentedTest/kotlin/androidx/compose/material3/ProgressIndicatorTest.kt
M compose/material3/material3/src/commonMain/kotlin/androidx/compose/material3/ProgressIndicator.kt

https://android-review.googlesource.com/2764233

jb...@google.com <jb...@google.com> #10Sep 28, 2023 08:01PM

Thank you Max!

pr...@google.com <pr...@google.com> #11Oct 18, 2023 08:41PM