crash on zoom - Githubissues

brodavi commented 9 years ago

First let me say THANK YOU! I have been wanting something like this for ages, and I've been fiddling around with my own attempts at this without much success. What you have here is a thing of beauty in my eyes.

But I did in fact experience a crash. A pretty bad one. I went to http://josephernest.github.io/bigpicture.js/bigpicture.html and started panning, then double-clicking to zoom in on some text, then ctrl-double-clicking to zoom out. Then I used my laptop's trackpad to attempt zooming in/out, but almost immediately zoomed in or out so far that the screen was emptied. I tried zooming back in/out with the trackpad, but couldn't find a zoom level with anything visible. I remembered reading something about F2 being the 'biggest picture', and that I should maybe try it twice, so I hit F2 twice. By this point, my harddrive was thrashing, and my mouse was stuttering badly. I was unable to close the tab, and unable to bring up the task manager. I let it alone for a few minutes of painful flailing about, and the tab finally disappeared.

This is Windows 8.1 and Chrome.

Looking through the code very briefly, the only thing that I could come up with was maybe the mins and maxs in seethebigpicture() should be less massive than -+Infinity? Not really sure though. I'd have to look at it more.

Thanks again! This is awesome :)

blakev commented 9 years ago

Without looking yet, it should be numerically bound on one end...and since this is a big picture, probably the minimum.

josephernest commented 9 years ago

Thanks @brodavi for your comments!

1) Yes I also noticed a performance problem with Chrome (verrrry slow panning when too much zoomed in). Strangely, it happens with Chrome only : the same page with the same big zoom factor in Firefox or IE10 is totally smooth when panning. Possible reason : when big zoom factor, the container <div> is very big (could easily be 1'000'000px wide), and probably Chrome tries to keep everything in memory, that could cause the crash. I currently don't know how to deal with that on Chrome. Any idea on how to say to Chrome "Don't try to load everything in memory!" ?

2) The "press F2 twice" mention is probably a bit unclear in my demo. In fact: "Press F2" => this will display the biggest picture available, "Press F2 again" => it will bring you back to where you were before. I'm looking for a better sentence in the demo to explain this feature, any idea ?

3) The var minX = +Infinity is just trick to be sure that the if condition in if (rect.left < minX) minX = rect.left; will always be true on the first call. I don't think the crash will come from here, it will surely come more from the size of the container <div>.

4) A question @brodavi @blakev : on http://bigpicture.bi, the data is saved on server, but here for this small library demo, the data is stored in localStorage. Consequence: if you clean browser history, the text goes away.

Question: in this small live demo, should I remove the localStorage feature, so that each time you reload the page, the user always begins with the original page (i.e. modifications will be lost in this small demo) ?

blakev commented 9 years ago

@josephernest yeah, I'd probably remove it. Or put some kind of "Start Over" button in a corner.

brodavi commented 9 years ago

Haha no I don't know how to tell that to chrome. Have you considered a quadtree for a sort of scenegraph type of optimization? I would hate to add complexity to your simple system, but in this case it might be necessary.

I would recommend starting the demo with just the text "click and drag left", then this action will reveal more text: "congratulations, you have learned panning! Double-click here-> . <- to zoom" The user zooms to reveal the text "you got zooming! ctrl-double click will zoom out, but for now, press F2", which will zoom out to max level. At this level, more text is visible: "F2 will bring you to this max zoom level. Press F2 to return to the zoom level you were previously. Single-click to add text. Congrats! You've learned everything!"

This makes the demo more engaging and lets the user build skill as a game.

I agree with @blakev. I would remove the automatic saving to localstorage. I would add the button "Start Over", and I would also add the button " Save".

I can make a pull request for these things, if you'd like.

josephernest commented 9 years ago

Yes, quadtree may be required for optimization... (Do you mean it would only load/render the necessary textboxes, and not the whole universe of text? like Google Maps, etc. ?)

Great idea for the Demo that lets the user build skill as a game! Ok for a pull request! :)

Ok, let's remove automatic saving to localStorage.
Would "Start Over" button be equivalent to reload of the page ? (if no localStorage, Start Over button may be not necessary => reload of the page would do the same?) Where would button "Save" save to ? localStorage ? Export as HTML ?

More generally I'm hesitating :

Should this library demo here only present the core things : zooming / panning features ? (then anyone could use this library in any website)

or

Should the library demo present saving features like in the bigger website (that use server side PHP for saving) http://bigpicture.bi ?

brodavi commented 9 years ago

Yes, I think you are right. For the core things, it is not necessary to do localstorage. It can only add confusion. I made a pull request to remove localstorage serialization.

Also made a separate pull request for game-style tutorial.

Thinking more about optimization, quad tree doesn't make sense in this context, because there are multiple zoom levels, and you may want to hide things that are huge or tiny in scale, but still within the 'viewable window' of your quad tree. So maybe just a simple check for size? If size is greater or less than some threshold of current zoom level, set DOM element to be hidden maybe?

josephernest commented 9 years ago

Thanks @brodavi for the pull requests, I have added them!

About performance problem with Chrome, here is a small test which will help to reproduce the issue:

try to launch this page http://gget.it/m1lqe1g3/bigpicture-test.html (you first need to save/copy it in the same folder as bigpicture.js)
there are only 2 textboxes : 1 small text, and 1 big text (on the right)
zoom on the small text so that one letter takes the full height of the browser
it will be super slow on Chrome when dragging! Why?

josephernest commented 9 years ago

@brodavi A single modification in the CSS (position: fixed instead of position: absolute) seems to improve a lot the rendering performance with Chrome. I don't know why but it seems to work. I just modified the CSS in the last commit. Can you try if it works for you ?

Running your tutorial with full zoom on the sentence "You've mastered zooming in" (one letter = 800 px high :) ) :

was very slow when dragging on Chrome before / crashing sometimes
is normal very smooth on Chrome now

I hope it's the same for you. If so, it could solve this Crash on zoom issue.

__ Another example http://gget.it/caveot4g/big-div-test.html (slow) http://gget.it/e0ubdh67/big-div-test_fixed.html (seems to work now thanks to this little modification)

brodavi commented 9 years ago

@josephernest Yes, that seems to be much faster indeed! Great! :)

josephernest / bigpicture.js

crash on zoom #1