#79 inverted smart scroll isn't very smart

SVN
open
nobody
None
5
2014-05-18
2013-10-01
asl97
No

problem: the inverted smart scroll "jumps" over the center part of the page

when: it happen when the image is too long (width) or when the screen resolution isn't high enough

to show a example of the problem, i use this long(width) but short(height) image (attachment),
on my screen(1280x768), normal inverted scroll mode,
it show 1, 2 and 3(part*), when i press the space bar, it "jumps" to 7(part*), 8 and 9
skipping 4, 5 and 6, for manga mode, it "jumps" from 7,8,9 to 1,2,3

if attachment disappear:
http://i.imgur.com/mmA4m9t.png
for a real world example, this page should do: http://gallery.minitokyo.net/download/575195

* part of the number

1 Attachments

Discussion

  • asl97
    asl97
    2013-10-01

    now that i read what i posted, i think i wasn't very clear about what i mean by "when the screen resolution isn't high enough"

    what i mean by that is that when the when the screen resolution isn't big, it make the program to skip over the center of the image for smaller image too,
    if you still don't understand what i mean by that, just ignore the screen resolution part.

    ps, it seem the normal smart scroll works fine, only the inverted smart scroll is effected by it.

     
  • Oddegamra
    Oddegamra
    2013-10-01

    I can see what you mean. Inverted smart scroll started out as a little hack with the least amount of effort required as possible. What is does is simply reverse the distance MComix scrolls into X and Y direction. So, instead of scrolling left/right first, it scrolls down first. This doesn't change that the pivotal points still remain the same, i.e. after having scrolled down, it will jump right to the second page, which usually is the left/rightmost position, depending on normal mode/manga mode. I can imagine that on small screens, parts of the image would be skipped in the center. I'll look into splitting the inverse scrolling mode from the normal mode and then try to make it at least a bit smarter.

     
  • Ark
    Ark
    2013-10-07

    Talking about making it smarter, how about "merging" this feature request (which actually looks like a bug to me) with [#75]?

    I just added axis remapping capabilities to scrolling.py. It would be cool to see this piece of code in action. I attached a piece of code for testing purposes.

    However, there are still some bugs in my attempts to integrate scrolling.py and I don't wont to break anything. I am only used to branching in Git (to some extent) but not SVN. (I know that there is an SVN interface for Git but I am not used to it, either.) Because of that, I don't commit my experiments to the repository here for now.

     

    Related

    Feature Requests: #75

  • Oddegamra
    Oddegamra
    2013-10-07

    Concerning merging of tickets, I'd love to, but apparently, there is no such feature in this current version of the SF ticketing platform. In any case, I'd agree that this is more of a bug than something that should be considered a feature.

    Great work on the scrolling code so far. I've been thinking about ways to best integrate the module into the main code for a while. Even if some things break, this would greatly help to get a better "feel" for changes and how they interface with the remaining code. Unfortunately, I have come up short until now, but I only recently started at a new job and am thus a bit short on time.

    Well, and concerning branching, I'm not completely certain that it would even work for this repository. As far as I know, the repository has to be set-up in a certain way to allow for branches (i.e. it has to have trunk/branches/tags folders at the root level). Incidentally, I also only have a local Git repository that hooks into this SVN repo. If you happen to have a Github account, we can exchange experimental branches there. I usually keep both this repository and http://github.com/oddegamra/mcomix up-to-date. The only problem is that syncing code from Git and then pushing it onto the SVN repository changes Git commits, so other Git repositories often have to be kept in sync with the --force switch to overwrite local history.

     
  • Ark
    Ark
    2013-10-08

    Thank you for the reply.

    I just fixed a few bugs ([r954] and [r955]). Maybe this already helps understanding the flaws in the current design. For example, it would be nice if EventHandler._flip_page looks like this:

    def _flip_page(self, number_of_pages):
        self._extra_scroll_events = 0
        self._window.flip_page(number_of_pages)
    

    That is, it would be nice to get rid of methods like MainWindow.next_page_fast_forward, MainWindow.previous_page etc. and only have a single method MainWindow.flip_page(self, number_of_pages) instead (which itself calls MainWindow.set_page for example). The thing is that these old methods merely differ in (non-trvial) constants! And we should eliminate constants as soon as possible. (For example, see [r955] event.py:71 and event.py:74, I just introduced 10 and -10, respectively. I should go and replace them with something like SEVERAL_PAGES and -SEVERAL_PAGES, respectively.)

    However, there are some pieces of code still depending on some of these methods (e.g. ui.py:39), so we need to get rid of these calls first. However, if you want to keep these methods, we should change them to do nothing but call another method. (Presumably, the new implementation will pass a constant to the other method, so, again, its mere purpose is to get rid of the constant.)

    About branching: I don't know whether or when I will sign up to GitHub, so I hope it's okay if I attach patches and/or files in this thread instead. They might need some discussion anyway. (I wonder if it's possible to turn this SVN repo to a Git repo.)

    PS: It happend again. I got mail for r955 but not for r954. Strange. EDIT: I just got back to my computer and I finally received the mail.

     

    Related

    Commit: [r954]
    Commit: [r955]


    Last edit: Ark 2013-10-08
  • Oddegamra
    Oddegamra
    2013-10-08

    From what I understand, the installer is provided by SF on a purely opt-in basis, as configured by the project administrator. Filezilla agreed to this knowing that their software would be bundled up with other, less savory components for a small monetary compensation. Well, if they wanted to ruin the Filezilla reputation, they did so splendidly. I considered moving away from SF when I read about it, but it's probably more work than it's worth at this point. At least until SF decides to roll out their installer software for everyone without asking for permission first.

     
    • asl97
      asl97
      2013-10-09

      i already move from github to sourceforge only to move to google code
      (cause github need https and so does sourceforge to see the codes)
      that can easily browse the newest code which i use for my inbuilt update system without ssl

      moving is very easy,
      first, make a repo in the place where you want to move to,
      second, clone the empty repo on to your computer,
      third, copy the files from the local repo to the new empty repo,
      fourth, "git add ." to add all the files in the new repo,
      fifth , "git commit -a -m 'move from * to here'" to commit,
      and last, "git push" and a whole lot of waiting

      and it's done :D

       
  • asl97
    asl97
    2013-10-09

    i wonder how long would the repo on fedora (and other linux) take to get updated to the newest xcomix once this bug get fix...

     
  • Ark
    Ark
    2013-10-10

    Thanks for the explanations.

    I identified the following major steps and pitfalls to consider when implementing the smarter scrolling:

    1. Given a 1-dimensional arrangement of n-dimensional non-overlapping boxes S (=a sequence of images) and an intersecting n-dimensional box V (=a viewport), what is the intersection of S and V? (Since we have to deal with plain images, n must be 2 here.) Actually, I already coded something that calculates that. The result says which part of which image is located in which part of the viewport.
    2. This is needed to calculate which image the user is currently looking at. I don't know how to calculate that right now. However, it can't be difficult since viewers such as Evince are capable of calculating this.
    3. Now that we know what the reader is looking at, we remove all the other images from the next calculation. Thus, we might need to adjust the scrollbar values, move the origin to the upper left corner of the image etc. so we can perform the calculations as if there are no other images.
    4. This step is easy: We simply let scrolling.py calculate the new position of the viewport.
    5. Then we integrate the result to the actual viewport that contains the other images, i.e. switch back to the former origin, readjust the scrollbars accordingly etc.

    However, there might be at least the following things to consider:

    • We must ensure that the last two steps do not accidentally change the image we calculated in step 2. For instance, if the reader is looking at image A, we must ensure that the new viewport position means that the reader is still looking at A.
    • However, if (and only if) the reader is about to turn the page (thanks to the scrolling mechanism and because the reader really is at the end of page A), we must turn to page B (and, of course, reflect this by moving the viewport accordingly).
    • In order to handle several images in a row (not necessarily only 2), we might need to perform several page turns at once. For instance, if there are 10 images in a row and the first three of them are completely visible through the reader's viewport, hitting the space bar once should take the reader to the fourth, fifth and sixth image immediately. Another approach would be moving forward only one image per keystroke. Yet another idea: It should be up to the user. But I think this is merely a detail right now.

    Maybe the actual calculations are not as complicated as I think and/or they are already implemented in some OSS. Any way, I attached some ugly Java code that performs some of the calculations we might need. They might also be interesting for [#63].

    Comments are welcome.

     

    Related

    Feature Requests: #63

    Attachments
    • asl97
      asl97
      2013-10-10

      java?
      surely it possible to code it in python, so far, all the java apps i use always seem to crash.

      also, how would you link java code into a python app and wouldn't linking different language cause slow down due to the headers to start both the java and python stuff?

      edit: oh my, it the second page :D

       
      Last edit: asl97 2013-10-10
  • Oddegamra
    Oddegamra
    2013-10-11

    also, how would you link java code into a python app and wouldn't linking different language cause slow down due to the headers to start both the java and python stuff?

    I'm pretty sure he only posted the code for reference, not for actual inclusion.

    Maybe the actual calculations are not as complicated as I think and/or they are already implemented in some OSS. Any way, I attached some ugly Java code that performs some of the calculations we might need. They might also be interesting for [#63].

    Great idea for a general algorithm. Ignoring all images other than the "current" one is especially good, since this essentially reduces the complexity to one page at a time. Concerning item 2, it might not be necessary to do much calculation to determine the image the user is looking at. How about the "active" image is defined simply as the image that takes up more space in the viewport? Since image/viewport position and sizes are known, we could just take the visible width of image 1 and 2 and take the larger of the two. This likely works best for images of about equal sizes. If the user loads a 100x100 next to a 1x1 image (in the worst case), this would of course invalidate possibility of scrolling image 2...

    We could also keep track of pages that have already been scrolled down completely, and thus always scroll one page at a time and increment the counter when the next image is scrolled into view, or decrement the counter if the user steps backwards. The problem to this approach is that the user could just use his mouse to move the picture. When he uses the smart scroll key the next time, the image would revert to the last scrolled position, which would likely make little sense.

    I could also imagine combining both approaches, i.e. use an internal counter for normal scrolling, reset the counter when the user uses his mouse or the arrow keys to move the image, and then determine the active image simply by the larger size when he goes back to using the smart scroll key.

     

    Related

    Feature Requests: #63

  • Ark
    Ark
    2013-10-12

    I'm pretty sure he only posted the code for reference, not for actual inclusion.

    Yep.

    How about the "active" image is defined simply as the image that takes up more space in the viewport?

    This is not sufficient. If you have two images of equal size, both of them are way greater than the viewport (each dimension), the viewport has an even number of pixels in each dimension and you scroll to the center of the "combined" image (that is, the area of the center-right part of the left image equals the area of the center-left part of the right), you simply cannot decide which one to take.

    In these cases, we should use the reading direction (western or manga) in order to make a decision, I think. That is, we should tend to choose the image that appears earlier in the book. In the example above, we should take the left image if we are in western mode, and we should use the right image if we are in manga mode.

    But even this way, choosing the image that consumes more space simply doesn't work. Consider case A in the attachment (manga mode). The first (purple) image consumes less space than the second (green) image. Thus, smart scrolling will continue on the second image and the user will never see the lower right areas of the first image if he/she uses only smart scrolling.

    So I think it is a good idea to not rely on counters or similar "stateful" things. Instead, smart scrolling should work everywhere and behave the same way regardless of how we got into the current set-up of reading direction, viewport and (countless) images. And, of course, if the user uses only smart scrolling, he or she should see every part of every image, smart scrolling always advances towards the end of the book and never enters loops etc.

    In order to do so, I think now that the current image should be the one that is closest to the center of the viewport. (I wonder if other viewers such as Evince behave the same way.) This, however, implies that you can scroll in a way that every image gets a chance to be the closest one.

    A very simple approach works this way: The area that is "visitable" by scrolling is expanded so that every corner of every image can become the center of the viewport. This is illustrated in figure B. This approach has the advantage that, no matter how big the image is, you can always scroll to an "uncovered" (i.e. background-only) area. In figure B, if the user scrolls towards north, MComix could dynamically load the previous two pages. This way, it should be easy to combine this with a "continuous" mode (see [#63]).

    However, if the user does not want to use or cannot use a continuous mode (that's how it currently is, obviously), it might be better to shrink the visitable area to what is really needed for the closest-to-center approach. This is shown in figure C. In order to calculate the visitable area for this case, we need to do the following:

    1. For each image: Calculate the area that is visitable as if this image is the only image that is visible.
    2. Combine these areas to obtain the axis-aligned minimum bounding box.

    The drawback of this closest-to-center approach, however, is that double page mode will not work the same way it currently does.

    Well, I just committed some code that we might need in order to implement the closest-to-center approach, see [r956]. Any questions, suggestions, comments etc.? What to do next?

     

    Related

    Commit: [r956]
    Feature Requests: #63

    Attachments
  • Ark
    Ark
    2013-12-27

    I just added tons of Python code (including docs). (Almost) everything that has something to do with scrolling can now be calculated independently from toolkits such as GTK+. For [r963], I attached some code that demonstrates how to use a FiniteLayout object (an implementation of the arrangements as shown in figure C in my previous post). Have fun experimenting with it. Feel free to ask if you have any questions or suggestions for this API.

    In order to make actual use of this API, it might be necessary to keep the state of a FiniteLayout object and the state of the GUI widgets (mainly viewport position and viewport size) in sync. Admittedly, PyGTK is still a closed book to me, but I think it shouldn't be too difficult to write some code that copies the state of a FiniteLayout to some kind of "unmanaged" widget and vice versa.

     

    Related

    Commit: [r963]

    Attachments
  • Ark
    Ark
    2014-02-06

    Hi Oddegamra! Happy new year! (I'm late, I know.) How's it going?

    It looks like I get more and more used to Python and functional programming while coding for MComix. I'm currently squashing bugs by accident (sorry!) while incorporating zoom.py and scrolling.py. As you can see, in the last few weeks up to r981, I changed lots of things that have a quite visible influence on the user's experience. What do you think about these changes?

    Another question: In previous revisions, "invert smart scroll" is silently ignored if two images are shown. Bug or feature? I changed "invert smart scroll" in r981 because the new way looks more consistent to me. What do you think?

    (Notes to myself: There are some things left to do, especially MainWindow.scroll_to_fixed should be replaced by appropriate calls to FiniteLayout. Oh, and the changelog is kinda outdated.)

     
  • Oddegamra
    Oddegamra
    2014-02-07

    Hey Ark,

    happy new year to you, too. I've been keeping up with your changes and have been syncing regularly to test the things you've been committing. Overall, the quality seems to be quite good, I've yet to find a problem while casually using the program.

    As for invert smart scrolling, I think it was intentional that it did nothing in double-page mode. Originally, the feature was born from the idea that some people release double-page spreads as single image, i.e. [ | ] instead of [_] [_]. The original smart scrolling code does the wrong thing here, since it scrolls down first, then left/right. Of course, it doesn't know that it is looking at two pages, and should be scrolling left/right first, then down. So, for cases like this, you can enable invert smart scrolling and scroll in the right direction. For double-page mode, MComix should do the right thing by itself, since it knows that it is looking at two pages.

    There should be no problem with keeping the functionality as it is right now, though. After all, the user can still enable/disable the setting when necessary.

    Oh, and feel free to update the changelog whenever you think it is appropriate. ;)

     
    Last edit: Oddegamra 2014-02-07
  • asl97
    asl97
    2014-03-21

    it move (left/right) too less if you use the scroll wheel, feature or bug?

     
  • Ark
    Ark
    2014-03-23

    This is supposed to be a feature. See "Preferences" -> "Behaviour" -> "Number of pixels to scroll per mouse wheel turn". (Note that this is different from "Fraction to page to scoll per space key press (in percent)" where do you enter a percentage rather than an absolute number of pixels.) Simply increase these numbers.

    Note: As of [r981], these numbers are interpreted as maximum amounts. (That is, the actual scrolling distance might be smaller.) The current preferences dialog, however, still shows the old labels I used in the previous paragraph.

     

    Related

    Commit: [r981]

  • Ark
    Ark
    2014-04-06

    After the few amendments, refactorings, cleanups etc. I performed the other day, I would like to summarize some important features, properties and constraints of the new smart scrolling implementation. (This also serves as a note to myself so I don't lose track of them.) Here we go:

    1. Every part of every image will eventually be shown to the viewer (if you use only smart scrolling and don't change anything else). This fixes the bug described in this ticket.
    2. The points of the grid the viewport snaps to when using smart scrolling are equally distributed. This greatly improves how the viewer perceives the "speed" of the scrolling, see [#75] for details.
    3. Smart scrolling is now "stateless", meaning that it determines the next reasonable viewport position by only looking at the actual current positions, sizes, orientation etc. (The old implementation made use of state variables like _last_scroll_was_direction_1 and was often confused if something unforeseen had happened.)
    4. If you use double page mode and you scroll to the horizontal center (e.g. by using numpad keys), you might perfectly split the viewport into two halves, each of them showing a portion of the left and the right page, respectively. In this case, the calculations just mentioned will conclude that the viewer has just read the page that comes earlier in the book. This also holds true if there is no perfect split because the sum of the widths of these two halves (excluding the gap between them) is odd. To handle this, the images are arranged in a way that, if you scroll to the horizontal center using the numpad keys, the page that appears earlier in the book will be closer to the center than the other one and thus said to be the page the viewer just has read.
    5. More generally, the page that is closest to the center of the viewport is said to be the current one. There is a method called FiniteLayout.get_current_index(self) that determines the corresponding index (and, as implied earlier, respects the current orientation, e.g. western mode or manga mode). However, this property is currently used only by the smart scrolling implementation. This leads to inconsistencies. The "open with" feature, for example, always thinks that the current image is the one highlighted in the thumbnailer, no matter where you scroll to when in double page mode.
    6. When you are in single page mode and the image is wide enough so you can scroll horizontally and there is an odd number of horizontal positions smart scrolling snaps to, there is a center position it will snap to eventually. (This is because these positions are equally distributed, as described above.) The current implementation ensures that this horizontally centered position equals the horizontally centered position you scroll to when using the numpad keys. (This also works accordingly for the vertical axis when using "invert smart scroll", see below.)
    7. The source code for all of these geometry issues has been generalized to cope with any (natural) number of dimensions. While this might look overengineered at the first glance, it leads to less repetitive code and makes it way harder for bugs to hide. (Note: This generalization process is not yet finished, actually.)

    Now for the downside: At least lens.py doesn't know about the new backend, though. As a result, the magnifying glass is totally broken right now. In order to fix this, I would like to write a new abstraction that directly uses PIL to perform all the necessary operations. (The current implementation uses a mix of some of GTK's built-in features, e.g. rotate_simple(gtk.gdk.PIXBUF_ROTATE_CLOCKWISE) and some of PIL's features.) This would implicitly and thus easily fix some other old bugs as well. (For example, the lens currently totally ignores what you have entered in the "Enhance image" dialog.)

    @Oddegamra: Changing the backend to use PIL only would replace the "best" interpolation method from a hyperbolic interpolation to a bicubic interpolation because this is the best one PIL offers. (See https://en.wikipedia.org/wiki/Bicubic_interpolation for an example of what it looks like.) Is this okay for you?

    By the way, instead of using PIL, I'm currently using Pillow right now, and it perfectly works as a drop-in replacement for PIL with the advantage that it is actively maintained. See https://pypi.python.org/pypi/Pillow/2.4.0 for details. Maybe you want to replace the PIL library used in the all-in-one bundles (like the current mcomix-1.00.win32.all-in-one.zip) with Pillow for the next release. (If you do so, don't forget to mention Pillow as an alternative to PIL in the readme.)

     

    Related

    Feature Requests: #75

  • Oddegamra
    Oddegamra
    2014-04-07

    I noticed some of these things, yeah. Getting rid of the state (scroll direction) has been an especially good improvement and might as well have fixed a few other annoying bugs (such as switching from an overly large, scrolled image to a new page: the next scroll step would often step to a completely unexpected position). Great work.

    The lens' implementation has been a bother for a long time, so basically any change you make will probably be a 100% improvement to the current situation. If that means sacrificing some of GTK's post-processing: Well, whatever. I have considered moving away from Pixbuf in the past, but it turned out not to be feasible, since in the end, most of GTK's widgets still need a pixbuf at the end of the day (such as the thumbnail sidebar and the library's book widget). As long as only the lens is affected, it probably won't matter that much.

    Concerning Pillow, I already switched myself, too. Already edited the website to mention Pillow. If it isn't in the readme, I might have forgotten about that point. Will be fixed in a few moments.

     
  • Ark
    Ark
    2014-05-18

    Thinking about the lens, I consider a new rendering pipeline (if you want to call it this way) to improve memory usage, speed and responsiveness of MComix, especially when dealing with large images. The general idea looks like this:

    • Always use PIL Image objects for any operation, i.e. only use gtk.Image if there is no other way.
    • Create working copies only if necessary. These copies should be as small as possible.
    • Apply all affine transforms at once, including scaling.
    • Apply all color ops (contrast, brightness etc.) at once. This reduces noise and should improve both memory usage and speed (as opposed to the current implementation of "Enhance image" where there are new images for each color op with accumulated rounding errors, at least if these ops are not lazy). This might need some testing to check whether it really works as intended.

    There are some pitfalls:

    • The resampling method used for the affine transforms might need some more pixels from the environment due to the support the respective interpolation function might have. This also applies to sharpening and blurring.
    • Pillow's affine transform function does not accept ANTIALIAS as interpolation filter. Pillow's resize function, however, does. This issue has already been reported to the Pillow developers. In the meantime, we might need to work around this issue which means that we cannot apply all affine transforms at once but have to introduce an additional step.
    • Pillow's functions are not lazy in general. In order to achieve good results, we must apply sharpening/blurring before rescaling. However, if there is a large image and you zoom out, sharpening/blurring might take a very long time and a lot of memory. I consider cheating in this case. That is, if upscaling is necessary, everything is performed as usual. However, if downscaling is necessary (due to "zoom to fit", for example) sharpening/blurring is applied to the downscaled image rather than the original image. In order to compensate the relatively large radius in this case, I consider simply weakening the effect of the convolution. I don't know whether this is a valid approximation but a few tests in GIMP suggest that it might work quite well.
    • Since some resampling methods and filters might produce pixel values below 0 or above max, it might be a good idea to use floating point values rather than integers for the temporary images. However, MComix is a simple viewer and not a scientific image restauration application, so integers should work just fine.

    Thus, there are at least two cases to consider:

    • Case 1 (scaling factor > 1):
      • Create a pre-cropped working copy of the region of interest (including border pixels for the support).
      • Apply sharpening/blurring to the working copy. (This might create a new working copy.)
      • Apply the resulting affine transform to the working copy. (This will create a new working copy.)
      • Apply all color ops to the working copy.
      • Create a converted (and possibly cropped) image so PyGTK can render the result.
    • Case 2 (scaling factor <= 1):
      • Apply the resulting affine transform to the original image. (This will create a new pre-cropped working copy.) Make sure you keep enough border pixels for the convolution.
      • Apply sharpening/blurring to the working copy. Reduce this effect to compensate the new radius.
      • Apply all color ops to the working copy.
      • Create a converted (and possibly cropped) image so PyGTK can render the result.

    This way, we only need to create working copies that are as big as the screen (and some more pixels for the border) at most. Furthermore, this approach might work for the main area as well as the lens or the thumbnails, eliminating redundancies in the code.

    I attached a first draft. The only things it can do right now are "flip" and "rotate". Simply run it in a working directory that contains a "test_input.png" and it will create a "test_output.png". (The demo code shows a quite complicated way to do nothing so the result will be a simple copy as the matrix implies.)

    The bad thing is that I don't have that much time right now so any help would be greatly appreciated.

    EDIT: Some more details added, scaling factor = 1 pushed to case 2.

     
    Last edit: Ark 2014-05-18