repl: Render SVG cell outputs - #62889
Conversation
|
@MrSubidubi sorry about the ping, just wanted to ensure this doesn't get buried and outdated like the previous version ^^ |
| pub fn from_svg(svg: &str, cx: &App) -> Result<Self> { | ||
| // SVG is vector text rather than a raster format, so it goes through | ||
| // GPUI's SVG renderer instead of the `image` crate used above. | ||
| let image = Image::from_bytes(ImageFormat::Svg, svg.as_bytes().to_vec()); | ||
| let render_image = image.to_image_data(cx.svg_renderer())?; | ||
|
|
||
| // The SVG renderer rasterizes at SMOOTH_SVG_SCALE_FACTOR for crispness, | ||
| // so the pixel dimensions are scaled up from the logical size we want | ||
| // to lay out at. | ||
| let size = render_image.size(0); | ||
| let width = (size.width.0.max(0) as f32 / SMOOTH_SVG_SCALE_FACTOR).round() as u32; | ||
| let height = (size.height.0.max(0) as f32 / SMOOTH_SVG_SCALE_FACTOR).round() as u32; | ||
|
|
||
| Ok(ImageView { | ||
| clipboard_image: Arc::new(image), | ||
| height, | ||
| width, | ||
| image: render_image, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Why can we not parse the SVG directly through the SVG renderer, then render it at the requested size?
There was a problem hiding this comment.
That's doable.
from_svg now only parses and keeps the ParsedSvg. The raster is produced in render at the box the output is actually laid out in, times window.scale_factor(), via SvgSize::ExactSize, so it is 1:1 with physical pixels. It caches on the target device size and skips the work when that has not changed, and drops the superseded texture with cx.drop_image. This follows what mermaid.rs already does, which I had missed. SMOOTH_SVG_SCALE_FACTOR is gone from the repl crate, so no more undoing an internal detail of the renderer.
One thing to flag: ParsedSvg had no way to read the document's intrinsic size, which is needed to lay out before the first rasterization. I added a small size() accessor on it, so the diff now touches gpui as well. Happy to move it somewhere else if you would rather it not live there.
Re-rasterization is rare in practice, since max_width_for_columns depends on font metrics and the max_columns setting rather than window width, so a resize drag does not trigger it. It is synchronous in render. If you would prefer it on a background task with a fallback like mermaid, I can do that.
Tests cover the intrinsic size and that a 60x40 box at scale 2.0 gives a 120x80 raster, that the same size reuses it, and that a different size re-rasterizes.
dbb5da6 to
bb49792
Compare
MrSubidubi
left a comment
There was a problem hiding this comment.
Some more comments, if you feel like it's too much to ask, feel free to raise 🙂 However, think after we should probably be good
| /// The intrinsic size of the document, in logical pixels. | ||
| pub fn size(&self) -> Size<Pixels> { | ||
| let size = self.0.size(); | ||
| gpui_size(px(size.width()), px(size.height())) |
There was a problem hiding this comment.
Let's use something like crate::size here instead of renaming something to gpui_ in the GPUI crate
There was a problem hiding this comment.
Done, uses crate::size now.
| .parse_svg(&bytes) | ||
| .map_err(|error| anyhow::anyhow!("{error}"))?; |
There was a problem hiding this comment.
Can we not just
| .parse_svg(&bytes) | |
| .map_err(|error| anyhow::anyhow!("{error}"))?; | |
| .parse_svg(&bytes)?; |
There was a problem hiding this comment.
Parsing moved to a background task, so this is gone. The error is now kept in the view and shown in the UI.
| image: Option<Arc<RenderImage>>, | ||
| /// Set for SVG outputs, which are rasterized on demand at the size they are | ||
| /// laid out at rather than at a fixed size. | ||
| svg: Option<Arc<ParsedSvg>>, | ||
| rasterized_size: Option<Size<DevicePixels>>, |
There was a problem hiding this comment.
This now introduces some fields that are not needed for one another and adds mental overhead IMO.
How would you feel if we were to throw this into an enum ImageSource (I am terrible with names, feel free to give that a different name) and then move SVG and rasterized size into that? Then we do not need the comment as the enum would properly describe this. The enum would then probably need the required methods and/or some moved
There was a problem hiding this comment.
Done. There is now an ImageSource enum with a Raster variant and an Svg variant, and the SVG one is a small state enum of Parsing, Ready and Failed. The raster and the in flight render size live inside Ready, so those fields no longer exist when they make no sense. The parse task is still a field on the view rather than inside the enum, because the task replaces the state when it finishes and it would otherwise be dropping itself.
| let bytes = svg.as_bytes().to_vec(); | ||
| let parsed = cx | ||
| .svg_renderer() | ||
| .parse_svg(&bytes) |
There was a problem hiding this comment.
We do as_bytes then to_vec, but can we not just pass them borrowed?
There was a problem hiding this comment.
Right. The Vec is only made once for the clipboard image now, and the parse borrows the bytes from it.
| height: f32::from(intrinsic_size.height).round() as u32, | ||
| width: f32::from(intrinsic_size.width).round() as u32, |
There was a problem hiding this comment.
While we are at it, let's move this into size field please, then we can also just call .map on the size in one go
There was a problem hiding this comment.
Done, single size field, and the intrinsic size for SVG comes from ParsedSvg::size.
| @@ -123,9 +181,12 @@ impl Render for ImageView { | |||
|
|
|||
| let (height, width) = self.scaled_size(line_height, max_width, max_height); | |||
There was a problem hiding this comment.
Can we make scaled_size also return a size?
There was a problem hiding this comment.
Done, returns Option<Size>. It is None for an SVG that has not parsed yet.
| let target = size( | ||
| DevicePixels((f32::from(width) * scale_factor).round() as i32), | ||
| DevicePixels((f32::from(height) * scale_factor).round() as i32), | ||
| ); |
There was a problem hiding this comment.
Given https://github.com/zed-industries/zed/pull/62889/changes#r3903586667, we should then also be able to just map this here
There was a problem hiding this comment.
Done, it is a map over the size now.
| match cx | ||
| .svg_renderer() | ||
| .render_parsed(&svg, SvgSize::ExactSize(target)) | ||
| { | ||
| Ok(image) => { | ||
| self.rasterized_size = Some(target); | ||
| if let Some(previous) = self.image.replace(image) { | ||
| cx.drop_image(previous, None); | ||
| } | ||
| } | ||
| Err(error) => log::error!("failed to rasterize SVG output: {error}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
Here and at the parsing we have the probably biggest issues with this currently:
- Parsing currently happens on the main thread
- We have no error/loading state shown in the UI.
Especially the first one could lead to a hang, which I'd definitely not want (and we should maybe even consider limiting the render width to something like 2000 pixels).
Thus, can we move both to a background task, then update this? We could here give this like 2ms to parse this using select_biased, then if the task is not complete, move on with showing the loading state instead and then continue polling in the background.
Similarly, I'd like us to show a failed state with the error reason even perhaps if the SVG can either not be parsed or rendered, since that would be relevant to the user
There was a problem hiding this comment.
Both are on background tasks now.
Parsing is spawned when the view is created. Following your suggestion it races a 2ms timer, so a small SVG goes straight to the image and only a slow one shows a loading state. Rasterizing is also spawned, keyed on the target device size, so it does not respawn every frame and the previous raster stays on screen while a new one is made.
Parse and render failures are both kept in the view and shown as "Failed to render SVG: reason" instead of being logged.
Also capped the raster at 2000 device pixels on the longest side, scaled down proportionally, so a large document cannot ask for a huge texture.
SVG outputs (image/svg+xml) previously fell through to "Unsupported media type". Render them through GPUI's SVG renderer, reusing the existing ImageView via a from_svg constructor, and rank SVG above raster images so a kernel that offers both shows the scalable version.
bb49792 to
6f543ff
Compare
|
@MrSubidubi all eight addressed and pushed, replies are on each thread. repl tests, clippy and fmt pass locally. |
Closes #61717
Closes #60203
Reopened from #61718, which could not be reopened after being closed. Also supersedes #44512, which was closed as abandoned.
SVG outputs from kernels (image/svg+xml) were showing "Unsupported media type" instead of the image. matplotlib, plotly, and other libraries commonly emit SVG, so this was a visible gap for data science users. This came up in discussion #25936.
This renders SVG outputs through GPUI's SVG renderer, reusing the existing ImageView with a from_svg constructor. It also ranks SVG above raster images, so a kernel that offers both shows the scalable version. If you would rather keep PNG first, that is a one line change and I am happy to flip it.
The old jupyter_protocol issue that blocked #44512 (image/svg+xml being parsed as a generic image and dropped) is fixed in the version Zed uses now, so no cargo patch is needed.
I added a unit test that checks an SVG renders at its intrinsic size. This caught a scale factor bug where SVGs were rendering at twice their size. The two existing rank_mime_type tests are updated for the new ordering. Tested locally on macOS, and a cell producing image/svg+xml now renders the vector image inline.
Release Notes: