ragnarlotus / vue-flux

Image slider which comes with 20 cool transitions
https://ragnarlotus.github.io/vue-flux-docs/demos/demos
MIT License
542 stars 49 forks source link

Fix for custom transitions not properly loaded #60

Closed gterras closed 5 years ago

gterras commented 5 years ago

Hi,

I noticed custom transitions are not properly loaded :

https://github.com/deulos/vue-flux/blob/0096716c9cb46300938d4d78f0857842f2db366c/src/components/FluxTransition.vue#L76-L79

should probably return this.transition.component.

A few comments : I took a look at the code and really like the clean and clever architecture fully using Vue transitions power, custom transitions are super easy to make. Did you consider passing the full images array as prop to the transition component? It could open some space for fun advanced effects.

Also the first custom transitions I'm doing are "back" versions of default transitions such as slide or swipe. If it's not too much work I really support the come back of the direction feature in v6 because some effects feel really weird when the same direction is used for next and previous slides.

Last thing I can't seem to import BaseTransition from the dist folder in my custom transitions, is there a way to do so? I had to copy it. On a related note when installed with npm the src folder isn't present, only dist, is this a choice ?

ragnarlotus commented 5 years ago

Hi,

I noticed custom transitions are not properly loaded :

https://github.com/deulos/vue-flux/blob/0096716c9cb46300938d4d78f0857842f2db366c/src/components/FluxTransition.vue#L76-L79

should probably return this.transition.component.

It should not, that computed returns the name of the component because: https://github.com/deulos/vue-flux/blob/0096716c9cb46300938d4d78f0857842f2db366c/src/components/FluxTransition.vue#L7-L7

And the component is imported in: https://github.com/deulos/vue-flux/blob/0096716c9cb46300938d4d78f0857842f2db366c/src/components/FluxTransition.vue#L100-L100

So you should not have any problem... paste the console error or give me a repo I can review :smile:

A few comments : I took a look at the code and really like the clean and clever architecture fully using Vue transitions power, custom transitions are super easy to make. Did you consider passing the full images array as prop to the transition component? It could open some space for fun advanced effects.

I really didn't think about passing all images array. Do you think that people will use more than one image to make a transition?

Also the first custom transitions I'm doing are "back" versions of default transitions such as slide or swipe. If it's not too much work I really support the come back of the direction feature in v6 because some effects feel really weird when the same direction is used for next and previous slides.

You are totally right, I didn't think about it, so I will consider taking them back...

Last thing I can't seem to import BaseTransition from the dist folder in my custom transitions, is there a way to do so? I had to copy it. On a related note when installed with npm the src folder isn't present, only dist, is this a choice ?

I'm idiot :laughing: I forgot to include BaseTransition mixin in the package... thanks I will add it also. About not including the source is a choice because it is available here, so for what add extra size to the package if most people will never use it?

BTW yesterday just checked the glitch in other environments and found that it only occurs in Firefox and in my source version, the beta.18 transpiled version is fine, so I suspect that could probably come from a dependency updated recently. Yesterday I started to clean and review some code to make another beta release and see if still occurs in Firefox.

I will include array of images and direction of transitions, thanks a lot for the support!!

gterras commented 5 years ago

Hi and thanks for your response.

It should not, that computed returns the name of the component because

Maybe I'm doing something wrong but the source of this Object.assign (this.transition.component) has no main key, so all the props are added to the $options object without key, triggering an error.

Standard transitions :

Screenshot_20190904_115431

Adding a custom transition:

      transitions: [{
         name: 'TransitionTest',
         component: TransitionTest,
      }]

Screenshot_20190904_115617

Which gives a Unknown custom element: <TransitionTest> - did you register the component correctly? For recursive components, make sure to provide the "name" option.

Something like this works instead of the Object.assign() : this.$options.components[this.transition.name] = this.transition.component

I really didn't think about passing all images array. Do you think that people will use more than one image to make a transition?

I'm not sure it would be widely used but an interesting use case I see is displaying a relevant background (a third slide) for some effects like Blinds instead of a generic background. And of course crazy multi slides transitions !

You are totally right, I didn't think about it, so I will consider taking them back... I will include array of images and direction of transitions

That is double great news :+1:

thanks a lot for the support

Thanks a lot for this super cool slider :100:

ragnarlotus commented 5 years ago

