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

ImageViewer: Clamp zoom factor to sane values #9529

Merged
merged 37 commits into from Sep 19, 2022
Merged

Conversation

NiLuJe
Copy link
Member

@NiLuJe NiLuJe commented Sep 17, 2022

Computed with magic ;p

(Seriously, don't ask me to explain the maths :D).


This change is Reviewable

Should avoid egregious values that would potentially alloc insanely
large buffers (and likely fail to do so).
My napkin math was based on *total* math, so I'll probably bump the
percentage...
What we get by the time _render has finished has most likely already
been prescaled...
ImageWidget

Otherwise the max factor gets ever so slightly smaller each time
because we eat more memory at higher zoom levels ;).
Which makes the final zooming steps confusing as it jitters back and
forth.
@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 17, 2022

I'm still mildly unhappy with how fast it zooms out and how slow it zooms in (but mostly by the zoom out, the zoom in is somewhat okay), but that was admittedly not the focus here ;).

Re #9524

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 18, 2022

Okay, tried something stupid about the zoom outs.

At first glance, it feels slightly less worse. It's stupid, though ;p.

@poire-z
Copy link
Contributor

poire-z commented Sep 18, 2022

It's stupid, but it more or less works, maybe?

It just feels random :)

Mixed feelings/thoughts about the min/max scale factor depending on free mem:

  • I hope they get really low/high so we won't usually meet your values, and this change won't be noticed
  • it feels awkward that the min/max zoom factors depends on the weather (the current memory situation), and are not deterministic: 10 minutes ago, I was able to zoom in to read some little area in that map, and now that I'm in the jungle and I need to see more details, I can't zoom as much as 10 mn ago (ok, I can restart KOReader... but still).
  • I dunno how the free memory reported relates to the ability to malloc() a large blitbuffer: I feel it has to be a contiguous segment, while the free memory will report the sum of all free little segments: so, we may have a lot of free memory, and not be able to malloc'ate a medium continuous segment (?) (or is that that the kernel/libc is able to fake us a continuous segment from many little fragments with the cpu/kernel/process virtual address space ?)

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 18, 2022

  • I hope they get really low/high so we won't usually meet your values, and this change won't be noticed

That's the idea ;o).

  • it feels awkward that the min/max zoom factors depends on the weather (the current memory situation), and are not deterministic: 10 minutes ago, I was able to zoom in to read some little area in that map, and now that I'm in the jungle and I need to see more details, I can't zoom as much as 10 mn ago (ok, I can restart KOReader... but still).

Again, the idea is that, if we can't reach it, it's because it would very likely have crashed. (and/or been extremely slow because of general memory pressure).

  • I dunno how the free memory reported relates to the ability to malloc() a large blitbuffer: I feel it has to be a contiguous segment, while the free memory will report the sum of all free little segments: so, we may have a lot of free memory, and not be able to malloc'ate a medium continuous segment (?) (or is that that the kernel/libc is able to fake us a continuous segment from many little fragments with the cpu/kernel/process virtual address space ?)

MMU magic ((mostly) irrelevant concern ;)). Hence heuristics entering the stage instead of assuming all available memory is fair game.

@poire-z
Copy link
Contributor

poire-z commented Sep 18, 2022

Would need a bit of maths (or the mood to get into that):

  • we shouldn't directly play with the scale_factor
  • we should get the width (or height, or diagonal) of the image in the view, and the distance of the gesture - and compute the new width stretched/shrinked by that gesture (or the width at start of gesture vs the width at end of gesture) - and then compute the new scale_factor from that new width
  • and may be all that can just be a formula to scale_factor :)

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 18, 2022

  • we should get the width (or height, or diagonal) of the image in the view, and the distance of the gesture - and compute the new width stretched/shrinked by that gesture (or the width at start of gesture vs the width at end of gesture) - and then compute the new scale_factor from that new width

I napkin math'ed something like that, and it felt like it would lead to (usually) large jumps.

Haven't tried it in practice, though, and I agree that it sounds a lot saner ;).

Copy link
Contributor

@zwim zwim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all the zooming thing looks a bit saner and more straight forward than the old solution.

Did not look into the getScaleFactorExtrema.

Not tested (no time).

frontend/cache.lua Outdated Show resolved Hide resolved
frontend/util.lua Outdated Show resolved Hide resolved
@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 18, 2022

  • we should get the width (or height, or diagonal) of the image in the view, and the distance of the gesture - and compute the new width stretched/shrinked by that gesture (or the width at start of gesture vs the width at end of gesture) - and then compute the new scale_factor from that new width

I napkin math'ed something like that, and it felt like it would lead to (usually) large jumps.

Haven't tried it in practice, though, and I agree that it sounds a lot saner ;).

Whelp, a good night's sleep helps.

It makes a hell of a lot more sense, and it finally works sensibly :}.

Comment on lines 447 to 452
function ImageWidget:recomputeScaleFactor(scaling)
local req_width = self._bb:getWidth() * scaling
local req_height = self._bb:getHeight() * scaling

-- Best fit
return math.min(req_width / self._img_w, req_height / self._img_h)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't self._bb:getWidth() == self._img_w (idem for height) ? Aren't A and B the same in the the math.min(A, B) ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._bb is the scaled bb, while self_img_* is the unscaled image (i.e., the actual reference to compute the scale factor).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i.e., the two only match in an hypopthetical scale_factor == 1 scenario.

Copy link
Member Author

@NiLuJe NiLuJe Sep 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't A and B the same in the the math.min(A, B) ?

They ought to be since we're only concerned with best-fit w/ AR preserved here, yeah.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but we do not crop (and I'd like this to stay as is : zoom takes time, but panning is fast as it works with the same self._bb).
So, self._bb and the unscaled image keep the same A/R.
So, it looks to me that req_width / self._img_w == req_height / self._img_h == scaling.
(But I may miss something :) I'm just having a look at your diffs...)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, it looks to me that it comes down to:

function() ImageWidget:recomputeScaleFactor(scaling)
    return self.scale_factor * scaling
end

(except for the cases where there is not scale_factor or it is nil or 0 :) but we could as well store the scale_factor we use the last time as a property - and may be we are already doing that.)

But it doesn't feel like documentation or defensivness: it feels like there's some clever essential computation here and that one should stop here to understand it (like I'm doing :) and well, I stopped and I'm still stopped :) The comments is still confusing. It feels like someone got lost in maths and is taking too much precautions :) which I understand :) Just sharing so you might spend more time to be sure it's needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

function() ImageWidget:recomputeScaleFactor(scaling)
return self.scale_factor * scaling
end

It is very close to that (as in, the only difference introduced is because of minute differences between w & h, the various truncation passes, and rounding errors).

(except for the cases where there is not scale_factor or it is nil or 0 :) but we could as well store the scale_factor we use the last time as a property - and may be we are already doing that.)

It's already re-computed first in IV if it's 0.


