chalk icon indicating copy to clipboard operation
chalk copied to clipboard

Two SVG bugs

Open srush opened this issue 3 years ago • 6 comments

  • Newlines \n in svg specification seems to break in some browsers.
  • Images that are too large and then scaled down cause Affine to think transformation are degenerate.

Small temporary fixes.

srush avatar Oct 04 '22 18:10 srush

Hey! Thanks for the fixes!

The image fix works only for the SVG backend, right? I wonder if it would be more flexible (backend agnostic) to apply the scaling only when we create the image primitive:

 def image(local_path: str, url_path: Optional[str]) -> Diagram:
     from chalk.core import Primitive

-    return Primitive.from_shape(Image(local_path, url_path))
+    return Primitive.from_shape(Image(local_path, url_path)).scale(0.05)

Somewhat unrelated, but what should the url_path argument from the image function be set to (I see that's used only in the SVG backend)?

danoneata avatar Oct 06 '22 14:10 danoneata

Ah, I now see that things are a bit more complicated that I've imagined: if I understand correctly, the SVG backend doesn't display the image loaded from the local_path, but the one specified by url_path? The image from the local_path is used to retrieve the size?

danoneata avatar Oct 06 '22 14:10 danoneata

I think we need a couple long term fixes.

  1. figure out why large scaling ops break the degenerate check in affine.
  2. make image backend specific. Diagrams also has some backend specific code (svgtext)
  3. pull out and document scaling constants. I.e default images to local width 1? Not sure what is right to do here.

srush avatar Oct 06 '22 15:10 srush

Okay! So I guess you are suggesting to merge these temporary fixes for the time being and create separate issues for the long-term problems, right? I'm fine with that, but I just wanted to point out that the current changes will affect the image rendering with cairo (since the Image's width and height do not match the true width and height of the PIL image data); I don't have a satisfactory solution—maybe we could try resizing the image by the 0.05 factor in the from_pil function?

     format: cairo.Format = cairo.FORMAT_ARGB32
+    im = im.resize((int(im.width * 0.05), int(im.height * 0.05)))
     if "A" not in im.getbands():

danoneata avatar Oct 06 '22 21:10 danoneata

We don't need to merge. I'll fix Cairo for real here.

Mostly just sent this to remind myself since I needed it in my code.

srush avatar Oct 06 '22 21:10 srush

Ah, cool! Sorry for misunderstanding! I'll take a look at #113 then if you haven't started already working on it.

danoneata avatar Oct 07 '22 12:10 danoneata