-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Handle vectors #187
Handle vectors #187
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool. This is much less code than I anticipated.
Hold on this, GitHub only showed me a subset of changes lol |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, one nit.
src/codecs/resize/resize.ts
Outdated
const endAspect = dw / dh; | ||
|
||
if (endAspect > currentAspect) { | ||
const newSh = dh / (dw / sw); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think with some shuffling, we can make the math a bit more “obvious”:
dh / (dw/sw) = sw * dh / dw = sw/endAspect
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what's going on there, but it's definitely an invalid left-hand assignment. What am I missing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ohhh you mean change to sw / endAspect
! I see now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lol sorry. I am suggesting to use the expression on the very right and showing how I got there. This is not supposed to be JavaScript, just math :D My bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... GitHub really needs to fix their stupid code-review UI to update when someone comments.
src/codecs/resize/resize.ts
Outdated
return { sw, sh: newSh, sx: 0, sy: newSy }; | ||
} | ||
|
||
const newSw = dw / (dh / sh); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same trick here:
dw / (dh / sh) = sh * dw / dh = sh * endAspect
Ahh, so the idea would be that the SVG stays sharp when scaled, but the compressed version is pixels? I can take a swing at that. |
Yeah, I guess we should prevent rasterizing at every zoom interaction, but maybe we can rasterize at 200% once zoom is above 100%, rasterize at 400% once zoom is above 200% or something along those lines? |
I think we get it for free if I add the |
Happy to leave this as a stretch goal. (aka punt to a separate, future PR) |
Filed #190 for the sharpness thing. I agree it's worth doing. |
I also made the resizer vector-aware, so you can resize the image larger & stay sharp.
52c9438
to
28c6c61
Compare
* Allow loading SVG. Fixes GoogleChromeLabs#138. I also made the resizer vector-aware, so you can resize the image larger & stay sharp. * Handling SVG without width/height set. * Simplifying maths * Doh, case sensitive
DO NOT MERGE but please review (not based on master)
No longer fails when loading SVG.
Also, I made the resize preprocessor vector-aware, so it'll do the resizing in SVG if the source is SVG.