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

Add time remap specs #56

Merged
merged 7 commits into from
Jul 16, 2024
Merged

Add time remap specs #56

merged 7 commits into from
Jul 16, 2024

Conversation

fmalita
Copy link
Member

@fmalita fmalita commented Jul 2, 2024

No description provided.

@fmalita
Copy link
Member Author

fmalita commented Jul 2, 2024

I think we might have talked about punting this to 1.1, but one of our internal 1.0 clients is relying on the feature.

Since it's pretty straightforward to spec, and it's also supported with most players, it doesn't seem like stretch to add now - WDYT?

@mbasaglia
Copy link
Member

mbasaglia commented Jul 2, 2024

I thought there were inconsistencies between implementations. Can we confirm it works properly on most players?

@@ -71,3 +71,76 @@ decrease it ("stretching" the layer timeline).
</script>
</lottie-playground>

The `tm` property specifies a time remap function, which offers full control over the precomp
Copy link
Member

Choose a reason for hiding this comment

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

I'd create a new subsection for time remapping

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. Also added a subsection for time stretch.

@fmalita
Copy link
Member Author

fmalita commented Jul 3, 2024

I thought there were inconsistencies between implementations. Can we confirm it works properly on most players?

Do you recall what the inconsistency was?

I tested LottieWeb, LottieAndroid, and Skottie with this sample - they all seem to have good support.

It also works fine in the LottieFiles app, although I'm not sure whether that's using your new player of LottieWeb still. Maybe you can double-check.

time expressed in seconds, and evaluates all animatable precomp properties based on the new
time value:

$$t\prime = TM(t) * FPS$$
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it allowed for the time remap to only cover a portion of the animation? If so, what's the behavior? I would guess frames outside of the remap would play as normal. It may be worth explicitly calling this out.

Copy link
Member Author

Choose a reason for hiding this comment

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

The input/domain range is fixed ($[Layer.ip .. Layer.op]$), but the output/codomain range is arbitrary. Expanded the description.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I got it, so first keyframe MUST equal ip and last MUST equal op ?

I suspect this is more of a general keyframe requirement? So may make sense to document it there (as a separate PR).

Copy link
Member

Choose a reason for hiding this comment

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

Usually there's no timing requirements for keyframes (other than being strictly increasing), I don't recall tm being special in that case. Values before the first keyframe or after the last just have the same value as that keyframe.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah - Brandon, think of tm as just a regular animatable property (which it basically is). There are no special rules around its input range, it can be evaluated for the whole layer timeline just like all other animatable properties.

schema/layers/precomposition-layer.json Outdated Show resolved Hide resolved
@mbasaglia
Copy link
Member

Do you recall what the inconsistency was?

I think iOS didn't handle the timing correctly and had horrible performance when using time remapping

@fmalita
Copy link
Member Author

fmalita commented Jul 3, 2024

Do you recall what the inconsistency was?

I think iOS didn't handle the timing correctly and had horrible performance when using time remapping

Tested against the latest iOS tree (emulator), it appears to work now - maybe it was fixed?

@mbasaglia
Copy link
Member

maybe it was fixed?

Could be, maybe we can ask them to review this?

@fmalita
Copy link
Member Author

fmalita commented Jul 3, 2024

Could be, maybe we can ask them to review this?

I asked Cal to take a look at this, and he confirmed that time remapping is supported on iOS.

Copy link
Member

@Aidosmf Aidosmf left a comment

Choose a reason for hiding this comment

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

LGTM, just have a small question

}
],
"markers": [],
"props": {}
Copy link
Member

Choose a reason for hiding this comment

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

is it necessary?

Suggested change
"props": {}

Copy link
Member Author

Choose a reason for hiding this comment

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

Most likely not needed. Updated.

@Aidosmf Aidosmf added this to the 1.0 milestone Jul 16, 2024
@fmalita fmalita merged commit e37fb3e into lottie:main Jul 16, 2024
3 checks passed
b-wils pushed a commit to b-wils/lottie-spec that referenced this pull request Jul 19, 2024
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.

4 participants