You are right, the $options.components is an object and I should assign the component in its key.

I will release a new beta with the fix as soon as I can be sure that the glitch is gone, expecting to be today or tomorrow.

Thanks a lot!!

ragnarlotus commented 5 years ago

Just tried last changes and fixes and unfortunately the glitch remains even in published version.

Feel free to download sources and experiment in Firefox, specially running the wave transition. You will see the glitch right when pressing the button of 3D transition.

My concerns are following... the code didn't really changed since beta.18 and doing git checkout to that version does not fix the glitch so I started to suspect something caused by the dependencies... but the ironic was that even rolling back setting specific version of deps keeps the glitch.

So my only thoughts are the global dependencies. Beside that the problem seems to come from synchronicity so or there is a bug in one of my global deps or I need to find a way to sync the process, which seems to be the best but unwilling choice...

gterras commented 5 years ago

Are you talking about the controls jumping to the top of the slider ? Else I don't see anything. If yes I will try to look at it.

Also since your last commit can you npm install vue-flux@beta ? I get a notarget No matching version found for vue-flux@beta even though beta20 is listed on npm.

I managed to update before this last commit, it seems that the added dependency to core-js embed a node_modules in vue-flux folder, is it necessary ?

ragnarlotus commented 5 years ago

Sorry, yesterday I did some tries to check if with a packaged and released version the glitch was not happening but unfortunately remained so I had to unpublish them and forgot to restore the beta tag. No should be set to version 6.0.0-beta.18 again.

The glitch is a kind of blink or flash caused by the main image when changes to display the next and the transition component is not drawn yet. Also in the wave animation, the sides of each var are sometimes displayed before the front side causing an ugly effect, probably caused because sides are just colored which causes to render faster than the front.

I managed to solve the blink with a couple of setTimeout to ensure that the things are run in order but I don't like it as solution and I think that the best approach would be to control readiness of child components and control their rending although I am afraid that cost in performance, complexity and final weight will be a problem...

The core-js is just one of the tests I did... today I will try downgrading the global packages and I will record a video to show you the glitch, because if it is not happening to you we can do a comparison to find where does it comes.

Regards and thanks a lot for the help

gterras commented 5 years ago

The glitch is a kind of blink or flash caused by the main image when changes to display the next and the transition component is not drawn yet.

All right I can reproduce this almost consistently for many transitions on Chrome Linux as long as dev tools are opened. I thought you were talking about something else because most likely this glitch is due to the nature of Vue and the architecture of vue-flux. I've never seen it with dev tools closed, did you ?

Anyway it feels like this can be compressed to a specific point but in the end depending on the client you can never be sure it will be unseen, there will always be a split second where the content is empty before the image is displayed. Or maybe you can add a "background" component under the transition that will always be displaying the "from" image except between transition start and transition end, what do you think ?

I managed to solve the blink with a couple of setTimeout to ensure that the things are run in order but I don't like it as solution

indeed :crying_cat_face:

ragnarlotus commented 5 years ago

That's exactly the effect, but in your case is totally normal because having dev tools open disables the cache, which causes the preloaded images to be loaded again.

BUT I think you just pointed me to the problem and solution. I will try it this afternoon and let you know :grin:

ragnarlotus commented 5 years ago

Not lucky, firefox is working the same in all my home computers. But at work (previous version and in linux) the glitch does not happen.

I also tried downgrading the global packages and running with npm and yarn.

I recorded a video where you can see the glitch I mean, https://we.tl/t-VoqXbhVKlb grey bars sometimes in wave transition and blinks in slide transition also random.

All seems to be just a matter of synchronicity and render timing... I will take a few days to find what is the best approach

ragnarlotus commented 5 years ago

Did a LOT of tests and all the results where the same, sync is not properly managed by firefox, not even v5, because just using v5 is causing the glitch, which means that something changed in firefox rendering engine....

The only available solution now is to control the rendering flow, so I will work on it tomorrow and make a release ASAP.

I will keep you updated

gterras commented 5 years ago

Hi,

I might have found something about the "grey bars" in Wave :

Screenshot_20190906_103550

A way to reproduce it consistently is to wrap the content of

https://github.com/deulos/vue-flux/blob/4aefd69fd3bc7115d6fa8cf9963556ed22144cb4/src/components/FluxImage.vue#L60

