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

Unexpected changes #14

Open
abu-irrational opened this issue Jul 19, 2024 · 1 comment
Open

Unexpected changes #14

abu-irrational opened this issue Jul 19, 2024 · 1 comment

Comments

@abu-irrational
Copy link

Hi Wiliam,

I noticed you recently made two changes that broke compatibility with my integration.

The first one: seconds().

This function is not defined among all those in svg/.h (it's defined in app/).
Not bad, I can always put
#define seconds() 0
since I'm not interested in timing..

The second change is about the SVGDocument constructor.

It was before
SVGDocument(FontHandler *fh)
now it is
SVGDocument(FontHandler *fh, const double w, const double h, const double ppi = 96)

Honestly, I don't understand why it's required to specify the size of the canvas when loading/analyzing the SVG file.
From my point of view, the sizing of the canvas concerns the rendering phase, i.e. when I initialize an SVGDrawingContext by passing it
an appropriately sized BLImage.
What's more, only after loading an SVG file, I can know the image's dimensions (or rather the height/width ratios) to size the canvas (or set up a scale transformation).

Maybe I didn't understand correctly, but I think that canvaHeight,width should not be part of SVFDocument.

@Wiladams
Copy link
Owner

seconds() made a brief appearance for debugging. I'll turn that back off.

I've gone back and forth on the SVGDocument including or not including the canvas size. You're right in that the document does not need this information until it is time to render.

If you look at the SVGDocument class, you'll see that it inherits from "IAmGroot". This facilitates being able to render, because it contains environment information, like the Window/canvas width and height, and dpi.

This information is only needed when you actually render, but it's needed for two things.

  1. Size of the canvas. We need this so we can resolve cases where "%" is used for a size, instead of absolute units. We also need the 'dpi' for the same reason. When units, such as 'in', 'mm', 'cm' are used, we need to know the dpi so we can resolve to actual number of pixels.
  2. Looking up objects. There are many places that might be holding a reference to an object, such as the 'use' element, or gradients referring to a template gradient, or colors, referring to a gradient, etc. at 'bind' time, we need to know where we can lookup all those values. They are tied to the "IAmGroot" interface.

At the moment, 'bindToGroot()' is called within the 'addNode()' function of the document. Therefore, the SVGDocument already needs to know this information.

Well, it doesn't have to be like this at all. As long as when the document is being constructed, all those nodes are held in some form where they can be looked up later, this binding can happen at any time, such as when you're drawing into a context.

In that case, it make more sense to change the SVGDocument::draw() function to be more like

void draw(IRenderSVG * ctx, IAmGroot *groot) override

That way, you don't need to bind early, and you won't need to pass the width and height or dpi earlier.

The same can go for document construction. There's no reason SVGDocument needs to inherit from IAMGroot. As long as a pointer to such an object is included in the "loadFromXmlIterator" call, we can completely remove this code to a separate object, and the SVGDocument itself becomes even simpler.

Those are my thoughts 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

No branches or pull requests

2 participants