I can go back to scale_factor * scaling if you think it's clearer, it won't change much in terms of actual results ;).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do think it's clearer - in the sense it doesn't deceive you by making you think it's doing more and different than the previous self.scale_factor = self.scale_factor + inc :) (while I was somehow hoping it did different :)
(ie. it's clearer having this single line in ImageViewer, that having to follow the trail into ImageWidget to somehow conclude it only does this).

Copy link
Member Author

@NiLuJe NiLuJe Sep 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it is different, since it's actually matching what rescaling the image does (i.e., scale_factor = scale_factor * (1 + inc) or * (1 - dec), which is different than what happened previously, where we just uniformly added/took a chunk of the factor directly.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The change indeed did not jump as me, like it does now :)

local req_height = self._bb:getHeight() * scaling
logger.dbg("From", self._bb:getWidth(), self._bb:getHeight(), "to", req_width, req_height)

-- Best fit (in most cases, we're operating on a best-fit scale factor already, so both sides should match).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still asking :) "in most cases" - did you meet cases where both sides did not match ?
Also, I dunno about the word "best-fit" : we always keep aspect ratio, we just scale, always keeping the original aspect ration - and we start the image scaled so it's fully contained in the screen, A/R preserved.

Copy link
Member Author

@NiLuJe NiLuJe Sep 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, but I haven't checked many IV callers ;).

(Best-fit is the wording used in the original code; because there is a stretch codepath, but, IIRC, it's only used by the Screensaver and the Screensaver-like plugins).

Copy link
Member Author

@NiLuJe NiLuJe Sep 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And those callers should never trip manual zooms ;).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, ok, you thought about these then. Logic splitted between simple ImageViewer and twisted serves-all-use-cases ImageWidget doesn't make things clear :/

@poire-z
Copy link
Contributor

poire-z commented Sep 18, 2022

I trust you on the user experience with pinch/spread.
Please check it works as nicely with swipe along the left/right screen edges (which is the only option on devices without multitouch, like the emulator :)

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 18, 2022

Please check it works as nicely with swipe along the left/right screen edges

Those are currently always computed relative to the screen's dimensions (which sorta makes sense given the gesture), I'll see if I can fudge it similarly (because otherwise we're back to needing seven gestures to zoom in on low factors ;p).

Also, update the zone to match GestureManager's *_edge zones.
(Which matches how wide my tiny fingers actually are ;p).
As it apparently doesn't make the intent clearer ;).
@poire-z
Copy link
Contributor

poire-z commented Sep 18, 2022

OK, tested on my Kobo: feels a lot less frustrating, and feels rather alright.
Sometimes, zoom out feels too radical, and often get me too quickly to a too small image - but I guess it can feel normal.
Also, the zoom-in limitation sometimes feels like a limitation :) But I guess it's also ok: I could zoom in enough to be able to read the detail I wanted as a user (but not enough to see the marvels of LunaSVG scaling :) ie. the first image in my test suite #9510 (comment) (map of north of spain).

Probably unrelated to these changes, and I can't reproduce it on the emulator: zooming in on my Kobo on the images at page 20 (some biology stuff in French) and page 58 (map of the world with flags) crashes (before your max cap stops me) with something I have never seen: ./luajit: C++ exception. Probably LunaSVG/crengine :/

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 18, 2022

Sometimes, zoom out feels too radical, and often get me too quickly to a too small image - but I guess it can feel normal.

Yup, same general feeling here, FWIW. When the distance is relative to the screen, it feels better when zooming in (only a tad slow), but it's extremely irritating when zooming out from small factors.

(What I mean to say is that this is a compromise that feels okay in most cases, compared to stuff that might feel better in some cases but very much worse in others).

Also, the zoom-in limitation sometimes feels like a limitation :)

We can always bump up the heuristics later, I ended by testing on an Elipsa, where we have RAM to spare, and it's definitely on the low end of things, so we should have a bit of leeway ;).

./luajit: C++ exception.

I had one crash inside MuPDF during testing, too (on the H2O, at high scaling, so it might have been caused by memory exhaustion somewhere inside mupdf during scaling. Or not ;p).

@poire-z
Copy link
Contributor

poire-z commented Sep 18, 2022

Alas, not much help from your gdb on my kobo:

09/19/22-01:30:48 DEBUG Contact:voidState Contact lift detected
09/19/22-01:30:48 DEBUG ImageViewer:_clean_image_wg
09/19/22-01:30:48 DEBUG CreImage: scaling for 4.9095478425049 1072 1448
/mnt/onboard/.adds/koreader/luajit: C++ exception
[Inferior 1 (process 4163) exited with code 01]
#

We can always bump up the heuristics later, I ended by testing on an Elipsa, where we have RAM to spare, and it's definitely on the low end of things, so we should have a bit of leeway ;).

I guess it's fine.
I wasn't using much RAM, this is the limit I get:

09/19/22-01:34:01 DEBUG ImageViewer:_clean_image_wg
09/19/22-01:34:01 DEBUG CreImage: scaling for 4.2277288519093 1072 1448

09/19/22-01:35:08 DEBUG ImageViewer:onZoomIn: Hit the max scaling factor: 4.2277288519093

when rss 85m vsz 293m luajit /mnt/onboar

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2022

Alas, not much help from your gdb on my kobo:

You have to set a break on throw or something to catch a C++ exception, IIRC.

@poire-z
Copy link
Contributor

poire-z commented Sep 19, 2022

You have to set a break on throw

Ok, this worked:

09/19/22-12:02:37 DEBUG CreImage: scaling for 5.4721249724593 1072 1448

Catchpoint 1 (exception thrown), 0x2b7ea020 in __cxa_throw ()
   from /mnt/onboard/.adds/koreader/./libs/libstdc++.so.6
(gdb) bt
#0  0x2b7ea020 in __cxa_throw ()
   from /mnt/onboard/.adds/koreader/./libs/libstdc++.so.6
#1  0x2b7ea48c in operator new(unsigned int) ()
   from /mnt/onboard/.adds/koreader/./libs/libstdc++.so.6
#2  0x2d3e9e72 in lunasvg::Bitmap::Impl::Impl(unsigned int, unsigned int) ()
   from /mnt/onboard/.adds/koreader/./libs/liblunasvg.so
#3  0x2d3e9f2c in lunasvg::Bitmap::Bitmap(unsigned int, unsigned int) ()
   from /mnt/onboard/.adds/koreader/./libs/liblunasvg.so
#4  0x2d3eab82 in lunasvg::Document::renderToBitmap(unsigned int, unsigned int, unsigned int) const () from /mnt/onboard/.adds/koreader/./libs/liblunasvg.so
#5  0x2d202516 in LVSvgImageSource::Render(int&, int&, unsigned int, bool) ()
   from /mnt/onboard/.adds/koreader/./libs/libcrengine.so
#6  0x2c0eac6e in ?? () from ./libs/libkoreader-cre.so
#7  0x00059a20 in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)

So, probably similar to a malloc() failing:

Bitmap::Impl::Impl(std::uint32_t width, std::uint32_t height)
    : ownData(new std::uint8_t[width*height*4]), data(nullptr), width(width), height(height), stride(width * 4)
{
}

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2022

Okay, so maaaaaybe not so much breathing room after all :D.

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 5 files at r1, 2 of 4 files at r4, 1 of 2 files at r11, 1 of 1 files at r12, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @poire-z and @zwim)


frontend/cache.lua line 170 at r3 (raw file):

Previously, zwim wrote…

Additionally, I don't know what would happen if you call memfree /memtotal (when they come wrong lshifted from calcFreeMem).

Done.

Copy link
Member Author

@NiLuJe NiLuJe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r13, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @poire-z and @zwim)

@NiLuJe NiLuJe merged commit 38919c2 into koreader:master Sep 19, 2022
@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2022

I'm playing with another approach: zooming to the exact dimension that matches the distance between the final position of the two fingers. That possibly feels more natural, or at least more like what would happen if the rescaling was live and followed your fingers, like a corner drag to resize a window.

It's... mildly fun. Haven't played with it much, though. It requires tweaking how GestureDetector computes the gesture distance, which may be an issue for other pinch users (it currently uses the travel distance of both fingers).

diff --git a/frontend/device/gesturedetector.lua b/frontend/device/gesturedetector.lua
index 80d460d62..361bd77a6 100644
--- a/frontend/device/gesturedetector.lua
+++ b/frontend/device/gesturedetector.lua
@@ -1078,6 +1078,7 @@ function Contact:handleTwoFingerPan(buddy_contact)
                 ges_ev.ges = "outward_pan"
             end
             ges_ev.direction = gesture_detector.DIRECTION_TABLE[tpan_dir]
+            ges_ev.distance = end_distance
         elseif self.state == Contact.holdState then
             ges_ev.ges = "two_finger_hold_pan"
             -- Flag 'em for holdState to discriminate with two_finger_hold_release
diff --git a/frontend/ui/widget/imageviewer.lua b/frontend/ui/widget/imageviewer.lua
index b6e62e084..1ddaf9c3b 100644
--- a/frontend/ui/widget/imageviewer.lua
+++ b/frontend/ui/widget/imageviewer.lua
@@ -637,6 +637,7 @@ end
 
 -- Zoom events
 function ImageViewer:onZoomIn(inc)
+    logger.dbg("ImageViewer:onZoomIn", inc)
     if self.scale_factor == 0 then
         -- Get the scale_factor made out for best fit
         self.scale_factor = self._scale_factor_0 or self._image_wg:getScaleFactor()
@@ -671,6 +672,7 @@ function ImageViewer:onZoomIn(inc)
 end
 
 function ImageViewer:onZoomOut(dec)
+    logger.dbg("ImageViewer:onZoomOut", dec)
     if self.scale_factor == 0 then
         -- Get the scale_factor made out for best fit
         self.scale_factor = self._scale_factor_0 or self._image_wg:getScaleFactor()
@@ -706,6 +708,97 @@ function ImageViewer:onZoomOut(dec)
     return true
 end
 
+function ImageViewer:onZoomToHeight(height)
+    logger.dbg("ImageViewer:onZoomToHeight", height)
+    if self.scale_factor == 0 then
+        -- Get the scale_factor made out for best fit
+        self.scale_factor = self._scale_factor_0 or self._image_wg:getScaleFactor()
+    end
+
+    local new_factor = height / self._image_wg._img_h
+
+    if not self._min_scale_factor or not self._max_scale_factor then
+        self._min_scale_factor, self._max_scale_factor = self._image_wg:getScaleFactorExtrema()
+    end
+    -- Clamp to sane values
+    new_factor = math.min(new_factor, self._max_scale_factor)
+    new_factor = math.max(new_factor, self._min_scale_factor)
+    if new_factor ~= self.scale_factor then
+        self.scale_factor = new_factor
+        self:update()
+    else
+        if self.scale_factor == self._min_scale_factor then
+            logger.dbg("ImageViewer:onZoomOut: Hit the min scaling factor:", self.scale_factor)
+        else
+            logger.dbg("ImageViewer:onZoomOut: No change in scaling factor:", self.scale_factor)
+        end
+    end
+    return true
+end
+
+function ImageViewer:onZoomToWidth(width)
+    logger.dbg("ImageViewer:onZoomToWidth", width)
+    if self.scale_factor == 0 then
+        -- Get the scale_factor made out for best fit
+        self.scale_factor = self._scale_factor_0 or self._image_wg:getScaleFactor()
+    end
+
+    local new_factor = width / self._image_wg._img_w
+
+    if not self._min_scale_factor or not self._max_scale_factor then
+        self._min_scale_factor, self._max_scale_factor = self._image_wg:getScaleFactorExtrema()
+    end
+    -- Clamp to sane values
+    new_factor = math.min(new_factor, self._max_scale_factor)
+    new_factor = math.max(new_factor, self._min_scale_factor)
+    if new_factor ~= self.scale_factor then
+        self.scale_factor = new_factor
+        self:update()
+    else
+        if self.scale_factor == self._min_scale_factor then
+            logger.dbg("ImageViewer:onZoomOut: Hit the min scaling factor:", self.scale_factor)
+        else
+            logger.dbg("ImageViewer:onZoomOut: No change in scaling factor:", self.scale_factor)
+        end
+    end
+    return true
+end
+
+function ImageViewer:onZoomToDiagonal(d)
+    logger.dbg("ImageViewer:onZoomToDiagonal", d)
+    if self.scale_factor == 0 then
+        -- Get the scale_factor made out for best fit
+        self.scale_factor = self._scale_factor_0 or self._image_wg:getScaleFactor()
+    end
+    -- Enter trig! c.f., https://math.stackexchange.com/a/3369637
+    local r = self._image_wg:getCurrentWidth() / self._image_wg:getCurrentHeight()
+    local h = math.sqrt(math.pow(d, 2) / (math.pow(r, 2) + 1))
+    local w = h * r
+    logger.dbg("Current factor:", self.scale_factor)
+    logger.dbg("Current: w =", self._image_wg:getCurrentWidth(), "h =", self._image_wg:getCurrentHeight(), "d =", self._image_wg:getCurrentDiagonal(), "r =", self._image_wg:getCurrentWidth() / self._image_wg:getCurrentHeight())
+    logger.dbg("New: w =", w, "h =", h, "d =", d, "r =", w / h)
+
+    local new_factor = math.min(w / self._image_wg._img_w, h / self._image_wg._img_h)
+
+    if not self._min_scale_factor or not self._max_scale_factor then
+        self._min_scale_factor, self._max_scale_factor = self._image_wg:getScaleFactorExtrema()
+    end
+    -- Clamp to sane values
+    new_factor = math.min(new_factor, self._max_scale_factor)
+    new_factor = math.max(new_factor, self._min_scale_factor)
+    if new_factor ~= self.scale_factor then
+        self.scale_factor = new_factor
+        self:update()
+    else
+        if self.scale_factor == self._min_scale_factor then
+            logger.dbg("ImageViewer:onZoomOut: Hit the min scaling factor:", self.scale_factor)
+        else
+            logger.dbg("ImageViewer:onZoomOut: No change in scaling factor:", self.scale_factor)
+        end
+    end
+    return true
+end
+
 function ImageViewer:onSpread(_, ges)
     -- We get the position where spread was done
     -- First, get center ratio we would have had if we did a pan to there,
