Skip to content

fix: Invalid web LA keyframe processing when from/to offset is used #8035

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

Merged

Conversation

MatiPl01
Copy link
Member

@MatiPl01 MatiPl01 commented Aug 7, 2025

Summary

The previous implementation used an invalid type for the KeyframeDefinitions type assuming that all keys in KeyframeDefinitions are numbers. Because of that, it also converted all keys to numbers via the .map(Number) mapping which gave NaN offset values in the offsets array. As a result, the easing shift didn't work properly as NaN aren't valid offsets so we were getting undefined instead of the valid keyframe definition object.

Example recordings

Before

Screen.Recording.2025-08-07.at.15.35.00.1.mp4

After

Screen.Recording.2025-08-07.at.15.33.37.mp4

@MatiPl01 MatiPl01 requested a review from m-bert August 7, 2025 13:40
@MatiPl01 MatiPl01 marked this pull request as ready for review August 7, 2025 13:40
@MatiPl01 MatiPl01 self-assigned this Aug 7, 2025
@MatiPl01 MatiPl01 added this pull request to the merge queue Aug 7, 2025
Merged via the queue into main with commit e23e218 Aug 7, 2025
12 checks passed
@MatiPl01 MatiPl01 deleted the @matipl01/add-from-to-keyframe-offset-support-on-web branch August 7, 2025 13:50
MatiPl01 added a commit that referenced this pull request Aug 7, 2025
…8035)

## Summary

The previous implementation used an invalid type for the
`KeyframeDefinitions` type assuming that all keys in
`KeyframeDefinitions` are numbers. Because of that, it also converted
all keys to numbers via the `.map(Number)` mapping which gave `NaN`
offset values in the `offsets` array. As a result, the easing shift
didn't work properly as `NaN` aren't valid offsets so we were getting
`undefined` instead of the valid keyframe definition object.

## Example recordings

### Before


https://github.com/user-attachments/assets/1dd8933c-1811-41c7-ada1-4a87c29404c3

### After


https://github.com/user-attachments/assets/7b62f9ad-8d29-4e0f-99e4-89faf2cc19c6
MatiPl01 added a commit that referenced this pull request Aug 7, 2025
…8035)

## Summary

The previous implementation used an invalid type for the
`KeyframeDefinitions` type assuming that all keys in
`KeyframeDefinitions` are numbers. Because of that, it also converted
all keys to numbers via the `.map(Number)` mapping which gave `NaN`
offset values in the `offsets` array. As a result, the easing shift
didn't work properly as `NaN` aren't valid offsets so we were getting
`undefined` instead of the valid keyframe definition object.

## Example recordings

### Before


https://github.com/user-attachments/assets/1dd8933c-1811-41c7-ada1-4a87c29404c3

### After


https://github.com/user-attachments/assets/7b62f9ad-8d29-4e0f-99e4-89faf2cc19c6
MatiPl01 added a commit that referenced this pull request Aug 7, 2025
MatiPl01 added a commit that referenced this pull request Aug 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants