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

feat: multiscale #17

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,4 @@ For example, if you want to convert DMZ-White to Windows:
are using unconventional distros (e.g. Alpine) and are getting errors related
to `wand`, please see the [Wand documentation on installation][wand-install].

[wand-install]: https://docs.wand-py.org/en/0.6.7/guide/install.html
[wand-install]: https://docs.wand-py.org/en/0.6.7/guide/install.html
4 changes: 4 additions & 0 deletions win2xcur/cursor.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ def __init__(self, image: SingleImage, hotspot: Tuple[int, int], nominal: int) -
def __repr__(self) -> str:
return f'CursorImage(image={self.image!r}, hotspot={self.hotspot!r}, nominal={self.nominal!r})'

def scale(self, width: int, height: int) -> None:
self.image.scale(width, height)
self.nominal = max(width, height)
Copy link
Owner

Choose a reason for hiding this comment

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

This makes sense when generating new sizes, but I am not yet convinced it actually makes sense in general.

The nominal size is a special field in the Xcursor format that helps in selecting the image to use, but the actual image can be any size you want. There are weird situations with cursors that have a lot of padding around the actual image. In those cases, the outcome might not be desirable, which is why scaling was originally introduced. If we adjust the nominal size, then on the Linux side, a different cursor sprite would be selected, resulting in identical visual outcomes.



class CursorFrame:
images: List[CursorImage]
Expand Down
10 changes: 8 additions & 2 deletions win2xcur/main/win2xcur.py
Copy link
Owner

Choose a reason for hiding this comment

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

Is this supposed to be a win2xcur feature only?

Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
from threading import Lock
from typing import BinaryIO

from win2xcur import scale, shadow
from win2xcur import scale, shadow, multiscale
from win2xcur.parser import open_blob
from win2xcur.writer import to_x11

Expand All @@ -34,6 +34,10 @@ def main() -> None:
help='color of the shadow')
parser.add_argument('--scale', default=None, type=float,
help='Scale the cursor by the specified factor.')
parser.add_argument('--multiscale', action='store_true',
help='Generate multiple sizes for each cursor.')
parser.add_argument('--multiscale-min', type=int, default=12,
Copy link
Owner

Choose a reason for hiding this comment

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

Why is this default 12 when the array later is starts at 16?

help='Minimum size to generate.')

args = parser.parse_args()
print_lock = Lock()
Expand All @@ -48,7 +52,9 @@ def process(file: BinaryIO) -> None:
print(f'Error occurred while processing {name}:', file=sys.stderr)
traceback.print_exc()
else:
if args.scale:
if args.multiscale:
multiscale.generates_frames(cursor=cursor, min_size=args.multiscale_min)
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
multiscale.generates_frames(cursor=cursor, min_size=args.multiscale_min)
multiscale.generates_frames(cursor, min_size=args.multiscale_min)

elif args.scale:
scale.apply_to_frames(cursor.frames, scale=args.scale)
if args.shadow:
shadow.apply_to_frames(cursor.frames, color=args.shadow_color, radius=args.shadow_radius,
Expand Down
32 changes: 32 additions & 0 deletions win2xcur/multiscale.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
from win2xcur.cursor import CursorFrame, CursorImage

MULTSCALE = [16, 24, 32, 48, 64, 96, 128, 192, 256]

def generates_frames(cursor, min_size: int) -> None:
"""Generates multiple sizes for each cursor.

Args:
cursor (Cursor): The cursor to generate sizes for.
min_size (int): The minimum size to generate.

Returns:
List[Cursor]: The generated cursors.
Copy link
Owner

Choose a reason for hiding this comment

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

This directly contradicts the -> None. In general, I don't like declaring signatures in comments when you can explicitly declare them in the signature. DRY.

"""
frames = cursor.frames
new_frames = []
image_size = frames[0].images[0].nominal
Copy link
Owner

Choose a reason for hiding this comment

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

Shouldn't you pick the biggest image in the frame if you are resizing it? I am pretty sure the code doesn't sort it from biggest to smallest.

for size in MULTSCALE:
if size > image_size:
continue
if size < min_size:
break
for frame in frames:
new_images = []
for cur in frame:
new_cur = CursorImage(cur.image.clone(), cur.hotspot, cur.nominal)
new_cur.scale(size, size)
Copy link
Owner

Choose a reason for hiding this comment

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

What if the image is rectangular? Also why is every image in the frame scaled to the same size?

new_images.append(new_cur)
new_frame = CursorFrame(new_images, frame.delay)
Copy link
Owner

Choose a reason for hiding this comment

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

Why are you looping over all the sizes and repeat each frame for each size? I see why you run into the nominal size problem now. If the nominal size is different in different frames, then Xcursor treats them as separate sizes and not animations. This is a sign of a massive bug in your implementation—the number of frames should be constant despite all your image manipulation.

new_frames.append(new_frame)
del cursor.frames[:]
cursor.frames.extend(new_frames)
Comment on lines +31 to +32
Copy link
Owner

Choose a reason for hiding this comment

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

Inefficient, try:

Suggested change
del cursor.frames[:]
cursor.frames.extend(new_frames)
cursor.frames[:] = new_frames

2 changes: 1 addition & 1 deletion win2xcur/scale.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
def apply_to_frames(frames: List[CursorFrame], *, scale: float) -> None:
for frame in frames:
for cursor in frame:
cursor.image.scale(
cursor.scale(
int(round(cursor.image.width * scale)),
int(round(cursor.image.height) * scale),
)