@@ -719,36 +812,57 @@ function ImageViewer:onSpread(_, ges)
     -- meaning using the image dimensions here takes less zoom steps to get it back to a sensible size;
     -- *and* large scale factors (where the image dimensions are larger than the screen),
     -- meaning using the screen dimensions here makes zoom steps, again, slightly more potent.
-    local inc
     if ges.direction == "vertical" then
-        inc = ges.distance / math.min(Screen:getHeight(), self._image_wg:getCurrentHeight())
+        if ges.distance > self._image_wg:getCurrentHeight() then
+            self:onZoomToHeight(ges.distance)
+        else
+            self:onZoomIn(ges.distance / math.min(Screen:getHeight(), self._image_wg:getCurrentHeight()))
+        end
     elseif ges.direction == "horizontal" then
-        inc = ges.distance / math.min(Screen:getWidth(), self._image_wg:getCurrentWidth())
+        if ges.distance > self._image_wg:getCurrentWidth() then
+            self:onZoomToWidth(ges.distance)
+        else
+            self:onZoomIn(ges.distance / math.min(Screen:getWidth(), self._image_wg:getCurrentWidth()))
+        end
     else
-        local tl = Geom:new{ x = 0, y = 0 }
-        local br = Geom:new{ x = Screen:getWidth() - 1, y = Screen:getHeight() - 1}
-        local screen_diag = tl:distance(br)
-        inc = ges.distance / math.min(screen_diag, self._image_wg:getCurrentDiagonal())
+        if ges.distance > self._image_wg:getCurrentDiagonal() then
+            self:onZoomToDiagonal(ges.distance)
+        else
+            local tl = Geom:new{ x = 0, y = 0 }
+            local br = Geom:new{ x = Screen:getWidth() - 1, y = Screen:getHeight() - 1}
+            local screen_diag = tl:distance(br)
+            self:onZoomIn(ges.distance / math.min(screen_diag, self._image_wg:getCurrentDiagonal()))
+        end
     end
-    self:onZoomIn(inc)
     return true
 end
 
 function ImageViewer:onPinch(_, ges)
     -- With Pinch, unlike Spread, it feels more natural if we keep the same center point.
     -- Set some zoom decrease value from pinch distance
-    local dec
     if ges.direction == "vertical" then
-        dec = ges.distance / math.min(Screen:getHeight(), self._image_wg:getCurrentHeight())
+        -- FIXME: Only if image is smaller than the screen?
+        if ges.distance < self._image_wg:getCurrentHeight() then
+            self:onZoomToHeight(ges.distance)
+        else
+            self:onZoomOut(ges.distance / math.min(Screen:getHeight(), self._image_wg:getCurrentHeight()))
+        end
     elseif ges.direction == "horizontal" then
-        dec = ges.distance / math.min(Screen:getWidth(), self._image_wg:getCurrentWidth())
+        if ges.distance < self._image_wg:getCurrentWidth() then
+            self:onZoomToWidth(ges.distance)
+        else
+            self:onZoomOut(ges.distance / math.min(Screen:getWidth(), self._image_wg:getCurrentWidth()))
+        end
     else
-        local tl = Geom:new{ x = 0, y = 0 }
-        local br = Geom:new{ x = Screen:getWidth() - 1, y = Screen:getHeight() - 1}
-        local screen_diag = tl:distance(br)
-        dec = ges.distance / math.min(screen_diag, self._image_wg:getCurrentDiagonal())
+        if ges.distance < self._image_wg:getCurrentDiagonal() then
+            self:onZoomToDiagonal(ges.distance)
+        else
+            local tl = Geom:new{ x = 0, y = 0 }
+            local br = Geom:new{ x = Screen:getWidth() - 1, y = Screen:getHeight() - 1}
+            local screen_diag = tl:distance(br)
+            self:onZoomOut(ges.distance / math.min(screen_diag, self._image_wg:getCurrentDiagonal()))
+        end
     end
-    self:onZoomOut(dec)
     return true
 end

@poire-z
Copy link
Contributor

poire-z commented Sep 19, 2022

zooming to the exact dimension that matches the distance between the final position of the two fingers. That possibly feels more natural

That's what I had it mind it should be, so, good luck ! :)
Or what I think you mean :) it's a bit hard to express in words, my above attempt at that was

