Conversation
removal of sharp to use jimp is done Completely removed usage of sharp to rely on jimp with uses no native modules. rebase changes
src/types/jimp.ts
Outdated
| import { Jimp } from "jimp"; | ||
|
|
||
| // NOTE: jimp docs say the read return type JIMP but ts says otherwise. They haven't fully ironed out their types, once they do we can remove this | ||
| export type JimpImg = Awaited<ReturnType<typeof Jimp.read>>; |
There was a problem hiding this comment.
Curious to hear what folks this on this. Might be anti pattern, but i'm not sure.
There was a problem hiding this comment.
Lmao I love monkey patching. If it works it works I guess...
There was a problem hiding this comment.
Lmao I love monkey patching. If it works it works I guess...
I removed this in favor of a hopefully okay hard cast inside utils/image.ts
There was a problem hiding this comment.
I saw a couple places that used the same types, hence I thought putting it into it's own type file was better. Let me know if any objections.
src/utils/image.ts
Outdated
| const img = await Jimp.read(src); | ||
| return img; | ||
| } catch (error) { | ||
| // TODO: LEO: wondering if it's possible to intgrate logger from instagram-cli into here |
There was a problem hiding this comment.
Form what i've seen there's no logger implementation in this codebase. let me know if i'm missed somewhting. Otherise I think this could be something good to add just like in instagram-cli
There was a problem hiding this comment.
Nope there's no logging. I think it'd be nice to have a onError prop you can pass to <Image> so the logging can be easily integrated with projects downstream. Lmk what you think
| (currentProtocol: ImageProtocolName): ImageProtocolName => { | ||
| switch (currentProtocol) { | ||
| case "kitty": | ||
| return "iterm2"; |
There was a problem hiding this comment.
not sure why iterm2 was hardcoded here, but i changed it to kitty since we already implemntation for it. Let me know if I'm stepping on anyones toes here.
There was a problem hiding this comment.
This function is supposed to get the next best protocol when the current protocol isn't supported, so kitty --> iterm
endernoke
left a comment
endernoke
left a comment
There was a problem hiding this comment.
Thanks for the work 😄
When I test it the colors are garbled, I think it's an issue in the to<ProtocolName> functions:
You could reference a working implementation of reading individual pixel colors from jimp here:
https://github.com/sindresorhus/terminal-image/blob/a2d012f48748d2ef01e4e927125eb4dfc18071c9/index.js#L73
src/components/image/ITerm2.tsx
Outdated
| "\x1b]1337;File=" + | ||
| `size=${info.size};` + | ||
| // optional, only used for showing progress | ||
| // `size=${size};` + |
There was a problem hiding this comment.
size is actually needed here despite the iTerm docs because xterm.js (and perhaps some other terminal emulators) need it, see also xtermjs/xterm.js#5326
src/index.ts
Outdated
| // export { default as BrailleImage } from "./components/image/Braille.js"; | ||
| // export { default as HalfBlockImage } from "./components/image/HalfBlock.js"; | ||
| // export { default as SixelImage } from "./components/image/Sixel.js"; | ||
| // |
There was a problem hiding this comment.
I've realized the old exports are unnecessary 😂 Could you just delete these lines thanks
src/components/image/Ascii.tsx
Outdated
| .raw() | ||
| .toBuffer({ resolveWithObject: true }); | ||
| // resized in place | ||
| image.scaleToFit({ w: width, h: height }); |
There was a problem hiding this comment.
Why did you use scaleToFit here but resize elsewhere?
There was a problem hiding this comment.
Why did you use
scaleToFithere butresizeelsewhere?
Had to sanity check. I was trying match one to one from prev implementation, but it makes more sense to use scaleToFit for all to avoid distortion. It's changed now.
@endernoke Half block is fixed with my recent commit. I still need to figure out out why ascii and braille is acting out. Thanks again for the callout, will update ya'll. |
in rgba the maximum instenstiy across all channels is 255, hence we had to normalize against that by dividing by 255
Fixed resizing and fitting of images.
2 participants
No description provided.