in a nextTick(), this way image will never be loaded and the bars will appear for the duration of the transition (also more convenient with a longer transition speed).

This gives the time to find the culprit, which appears to be transform-style: preserve-3d in FluxCube : remove this prop and bars disappear. I think that at this point the transformation has started but since image is not yet loaded it uses the background color (sideColor) defined in Wave (#333).

From there I don't really know what's best to do but I can tell setting base transformStyle to flat in FluxCube and appying preserve-3d only when transition has started makes the glitch disappear (in my case I added css.transformStyle = 'preserve-3d' in getSideCss()).

Or maybe just delay the apply of sideColor in Wave .

Now this falls back to the other problem of the flash before transition start. I still have to see it when dev tools are not opened (and indeed this is due to cache: when dev tools are opened with settings "keep cache" the glitch does not appear). I will try to look into this this week end.

Tell me if it helps !

ragnarlotus commented 5 years ago

I was aware that the grey bars were seen because are rendered faster than the frontal image, but you just had a super idea!! :grinning:

I was sure that the solution had to be based in drawing the fronts before the sides but was not sure about the best way achieve it with best performance, but I think that giving the initial flat style and then changing it to preserve-3d when fronts are loaded will do the trick, good job :+1:

Also I kept testing and had more findings... the glitch in firefox just happens if page is realoded, not when just accessed by link or address bar :joy: it's just insane...

This afternoon I will try to make to synchronize all components in the flow and if I succeed and the flat trick works next week the release could see the light finally!!

Thanks a lot again man for the help

gterras commented 5 years ago

Ok that's good news ! About the flash problem, a few thoughts :

ragnarlotus commented 5 years ago

If I'm not mistaking in your screencast the flash only appears when the page has just loaded, confirming the cache problem. I don't see the preloader in your video, is it disabled ? That would explain why you see it and I don't.

Yes, I removed the all the complements in that test to check if without having to render them would affect or not, but it does not really change the preloading as it is managed by the Images controller, not in the FluxPreloader component. Anyway the images are loaded from M2 HD so don't even take a second to load, and the flash appears the same event without having to reload the page again, but you just don't see it because of the video frame rate...

Did you try to wrap the content of mounted in transition files into a nextTick() ? To my knowledge this is the only way to ensure children of current component are also mounted. In a typical transition like Slide the wrapper start transitioning potentially before flux-image are mounted. Having wrapper transform inside a nextTick() prevents this.

Yes, one of my thoughts is to move all the the transforms from the mounted hook to a method called play so it will be only called once the images are rendered. But before that I also need to syncrhonise the transition start from Transitions controller and the root image update, to ensure that are run right after.

ragnarlotus commented 5 years ago

nextTick inside FluxTransition.mounted() is not working... I will have to implement a ready bubble to know when everything is rendered, which still does not ensures that all have been rendered because I have no access to the browser engine.... I will try some CSS tricks to force engine rendering

ragnarlotus commented 5 years ago

Good news... I managed to control browser engine with this:

requestAnimationFrame(() => {
   requestAnimationFrame(this.run);
});

works perfect in firefox and chrome, just the wave greay bars remain...

also just did a commit if you want to test it

ragnarlotus commented 5 years ago

All wrong, still happens, much less frequently but still...

gterras commented 5 years ago

Thanks for your fixes ! I'm taking a look this afternoon on this. Can I PR a test branch if I find something ?

ragnarlotus commented 5 years ago

sure! dont even need to ask πŸ‘

if you find a solution test it in safari if you can because the glitch affects it too, at least in my ipad, don’t know in mac but probably too

tomorrow i will try some ideas if i have some time to spare. im sure im missing something. i want to check if nexttick affects all the components the same by bubbling or should be set in the respective component

regards!

gterras commented 5 years ago

Hi,

I kind of managed to do something interesting. Since I couldn't really reproduce the glitch my reference environment was Chrome with dev tools opened and cache disabled, which has the glitch most of the time if not anytime.

After reading your comment I've tried the demo on my iphone and had the glitch 100% of the time :(

With fix enabled I got it only a very few times and only when mashing next button, I suspect these case are due to system memory limit somehow reached and can be fixable somehow. Note that I had to disable allowToSkipTransition and add a check on the props I added before allowing a new transition to be created : when transitions are allowed be skipped and replaced this happens more often (maybe for previous reasons).

https://github.com/deulos/vue-flux/pull/61

See the PR (this is just a PoC) where the fix is applied on Fade and Slide. Note that Slide now can have a weird artifact when sliding (I've only seen it in Chrome but only with dev tools opened which always mess with transition anyway) but this is a different issue due to the dimensions changes I've made (see below), maybe it can be fixed with a flexbox positioning.

Anyway the main reason for the glitch to happen is that FluxImage doesn't really notify anyone when its image is loaded. There is something binded to the load event of the image but since it's a computed prop and it emits nothing the whole chain can't reliably happen in a correct order.

This is made even trickier because of the "permanent image" (let's call it like this) under the transtition has the weird behavior of displaying the to image at the same time transition starts. More on this below.

So right now the order of things is something like this :

where it should be something like this :

At this point the result is better but glitch still happens sometimes.

All of this is made really harder because of this "permanent image" change on transition start (and no Vuex :( . Unless I'm missing something (I'm pretty sure it breaks many features actually) currentIndex should ideally be changed once the transition has ended, removing the need for this bubbling up (and double render for the browser). To me the ideal flow is :

With this setup (as seen in PR) the result is way better.

The others changes I have made, though I'm not sure it changes anything per se, is removing most of the calls to size calculation (mask, images, etc). Is this really needed ? Since the slider has dimensions, why not gives background-size:contain;width:100%; height:100%; top:0; left:0; bottom:0; right:0; to its children whenever it is possible ? I know you're doing it in case no dimensions are provided but that's a lot of code and complexity.

In general I'm not sure FluxImage should use anything else than the CSS passed as prop, IE no CSS calculation inside this component, always receiving CSS from parent. It might be trickier with more complex effect Cube effects but I almsot made it work for Wave (not present in PR).

About Wave you still get the grey bars effect with the fix but this can be removed with the transformStyle: 'flat' trick.

I've also added a few CSS props to ensure 3d acceleration is always triggered/optimized like translate3d(0,0,0,0) to containers or will-change on images, and remove most of the nextTick in the chain because I found out they were most of the time between useless and harmful. Also maybe others things that I forgot :D

Anyway that's messy but that should be a start !

ragnarlotus commented 5 years ago

Really appreciated! I will review it and do some tests ASAP.

ragnarlotus commented 5 years ago

I will resume everything πŸ˜„

I suspect these case are due to system memory limit

Not really... It's about processing speed

when transitions are allowed be skipped and replaced this happens more often

This is expected but with the right lifecycle it would not happen

it can be fixed with a flexbox positioning

Yes, in fact I should review some styles because were though for IE which I'm not willing to support in this version

Anyway the main reason for the glitch to happen is that FluxImage doesn't really notify

You just got my latest test in which I tried to manage the readiness but was just a test, unfortunally like you said computed elements do not pay attention to child $refs so was fail test 😠

weird behavior of displaying the to image at the same time transition starts

That is why I moved the transition logic to played hook

So right now the order of things is something like this

Your discovering about current lifecycle is totally right, "permanent image" is updated faster than the transition is rendered

where it should be something like this

Mostly right although once the transition images are fully loaded, the "permanent image" can be updated safely and we don't really care when it is finished rendering or not

change on transition start and no Vuex

I really want to keep all of my projects as far as possible from dependencies, in fact I had to implement the mobile gestures myself, but as far as not being a big deal, deps are ok but when a bug or bug is found it becomes in a domino effect problem

currentIndex should ideally be changed once the transition has ended

Not possible because we would need another extra image for some transitions, like the fade one for example

To me the ideal flow is

That is not valid flow because the FluxTransition component should be agnostic, and not depend on "permanent image". In fact one of the features of this version is to make all components as independents as possible to use this not only as slider but as image library

With this setup (as seen in PR) the result is way better

Can't discuss it although not elegible for the reassons I will explain you 😞

removing most of the calls to size calculation

We need them from window resizing and full screen πŸ˜”

Since the slider has dimensions, why not gives background-size:contain

That would break the composed images like grid and vortex. Images are displayed as background to save tags and need to be positioned and masked for FluxGrid and FluxVortex components. So FluxImage needs to be independent but compatible with the dependent components, and doing some division and multiplication does not increase complexity so much, in fact it ensures a pixel perfect rendering. Think about SOLID principles.

In general I'm not sure FluxImage should use anything else than the CSS

I will be glad to implement a solution that works for grid and single image if it does not cause an extra code caused by a wrong lifecycle of a component that should not affect it

About Wave you still get the grey bars

Yes, I go step by step because if I solve the lifecycle, the bars will be solved too so they don't really worry me

I've also added a few CSS props

Really appreciate it, I will take some of your code to improve the current, do not doubt it! 😁

Right now, because it is in beta stage I will not take a patch as a solution if it does not really solves the root problem.

As last option I still have 2 wild cards, one is to use an extra image like in v5, and the other is to control all FluxImages in a recursive function.

Thanks again for your time and ideas. Although they were not what the project needed right now will help to improve it ☺️

ragnarlotus commented 5 years ago

I just upaloaded the recursive function wild card to dev for slide transition, feel free to test 😁

ragnarlotus commented 5 years ago

Great news... my requestAnimationFrame approach was good but missed a definitive detail. With my last commit the glitch is gone by 100% maintaining all the features. Feel free to test it in any browser or device πŸ˜„

I will add the wave bars fix and all of your improvements and make a new beta release.

I would like to add you as contributor because you helped me a lot, would you like?

gterras commented 5 years ago

Hi thanks for your feedback and work ! I see the changes are clever. However I still get the glitch 100% of the time on my iphone (SE latest firmware). I really think @load in FluxImage has to emit an event before FluxTransition is allowed to play (because despite the image being preloaded the browser still need some time to display it). Also of course you can add me, I plan to add this nice transition in a few weeks maybe you will find it interesting !

ragnarlotus commented 5 years ago

Forgive me for the question because can sound a bit stupid... but did you test it opening the last commit in a PRIVATE tab? Just say because testing my self, noticed that reload button did nothing, even in private tab, I always had to close it and open a new private for each try.

Using my last commit worked for me 100% perfect in:

Try with the private tab and let me know please.

This afternoon I expect to solve the bars problem of wave because yesterday I didn't have much time.

Regards!

ragnarlotus commented 5 years ago

I managed to solve 99% of the wave transition glitch with the flat trick and is all I can do to make it work.

I only reproduced it in a top computer and smashing the wave button, which is is enough for me. It is funny that in low computers or devices never happens.

Tomorrow I will do some code and CSS improvements, add you as contributor and release the probably latest beta.

Next will be to update and finish docs with demos and playgrounds, and then release version final stable version.

BTW you didn't tell me if the fix worked for custom transition, I guess it did.

gterras commented 5 years ago

Hi,

I've tried the latest version on iOS (cache cleared + private window) and unfortunately still have the glitch 100% of the time. Maybe it is related to the dev environment (I am running the project through npm run serve) and maybe it works better with a static build (which does not seem to be working with npm run bundle). Do you want me to record a video ?

ragnarlotus commented 5 years ago

In computer you have the glitch too?

I use yarn although npm run serve should not make any difference... The only reason causing not work is that

https://github.com/deulos/vue-flux/blob/e3d7cdebf6d5520c5796940cfbc11bc50d6fbd48/src/components/FluxTransition.vue#L97-L102

is not doing anything...

Can you place a Date.now() before each requestAnimationFrame and inside the second to know the time difference? That way I can compare with mines and maybe give us a clue...

About the video don't worry, I will ask a friend with iphone to come home and do some tests

ragnarlotus commented 5 years ago

I just confirmed the glitch in IOS mobile stands... which leads me to latest wildcard, can you see the glitch in https://deulos.github.io/vue-flux/ ?

If not I will have to implement same architecture, using a front stage while loading next transition and even make all process synchronous... 😞

ragnarlotus commented 5 years ago

Hello! just to keep you updated, I've been travelling the past 2 weeks so I didn't have no much time for the slider, but this is how is it going:

Some ideas came from you thank you!

ragnarlotus commented 5 years ago

Hello, I managed to solve the glitch, not in an elegant way but works 100%. Basically depending on the complexity of transition calculates a delay to start playing, although I still want to adjust the delay a bit more.

Feel free to test it although I already checked the animations in 2 iphones and both worked like a charm.

So on Friday I will release the new beta and update the docs. And as promised, If you want, give me any info you want to share as contributor :grin:

I'm also closing the issue as I consider that everything treated here is done and solved but you still can answer.

Thanks again for all your ideas, help and time.

Regards!