Closed tarsisexistence closed 7 months ago
I made a few controversial changes I'm open to discuss:
Ability to change the transitioning during the current transition: https://github.com/dkaoster/scrolly-video/pull/88/files#diff-04040b6e6fe5a265ce37465ef946eca4f67c7b272764952a82d7840b6018ee0aL401
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
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.
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 ðŸ«
@tarsinzer Sorry! Will take a look in the next couple of days!
@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?
@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? 🤔
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?
https://github.com/dkaoster/scrolly-video/assets/3477162/4e1600d9-1d39-4253-a0c7-5262d3e45e9b
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!
@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
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.
https://github.com/dkaoster/scrolly-video/assets/3477162/d66fa907-e944-4953-9b79-272587f3d1d4
@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,
}
)
Today I was thinking about browser support.
There are two potential ways:
@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.
@dkaoster sounds like a plan! I'll do it soon. Thanks for collaborating on that idea!
@dkaoster wdyt about the last commit? Couldn't understand how to test it properly, because it just literally jumps on backwards in Firefox.
@tarsinzer this looks good to me! I think you can go ahead and remove your demo and we should be good to merge.
@tarsinzer one more thing, do you mind updating this line in the readme with the new arguments?
@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.
@tarsinzer I see, I can help you add it into svelte and vue after this.
Solves my request: https://github.com/dkaoster/scrolly-video/pull/84#issuecomment-1913717931 Issue: https://github.com/dkaoster/scrolly-video/issues/87
The first commit is an actual feat.
The second commit is an example to play on it. It would be deleted for sure.