Skip to content

Conversation

@maxdexh
Copy link
Collaborator

@maxdexh maxdexh commented Nov 27, 2025

output.mp4

@maxdexh
Copy link
Collaborator Author

maxdexh commented Nov 27, 2025

In release mode, the overflow causes the image to be moved right by 32K units

@maxdexh
Copy link
Collaborator Author

maxdexh commented Nov 27, 2025

After the fix, zooming does nothing on wide pdfs, but that's to be expected with the way zooming works right now. Zooming out of a wide pdf would require decreasing the height while keeping the width unchanged, which is something the code cannot do right now.

@maxdexh
Copy link
Collaborator Author

maxdexh commented Nov 27, 2025

I would like to try rewriting the way zoom is represented and handled from scratch if that is okay with you. I'm open to discussion about how this could look.

@itsjunetime
Copy link
Owner

Oh, yeah - I never tested this with horizontal pdfs. I definitely need to include more of those in the tests so this doesn't happen again.

I kinda stopped working on zooming for a bit 'cause I would just get more confused each time I revisited it and tried to understand how all the moving parts and scaling factors and offsets work together. If you would like to rewrite the way it works, I would love it; my brain's just kinda fried on that front right now.

I also don't fully understand how the exact calculations in this PR work, and tbh I'm not seeing the same panic on my end, but it seems simple enough and if it fixes something for you then it's probably fine.

Also thanks so much for contributing, simple polish PRs really mean a lot :)

@itsjunetime itsjunetime merged commit 3dc5135 into itsjunetime:main Nov 27, 2025
1 check passed
@maxdexh
Copy link
Collaborator Author

maxdexh commented Nov 27, 2025

The cause of the overflow is the smaller_width.max(ratio * height) being able to exceed old_width, so the PR just clams down the arg of max to never exceed old_width. Sorry, I thought it was clear from the video what was going on.

- on unsigned integers is kind of a footgun ^^

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.

2 participants