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

feat: custom easing #88

Merged
merged 13 commits into from
Mar 22, 2024
Merged

feat: custom easing #88

merged 13 commits into from
Mar 22, 2024

Conversation

tarsisexistence
Copy link
Contributor

@tarsisexistence tarsisexistence commented Feb 4, 2024

Solves my request: #84 (comment)
Issue: #87

The first commit is an actual feat.

The second commit is an example to play on it. It would be deleted for sure.

@tarsisexistence
Copy link
Contributor Author

I made a few controversial changes I'm open to discuss:

  1. Ability to change the transitioning during the current transition: https://github.com/dkaoster/scrolly-video/pull/88/files#diff-04040b6e6fe5a265ce37465ef946eca4f67c7b272764952a82d7840b6018ee0aL401

  2. Replaced recursion of transitionToTargetTime in favor of inner recursion to keep track of progress. It wasn't possible to track the progress with targetTime, currentTime, and saved currentTime as a startTime because of the nature of easing. https://github.com/dkaoster/scrolly-video/pull/88/files#diff-04040b6e6fe5a265ce37465ef946eca4f67c7b272764952a82d7840b6018ee0aR319

  3. demos folder that I do not intend to merge. it would be easier to follow code review with a working example. Though I could do that in a codesandbox, but I hope it's fine for now.

@tarsisexistence
Copy link
Contributor Author

hey @dkaoster , just for my expectations, i'm curious when would you have time to review this PR?
Am asking because I have other stacked ideas to improve 🫠

@tarsisexistence tarsisexistence marked this pull request as draft February 11, 2024 09:01
@dkaoster
Copy link
Owner

@tarsinzer Sorry! Will take a look in the next couple of days!

@dkaoster
Copy link
Owner

@tarsinzer Had some time to look at this today.

I think what's going to be a little unfortunate is that this solution will only be compatible with the webcodecs approach to rendering, meaning that it will currently only be supported in chrome. Given that many people using this library are already confused by the implementations in different browsers, I'm hesitant to introduce another feature that only works in Chromium based browsers.

Additionally, I tried a few different easing functions and it seems most of them are not very well suited for this implementation, maybe there is a better way of doing this..

Thoughts?

@tarsisexistence
Copy link
Contributor Author

@dkaoster Regarding browser support, I can't say much, honestly...

And regarding easing functions, can you say what exactly you used? I used several ones, and all worked as expected. Do you think that the problem is on implementation side? 🤔

@dkaoster
Copy link
Owner

For example, if I try using this code on your branch,

<div className="scrolly-container" style={{ height: '300vh' }}>
  <ScrollyVideo
    ref={scrollyRef}
    src="https://scrollyvideo.js.org/goldengate.mp4"
    easing={d3.easeQuadInOut}
  />
</div>

it kind of rolls forward a few frames and then jumps?

Screen.Recording.2024-02-17.at.10.11.21.PM.mov

@dkaoster
Copy link
Owner

Regarding browser compatibility, my instinct is to wait on webcodecs support in all the major browsers: Chrome, Safari, and Firefox, and then we'll be able to simplify the implementation of this library a lot, and it would make more sense to implement this feature then. I'm open to hearing other thoughts if you feel like this feature should be included sooner, though!

@tarsisexistence
Copy link
Contributor Author

tarsisexistence commented Feb 18, 2024

@dkaoster, the issue on your preview is not related to the easing. I won't dive more deeply here, but it's something at the beginning when the video is uploading/decoding. It's visually noticeable (different colours and brightness of the frame). If you click too fast on the beginning, your animation will be broken.

Here's a preview of d3.easeQuadInOut. Looks like working fine 🤔
https://github.com/dkaoster/scrolly-video/assets/21989873/cf551c58-c4f7-4d5e-b463-d5b5a842bb8c

@dkaoster
Copy link
Owner

dkaoster commented Feb 18, 2024

No, what's broken seems to be the trackscroll feature. See this video of clicking on the seeker bar vs scrolling. If I set it to easeLinear or simply removing the easing it works fine.

Screen.Recording.2024-02-18.at.9.21.32.AM.mov

@tarsisexistence
Copy link
Contributor Author

tarsisexistence commented Feb 18, 2024

@dkaoster do you apply easing for the ScrollyVideo instance or via set percentage function? In my testing setup, I use a function instead of an instance. Actually, it's quite a bad idea to use easing for trackscroll feature.

scrollyRef.current.setVideoPercentage(
  parseFloat(e.target.value),
  {
    easing: d3.easeLinear,
  }
)

@tarsisexistence
Copy link
Contributor Author

Today I was thinking about browser support.

There are two potential ways:

  1. We ship easing without waiting for web codecs. Why? It doesn't block anyone. If you don't use easing - your implementation is the same as it was before.
  2. We adapt easing for the other part, which probably looks feasible.

@dkaoster
Copy link
Owner

@tarsinzer okay, I see what you're saying.

I agree with you, it's not a good idea to have easing on the track scroll feature, so I'm going to suggest that we remove the ability to add easing to the scrollyVideo instance completely and have it only available as an argument to the setVideoPercentage function.

Regarding browser support, I wonder if we should release this under a name called experimentalEasing or something so that people can use it while knowing that it will not work in many cases. I think I'd be okay with something like that.

@tarsisexistence
Copy link
Contributor Author

@dkaoster sounds like a plan! I'll do it soon. Thanks for collaborating on that idea!

@tarsisexistence
Copy link
Contributor Author

tarsisexistence commented Mar 2, 2024

@dkaoster wdyt about the last commit? Couldn't understand how to test it properly, because it just literally jumps on backwards in Firefox.

@dkaoster
Copy link
Owner

@tarsinzer this looks good to me! I think you can go ahead and remove your demo and we should be good to merge.

@dkaoster
Copy link
Owner

@tarsinzer one more thing, do you mind updating this line in the readme with the new arguments?

@tarsisexistence tarsisexistence marked this pull request as ready for review March 17, 2024 14:16
@tarsisexistence
Copy link
Contributor Author

@dkaoster absolutely!

I just updated it. Please validate.
The only doubt here is that setCurrentTimePercent is only available for React API, as far as I understand.

@dkaoster
Copy link
Owner

@tarsinzer I see, I can help you add it into svelte and vue after this.

@dkaoster dkaoster merged commit 5750cda into dkaoster:main Mar 22, 2024
@tarsisexistence tarsisexistence deleted the easing branch March 24, 2024 15:08
@tarsisexistence tarsisexistence changed the title Feat: custom easing feat: custom easing Mar 31, 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.

2 participants