search

chalk-diagrams / chalk

A declarative drawing API in Python
MIT License
282 stars 13 forks source link

Two SVG bugs #114

Open srush opened 2 years ago

srush commented 2 years ago

Small temporary fixes.

danoneata commented 2 years ago

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 commented 2 years ago

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?

srush commented 2 years ago

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.

danoneata commented 2 years ago

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():
srush commented 2 years ago

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.

danoneata commented 2 years ago

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