bug#69992: Minor improvement to image map transformation logic

[David Ponce]

Re-sent to all (sorry)

On 27/03/2024 23:17, Joseph Turner wrote:
[...]
> On my machine, not all tests pass with the patch.  Please be sure that
> these three new tests pass:
>
> image-create-image-with-map
> image--compute-map-and-original-map
> image-transform-map
>
> Personally, I find it easier to understand image map transformation when
> the logic is split into multiple functions.  However, the benefit of
> readability could certainly be outweighed by a noticeable improvement to
> user experience.  Please share some benchmarks.
>
> Please keep in mind that `image--delayed-change-size' already debounces
> image transformation, so this code may not be so performance-critical.
  
Hello,

After more work, testing and benchmarks, I can finally confirm that my
proposed version of `image--compute-*map' without the logic splits
into multiple functions is not significantly faster than the current
version with the logic splits into multiple functions :-)

What I found interesting after profiling both current and proposed
functions is that most of the time is consumed by the call to
`image-size'!

I also found that the current implementation is not correct when
rotation is not a multiple of 90 degrees.  In this case, Emacs still
scales the image if specified, but ignores rotation and flipping.
However, in this case, the `image--compute-*map' functions do not
recompute map.

The attached new patch fixes the logic to be consistent with Emacs
internal implementation, plus some other tweaks to check if a
transformation apply before to call the transformation function.
I also updated some tests according to functions changes.
Here is a possible change log:

2024-03-28  David Ponce  <da_vid@orange.fr>

        * lisp/image.el (image--compute-scaling)
        (image--compute-rotation): New functions.
        (image--compute-map, image--compute-original-map): Use them.
        Ensure all transformations are applied or undone according to what
        Emacs does internally.  Call a transformation function only when
        needed.  Fix doc string.
        (image--scale-map): Assume effective scale argument.
        (image--rotate-map): Assume effective rotation argument.
        (image--rotate-coord): Improve doc string.
        (image--flip-map): Remove no more used flip argument.

        * test/lisp/image-tests.el (image-create-image-with-map): Use a
        valid SVG image otherwise `image-size' will not return a valid
        value and calculation of scale could fail.
        (image-transform-map): Update according to changed signature of
        image--flip-map.

This new version passes the `image-create-image-with-map' and
`image-transform-map' tests.  But on my laptop, the
`image--compute-map-and-original-map' fails the same for both the
current and proposed version of the functions:

F image--compute-map-and-original-map
     Test ‘image--compute-map’ and ‘image--compute-original-map’.
     (ert-test-failed
      ((should
        (image-tests--map-equal (image--compute-map image) rotated-map))
       :form
       (image-tests--map-equal
        (((circle ... . 24) "a" (help-echo "A"))
         ((rect ... 127 . 77) "b" (help-echo "B"))
         ((poly . [199 161 206 160 213 154 218 146 221 136 ...]) "c"
          (help-echo "C")))
        (((circle ... . 24) "a" (help-echo "A"))
         ((rect ... 54 . 77) "b" (help-echo "B"))
         ((poly . [126 161 133 160 140 154 145 146 148 136 ...]) "c"
          (help-echo "C"))))
       :value nil))

Thanks!

image-compute-map-V0.patch
Description: Text Data