or the width at start of gesture vs the width at end of gesture

@NiLuJe
Copy link
Member Author

NiLuJe commented Sep 19, 2022

FWIW, currently, distance is the sum of the travel of both fingers. I'm using the distance between both fingers at the end of the gesture instead. If you start or end from pinched (i.e., "kissing" ;p) fingers, the two roughly matches, but that's not always what the gesture looks like (as you can start/end with both fingers still apart, possibly quite far) ;).

(Fun fact: noticed that this also applies to two_finger_swipe, where it makes absolutely no sense (i.e., for the same actual distance, a two finger swipe will report roughly twice the distance of the same swipe with a single finger :s); but existing code already had to correct for it, so I just documented the quirk).

NiLuJe added a commit that referenced this pull request Sep 21, 2022
* ImageViewer: Minor code cleanups
* GestureDetector: Fix the `distance` field of `two_finger_pan` & `two_finger_swipe` gestures so that it's no longer the double of the actual distance traveled. Get rid of existing workarounds throughout the codebase that had to deal with this quirk.
NiLuJe added a commit to NiLuJe/koreader that referenced this pull request Sep 27, 2022
NiLuJe added a commit that referenced this pull request Sep 27, 2022
TranHHoang added a commit to TranHHoang/koreader that referenced this pull request Oct 23, 2022
KOReader 2022.10 "Muhara"

![koreader-2022-10](https://user-images.githubusercontent.com/202757/197379886-75c933df-8236-4be2-9287-304a88778b67.png)

We skipped last month's release because I was right in the middle of moving, which serendipitously coincided with fairly drastic changes that needed more time for testing, such as a big rewrite of gestures and multitouch (koreader#9463).

Users of the Dropbox plugin will now be able to use the new short-lived tokens (koreader#9496).

<img width="40%" alt="image" src="https://user-images.githubusercontent.com/59040746/193070490-a3d477db-bd82-431b-95fd-2c4765244378.png" align="right">One of the more visible additions is the new Chinese keyboard contributed by @weijiuqiao, based on the [stroke input method](https://en.wikipedia.org/wiki/Stroke_count_method) (koreader#9572). It's not smart and it requires knowledge of stroke order. A tutorial can be found [here](https://github.com/koreader/koreader/wiki/Chinese-keyboard), part of which I will reproduce below.

<hr>

The stroke input method groups character strokes into five categories. Then any character is typed by its stroke order.
| Key | Stroke type |
| ------ | ------ |
| `一` | Horizontal or rising stroke |
| `丨` | Vertical or vertical with hook |
| `丿` | Falling left |
| `丶` | Dot or falling right |
| `𠃋` | Turning |

For example, to input 大, keys `一丿丶` are used.

Note all turning strokes are input with a single `𠃋` key as long as they are written in one go. So 马 is input with `𠃋𠃋一`.

After getting the intended character, a `分隔`(Separate) or `空格`(Space) key should be used to finish the input. Otherwise, strokes of the next character will be appended to that of the current one thus changing the character.

Besides, the keyboard layout contains a wildcard key `*` to use in place of any uncertain stroke.

Swipe north on the `分隔`(Separate) key for quick deletion of unfinished strokes.

<hr>

Logo credit: @bubapet

We'd like to thank all contributors for their efforts. Some highlights since the previous release include:

* NewsDownloader: Strip byte order mark from xml string before parsing (koreader#9468) @ad1217
* GestureDetector: Full refactor for almost-sane(TM) MT gesture handling (koreader#9463) @NiLuJe
* Kobo: Unbreak touch input on fresh setups on Trilogy (koreader#9473) @NiLuJe
* Kobo: Fix input on Mk. 3 (i.e., Kobo Touch A/B). (koreader#9474, koreader#9481) @NiLuJe
* Kindle: Attempt to deal with sticky "waking up" hibernation banners (koreader#9491) @NiLuJe
* Add "Invert page turn buttons" to Dispatcher (koreader#9494) @NiLuJe
* [UIManager] Outsource device specific event handlers (koreader#9448) @zwim
* AutoWarmth: add a choice to control warmth and/or night mode (koreader#9504) @zwim
* Allow F5 key to reload document (koreader#9510) @poire-z
* bump crengine: better SVG support with extended LunaSVG (koreader#9510) @poire-z
* CRE/ImageViewer: get scaled blitbuffer when long-press on SVG (koreader#9510) @poire-z
* RenderImage: use crengine to render SVG image data (koreader#9510) @poire-z
* Wikipedia EPUBs: keep math SVG images (koreader#9510) @poire-z
* TextViewer: add Find (koreader#9507) @hius07
* A random assortment of fixes (koreader#9513) @NiLuJe
* Add Russian Wiktionary dictionary (koreader#9517) @Vuizur
* add custom mapping for tolino buttons (koreader#9509) @hasezoey
* Profiles: add QuickMenu (koreader#9526) @hius07
* ImageViewer: Clamp zoom factor to sane values (koreader#9529, koreader#9544) @NiLuJe
* ReaderDict: fix use of dicts with ifo with DOS line endings (koreader#9536) @poire-z
* Kobo: Initial Clara 2E support (koreader#9545) @NiLuJe
* TextViewer: add navigation buttons (koreader#9539) @hius07
* ConfigDialog: show button with default values in spinwidgets (koreader#9558) @hius07
* Misc: Get rid of the legacy defaults.lua globals (koreader#9546) @NiLuJe
* Misc: Use the ^ operator instead of math.pow (koreader#9550) @NiLuJe
* DocCache: Unbreak on !Linux platforms (koreader#9566) @NiLuJe
* Kobo: Clara 2E fixes (koreader#9559) @NiLuJe
* Keyboard: add Chinese stroke-based layout (koreader#9572, koreader#9582) @weijiuqiao
* Vocabulary builder: add Undo study status (koreader#9528, koreader#9582) @weijiuqiao
* Assorted bag'o tweaks & fixes (koreader#9569) @NiLuJe
* ReaderFont: add "Font-family fonts" submenu (koreader#9583) @poire-z
* FileManager: add Select button to the file long-press menu (koreader#9571) @hius07
* Dispatcher: Fixes, Sort & QuickMenu (koreader#9531) @yparitcher
* Cloud storage: add Dropbox short-lived tokens (koreader#9496) @hius07
* GH: Extend the issue template to request verbose debug logs for non-crash issues. (koreader#9585) @NiLuJe
* Logger: Use serpent instead of dump (koreader#9588) @NiLuJe
* LuaDefaults: Look for defaults.lua in $PWD first (koreader#9596) @NiLuJe
* UIManager: Don't lose track of the original rotation on reboot/poweroff (koreader#9606) @NiLuJe
* ReaderStatus: save status summary immediately on change (koreader#9619) @hius07
* [feat] Add Thai keyboard (koreader#9620) @weijiuqiao
* Dispatcher: Fix subtle bug with modified items being added twice to the sort index (koreader#9628) @yparitcher
* Vocabulary builder: supports review in reverse order (koreader#9605) @weijiuqiao
* Exporter plugin: allow adding book md5 checksum when exporting highlights (koreader#9610) @sp4ke
* buttondialogtitle: align upper borders (koreader#9631) @hius07
* Kobo: Always use open/write/close for sysfs writes (koreader#9635) @NiLuJe
* OPDS-PS: Fix hardcoded namespace in count (koreader#9650) @bigdale123

[Full changelog](koreader/koreader@v2022.08...v2022.10) — [closed milestone issues](https://github.com/koreader/koreader/milestone/59?closed=1)

---

Installation instructions: [Android](https://github.com/koreader/koreader/wiki/Installation-on-Android-devices) • [Cervantes](https://github.com/koreader/koreader/wiki/Installation-on-BQ-devices) • [ChromeOS](https://github.com/koreader/koreader/wiki/Installation-on-Chromebook-devices) • [Kindle](https://github.com/koreader/koreader/wiki/Installation-on-Kindle-devices) • [Kobo](https://github.com/koreader/koreader/wiki/Installation-on-Kobo-devices) • [PocketBook](https://github.com/koreader/koreader/wiki/Installation-on-PocketBook-devices) • [ReMarkable](https://github.com/koreader/koreader/wiki/Installation-on-ReMarkable) • [Desktop Linux](https://github.com/koreader/koreader/wiki/Installation-on-desktop-linux) • [MacOS](https://github.com/koreader/koreader/wiki/Installation-on-MacOS)
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.

None yet

3 participants