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

De-dupe gradient stops #2081

Merged
merged 2 commits into from May 27, 2022
Merged

De-dupe gradient stops #2081

merged 2 commits into from May 27, 2022

Conversation

gpeal
Copy link
Collaborator

@gpeal gpeal commented May 26, 2022

Refer to code comments for more info:
Before:
CleanShot 2022-05-26 at 11 25 30
After:
CleanShot 2022-05-26 at 11 26 54

Comment on lines 276 to 278
float[] merged = new float[mergedNotTruncated.length - numDuplicates];
System.arraycopy(mergedNotTruncated, 0, merged, 0, merged.length);
return merged;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

Suggested change
float[] merged = new float[mergedNotTruncated.length - numDuplicates];
System.arraycopy(mergedNotTruncated, 0, merged, 0, merged.length);
return merged;
return Arrays.copyOf(mergedNotTruncated, mergedNotTruncated.lengh - numDuplicates);

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done!

}

if (Float.isNaN(b) || a < b) {
mergedNotTruncated[mergedIndex++] = a;
Copy link
Contributor

Choose a reason for hiding this comment

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

isn't mergeIndex can be replaced by i?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yup!

Comment on lines 243 to 248
final float a;
if (aIndex < arrayA.length) {
a = arrayA[aIndex];
} else {
a = Float.NaN;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit could be elvis

Suggested change
final float a;
if (aIndex < arrayA.length) {
a = arrayA[aIndex];
} else {
a = Float.NaN;
}
final float a = aIndex < arrayA.length ? arrayA[aIndex] : Float.NaN

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

@gpeal gpeal merged commit 3a429d8 into master May 27, 2022
@gpeal gpeal deleted the gpeal/opacity-stops-old-api branch May 27, 2022 02:45
@imbryk
Copy link

imbryk commented May 25, 2023

@gpeal @denis-bezrukov
This PR is crashing the app for quite a few complex animations we use.
I'm not able to identify the place in our animations, as they are really big, but I've found that the problem is that this PR changes the length of color array in GradientColor which results in https://github.com/airbnb/lottie-android/blob/master/lottie/src/main/java/com/airbnb/lottie/animation/keyframe/GradientColorKeyframeAnimation.java being created with keyframes which have GradientColors of different lengths, which eventually crashes at the GradientColor.lerp method with java.lang.ArrayIndexOutOfBoundsException

This can theoretically be simply fixed not to crash, but I believe that would completely break the intended gradient animation.

If the problem solved by this PR was needed mostly for static lottie files, maybe you could apply this change only when parsing a static value:
https://github.com/airbnb/lottie-android/blob/master/lottie/src/main/java/com/airbnb/lottie/parser/KeyframeParser.java#L369
but when parsing animation keep the old way of parsing it (i.e. keeping all values)

@gpeal
Copy link
Collaborator Author

gpeal commented May 25, 2023

@imbryk Could you attach an animation that is crashing? I'd rather fix the problem than fall back to something wrong.

@imbryk
Copy link

imbryk commented May 25, 2023

@gpeal would it be possible to send it to you privately?

@Blincwear88
Copy link

#2297

@gpeal
Copy link
Collaborator Author

gpeal commented May 25, 2023

@imbryk Yeah, go ahead and DM it to me on Twitter but open a new issue saying you did that so it doesn't slip through the cracks the next time I work on this.

@imbryk
Copy link

imbryk commented May 26, 2023

@gpeal I am not able to send you a message on twitter 🤷 - please grab the files from gdrive:
https://drive.google.com/drive/folders/1LOiSaIJq1LJL848Ne8rA9Omrn2ErynfY

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.

None yet

4 participants