Skip to content
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

Two SVG bugs #114

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Two SVG bugs #114

wants to merge 2 commits into from

Conversation

srush
Copy link
Collaborator

@srush srush commented Oct 4, 2022

  • 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.

@danoneata
Copy link
Collaborator

danoneata commented Oct 6, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator Author

srush commented Oct 6, 2022

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
Copy link
Collaborator

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
Copy link
Collaborator Author

srush commented Oct 6, 2022

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
Copy link
Collaborator

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants