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

Boundless recording surfaces do not currently work with svgs #150

Closed
hustf opened this issue Mar 23, 2021 · 38 comments
Closed

Boundless recording surfaces do not currently work with svgs #150

hustf opened this issue Mar 23, 2021 · 38 comments

Comments

@hustf
Copy link
Contributor

hustf commented Mar 23, 2021

This is included as a comment in /test/test-snapshot.jl

# It seems that the svg device called by Cairo fails to find
# a size reference from unbounded recording surfaces. Botched up svgs
# include "-1.#QNAN", or very long strings. This occurs with and
# without 'Pro api' text on the drawing.
# However, it seems that if an svg has previously been rendered from a surface
# with bounds, the svg renderer works normally with unbounded surfaces.

If somehow Cairo.jl could be provided with the size reference it seems to need for svg drawings, a boundless recording surface could be provided by default. This could be quite useful for scripting if combined with get-extents. Scripting users could get instant results displayed without boilerplate.

@stale
Copy link

stale bot commented Mar 31, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale stale/inactive/superseded label Mar 31, 2021
@hustf
Copy link
Contributor Author

hustf commented Apr 6, 2021

Keeping this alive...

With Luxor release 2.11.0, we have recording surfaces and snapshots. There's now more motivation to start the groundwork for boundless surfaces in Cairo.

I'm dreaming of using Luxor as an engineering sketch tool, where you naturally work along a roll of paper which extends as your argument grows down the page. Or to the side, if you work that way. And with gridded paper, if you're that kind of person.

For short stubs, Luxor already works great for this. But if we put in the necessary pull request to Cairo.jl, Luxor can form the core part of such a tool, along with Latexify, Plots, and a zoom / move around viewer like MiniFB. The live viewer could be supported by an async loop.

@stale stale bot removed the stale stale/inactive/superseded label Apr 6, 2021
@cormullion
Copy link
Member

Cool! I'll add a label so that this stays open.

@oheil
Copy link
Contributor

oheil commented Aug 11, 2022

Is there an example for what this is about? For me it's not clear what is meant with "boundless surfaces". What is here to be expected to work, what doesn't work? Any code examples?

@hustf
Copy link
Contributor Author

hustf commented Aug 12, 2022

Just to let you know I saw the comment, and will provide an example at first opportunity. I also need a little time to remember the details here.

@hustf
Copy link
Contributor Author

hustf commented Aug 19, 2022

I made an example repo with the kind of scripting workflow I'm describing. issue150.jl contains
some functions documented with examples.

include("issue150.jl")

Paste from ?snap:

    Drawing(NaN, NaN, :rec)
    reset_INK_EXTENTS() # Not necessary in a fresh session
    background("deepskyblue2")
    setcolor("gold2")
    circle(O, 100, :fill) |> encompass
    setcolor("white")
    setfont("Sans", 100)
    settext("☼", O + (-34, 50); markup=true)
    snap("The sun")
    setline(10)
    ellipse(O, O + (550, 150), 1200, :stroke) |> encompass
    snap("A planetary orbit")
    setline(20)
    ellipse(O, O + (-550, 5500), 7000, :stroke) |> encompass
    snap() do
        setcolor("black")
        circle(O, 3, :fill)
        setcolor("darkgreen")
        settext("Comet's orbit", O + (-100, -120))
        settext("Origin of \nthis overlay", O; markup=true, valign="bottom")
    end

Each snap outputs a .png (works well) and an .svg (not so well) file (and shows the png in editor).

I'm including the last two here:

3
3

@hustf
Copy link
Contributor Author

hustf commented Aug 20, 2022

As per oheil's questions: A boundless recording surface is created with Drawing(NaN, NaN, :rec).

If the pull request linked above is merged into Cairo.jl, we won't need to keep track of where we are drawing on the boundless surface. In the example above, we keep track of extents with encompass(..).

The linked example works thanks to the amazing thread safety PRs from oheil.

@oheil
Copy link
Contributor

oheil commented Aug 22, 2022

Thanks for these examples and explanations, I will have a look when I have some spare time...

@oheil
Copy link
Contributor

oheil commented Sep 3, 2022

I checked and think part of it is a original Cairo problem. For SVG setting a background color is realized as a <rect ...>. It seems that Cairo ignores the cropping box coordinates for the background <rect..>. See:

using Luxor, Colors
d=Drawing(NaN, NaN, :rec)
background("deepskyblue2")
snapshot(;fname="test.svg",cb=BoundingBox(Point(-50,-50),Point(300,300)))

results in:

<svg ... viewBox="0 0 350 350" ... >
...
<rect x="0" y="0" width="16777215" height="16777215" .../>
...
<use ... transform="matrix(1,0,0,1,50,50)"/>

The transformation results in an effective <rect ...> of:

<rect x="50" y="50" width="16777215+50" height="16777215+50" .../>

In the example Cairo should create a svg <rect ...> of:

<rect x="-50" y="-50" width="16777215" height="16777215" .../>

Setting width and height to these arbitrary large values is questionable too. Why not just setting them to the size of the viewBox (350,350)? I don't know enough of Cairo to open a good bug report there.

@oheil
Copy link
Contributor

oheil commented Dec 9, 2022

As setting the background with function backgound is realized in cairo (for svg) by just drawing a large rectangle, how about doing the same but already in Luxor.jl, e.g.:

function background(col::Colors.Colorant)
    gsave()
    setcolor(col)
    #paint()
    rect(-16777215/2,-16777215/2,16777215,16777215;action=:fill)
    r, g, b, a = get_current_redvalue(), get_current_greenvalue(), get_current_bluevalue(), get_current_alpha()
    grestore()
    return (r, g, b, a)
end

Perhaps there must be a check if current Drawing has no bounding box, I didn't implemented it, so just thinking about.
What do you think, would this be a workaround?
As far as I can think (which is not so far) this would solve/workaround all examples above.

@hustf
Copy link
Contributor Author

hustf commented Dec 9, 2022

That is certainly a solution for some cases. About which, see 2).

Potential disadvantages would be felt on the rendering side, hence not necessarily our problem. The svg specification might be called bloated when it comes to scales and coordinate systems, so we shouldn't be surprised if various browsers and applications do their own simplifications.

The large values should probably be from typemin and typemax, with UInt16 or whatever Cairo uses.

I believe we might need a test for the type of drawing as well? This is applicable to svg of course. But what about recording surfaces? Much of the point of recording surfaces is that we can produce bitmaps initially, but output a vector drawing in the end. Would this great big rectangle lead to the bitmap rendererer allocating 16777215² pixels somewhere?

Summary: Test to see if it works?

@oheil
Copy link
Contributor

oheil commented Dec 12, 2022

I did some experiments. Outcome is that there are no huge allocations happening. Even a check for existing or not existing bounding boxes seems to be not needed.
Here is an example, but I did a few more experiments:

using Luxor, Colors

import Luxor.background
function background(col::Colors.Colorant)
    gsave()
    setcolor(col)
    println("its me")
    rect(-100000/2,-100000/2,100000,100000;action=:fill)
    #paint()
    r, g, b, a = Luxor.get_current_redvalue(), Luxor.get_current_greenvalue(), Luxor.get_current_bluevalue(), Luxor.get_current_alpha()
    grestore()
    return (r, g, b, a)
end

d=Drawing(1000, 1000, :rec)
origin()
background("blue")

snapshot(fname = "bg1.png")

sethue("red")
fontsize(100)
text("hello")

snapshot(fname = "bg2.png")

finish()

I tried :rec and snapshot, also "file.png" and ".svg".
There is never happening a large allocation of like 100000x100000=10G .
Monitoring the Julia process does not show any large or even visible RAM usage. Most visible is starting Julia itself. Everything else does not show up slightest in RAM usage for the 1000x1000 image.

I didn't found any restrictions on the size of a Cairo bitmap (except 32767x32767 but from year 2014), so I would stick with the 16777215, roughly, but there seems to be some issues, I don't understand:
With

function background(col::Colors.Colorant)
    gsave()
    setcolor(col)
    bg_max=8000000
    #println("bg max="*string(bg_max))
    rect(-bg_max,-bg_max,2*bg_max,2*bg_max;action=:fill)
    #paint()
    r, g, b, a = Luxor.get_current_redvalue(), Luxor.get_current_greenvalue(), Luxor.get_current_bluevalue(), Luxor.get_current_alpha()
    grestore()
    return (r, g, b, a)
end

A value of floor(Int,16777215/2) = 8388607 doesn't work but e.g. 9000000. To be on the safe side I propose 8000000.

I would do a PR but would like to discuss it further. Anything which needs to be checked deeper? Doubts? Resistance? Fears? Some oversight?

@hustf
Copy link
Contributor Author

hustf commented Dec 13, 2022

Doubts? Resistance? Fears? Some oversight?

There is this: PR356 / issue 355, should they choose to accept it, would become rather useless with this change, as it would return our great big rectangle.

I think the Luxor community would mostly survive the potential inkextent being ruined, though: It seems Cairo.jl is unmaintained, and that 'inkextents' PR is unlikely to be accepted. If users do the work to implement 'inkextents' in their own programs, they should also be able to avoid calling 'background'. Better still, we include a keyword argument to disable the new functionality. E.g. if we call the new functionality 'frameextend':

function background(col::Colors.Colorant; frameextend = true)

@cormullion
Copy link
Member

It seems Cairo.jl is unmaintained, and that 'inkextents' PR is unlikely to be accepted.

You could try adding tests and bumping that PR - I don't think @lobingera has abandoned Cairo.jl yet.

@hustf
Copy link
Contributor Author

hustf commented Dec 13, 2022

Thanks, cormullion! I'll have a look at adding tests for that PR.

I did a little bit of testing with the proposed filled rectangle function, and found that the fill color disappears when combined with scaling. I ran out of time to make a minimal example right now, sorry!

@hustf
Copy link
Contributor Author

hustf commented Dec 13, 2022

Here's a minimal example, starting with oheil's proposed 'background'. In the 'nonworking' part, the background is transparent.

I have no idea why it's not working, and hope there's some obvious easy reason in what I'm doing here?

I ran this in vscode, as 'snapshot' in a REPL doesn't directly launch a viewer with my current upset.

using Revise, Luxor
using Luxor.Colors: Colorant

# The proposed modification
import Luxor.background
function background(col::Colorant; frameextend = true)
    gsave()
    setcolor(col)
    if frameextend
        bg_max = 8000000
        rect(-bg_max,-bg_max,2*bg_max,2*bg_max;action=:fill)
    else
        paint()
    end
    r, g, b, a = Luxor.get_current_redvalue(), Luxor.get_current_greenvalue(), Luxor.get_current_bluevalue(), Luxor.get_current_alpha()
    grestore()
    return (r, g, b, a)
end
function background(col::T; frameextend = true) where T <: AbstractString
    return background(parse(Colorant, col); frameextend)
end
function background(r, g, b, a = 1; frameextend = true)
    return background(RGBA(r, g, b, a); frameextend)
end



# We want to zoom in on this bit, 
# right side of a filled circle at origo
cb = BoundingBox(O + (95, -10), O + (115, 20))

# Not working as intended
Drawing(NaN, NaN, :rec)
background("deepskyblue2", frameextend=true)
setcolor("gold2")
circle(O, 100, :fill) 
snapshot(;cb, scalefactor = 10.0)

# Working as intended
background("deepskyblue2", frameextend=false)
circle(O, 100, :fill) 
snapshot(;cb, scalefactor = 10.0)

@hustf
Copy link
Contributor Author

hustf commented Dec 13, 2022

It often takes a little while for me to get (back) into the right way of thinking about this. Hence, the 'large allocation' comment was not well thought through. A 'boundless recording surface' in Cairo is a stack of internal cairo commands: circle here, line there, etc. Let's call that stack our 'drawing' for now, and we may imagine a 'world' in which we did draw those things.

So the 'large rectangle' command is recorded, but the results of that command are not realized until after we have defined a canvas on which to project our drawing. A side note: Cairo can also convert those internal commands to a human readable text file, but the only purpose would be debugging. There is no way to parse such a text file back into Cairo's internal language.

Potentially large allocations would stem from us defining a very large 'canvas' to project on, and storing the projection as a bitmap. Typically, the canvas would be 640x480 pixels and stored in a png file. In the case of svg vector drawings, the problems would occur when displaying the text file. The svg is just a well defined instruction for how to project our drawing on a canvas.

Luxor docs use other meanings of 'Drawing' and 'Canvas': Here, the manifestation as an end result IS the Drawing. That's a less boring, and a good way to think about computer graphics.

@oheil
Copy link
Contributor

oheil commented Dec 13, 2022

With a zooming scalefactor, which is probably applied to the bg_max value of the rect call, it is overflowing. It's the same if you choose bg_max 10x larger, it doesn't work.
But this examples shows, that we can't solve this issue in a general way. There will always be some corner cases, where it can be constructed to fail.
Perhaps we can choose bg_max only reasonable large, like e.g. 100000. This is perhaps large enough to cover all normal use cases. Or we let the user control the size with a parameter to background()?

function background(col::Colorant; frameextend = 0)
...
    if frameextend>0
        rect(-frameextend, -frameextend, 2*frameextend, 2*frameextend; action=:fill)
    else
        paint()
    end
...
end

But than the user can call rect() instead of background on his own.
OK, now I think it should not be solved like this.
Should we create a Cairo native example, perhaps in C, to show the bug and to open an issue at the Cairo library?

@hustf
Copy link
Contributor Author

hustf commented Dec 14, 2022

I agree: some kind of overflow issue inside of the Cairo binary is stopping the proposed fix. Thank you for the insight!

But the core of this issue lies in the part that writes the svg; it doesn't cover viewports with negative coordinates.

Assuming we don't want to change long-established code like Cairo and its "svg factory", a quick fix would be to open the output svg file, replace and save:

<defs>
=>
<defs><style>svg { background-color: salmon; }</style>

Is that fix something we ought to do automatically in Luxor? The code would need to reside in finish(). But that function doesn't know which colour we wanted for the background. Unless we had a field in the Drawing type for storing background colour.

My vote is actually for closing this issue as a NOFIX. If there is a suitable place for dirty tricks like this in the documentation: fine! If not, at least this issue is searchable.

As for Cairo itself, I'd say it is what it is. I'll continue being grateful for it, and not post any issue there.

@oheil
Copy link
Contributor

oheil commented Dec 14, 2022

Fixing the svg output in finish() should be easy enough, except for the decision, what and when exactly. I will do some research on it and create a working proposal. It should be no problem to remember the color for calls to background().

Do not close this issue yet, creating a workaround is better than NOFIX.

You (and others) may provide some special graphic examples for testing, e.g. clipping regions, which have to be considered carefully, when manipulating the result svg string. All kind of more or less complex examples which may affect the result of a call to background(). Perhaps you already have something in your collections.

@hustf
Copy link
Contributor Author

hustf commented Dec 14, 2022

It's very nice if you would implement a fix that enables my kind of workflow with Luxor: I very much like to develop a drawing, zooming in and out etc. I'm always using Luxor through a 'personal package' suiting my own habits, quirks and interest. That includes storing the background and main foreground colour - other colours are often mixes of those. So for a scripting working style, to have Luxor able to e.g. revert() to the original background or foreground colour would reduce the number of @layer begin...end strewn all over. A topic for another issue / PR, perhaps?

The <style>svg { background-color: salmon; }</style> solution was made by fibo on stackoverflow. Other commenters considered viewport-fill, but having warm feelings for 'style' elements I didn't look hard for alternatives.

A reasonable test is the first image from the previously linked repo. It would be easy to drop the fancy overlay, which requires threads, while keeping the cropping boxes and the first image:
Svg:

Png:

...

@oheil
Copy link
Contributor

oheil commented Dec 15, 2022

The <style>svg { background-color: salmon; }</style> solution was made by fibo on stackoverflow. Other commenters considered viewport-fill, but having warm feelings for 'style' elements I didn't look hard for alternatives.

Neither setting background-color nor viewport-fill is a solution.
The background-color style isn't part of the image and has only effect if viewed with a browser. Other tools which open svg ignore it, e.g. Inkscape, Libre Draw.
viewport-fill can only be applied to viewport-creating elements, which is only <svg>, the others are not relevant for us in this context here, e.g. <animation>.
The following example can't be solved using them:

using Luxor, Colors
d=Drawing(NaN, NaN, :rec);
background("deepskyblue2")
setcolor("grey")
rect(-140,-140,280,280, :fill)
setcolor("black")
circle(O, 100, :stroke)
circle(O, 100, :clip)
background("magenta")
snapshot(;fname="test.png",cb=BoundingBox(Point(-150,-150),Point(150,150)))
snapshot(;fname="test.svg",cb=BoundingBox(Point(-150,-150),Point(150,150)))
finish()

PNG:
test
SVG:
test

I think the best way (the only way) is to tweak the resulting svg rect commands which are produced when calling background.

Opening the svg with Inkscape shows also a problem with the high value of 16777215 in

<rect x="0" y="0" width="16777215" height="16777215"

the viewBox (0,0,300,300) is ignored and the value is used as is, causing a bit performance issues. This is also an inkscape problem but perhaps it is something not defined in the svg specifications. I didn't check, but exporting it as png from inkscape clips fine.

Therefor the route I follow now is to adjust the

<rect x="0" y="0" width="16777215" height="16777215"

after it is produced by the cairo library and before it is written to file.

@oheil
Copy link
Contributor

oheil commented Dec 15, 2022

So for a scripting working style, to have Luxor able to e.g. revert() to the original background or foreground colour would reduce the number of https://github.com/layer begin...end strewn all over. A topic for another issue / PR, perhaps?

Yes, another issue. I don't think that the background color is part of the state in cairo_save(). My understanding is that cairo doesn't have the concept of a background color at all. Perhaps that's the underlying reason of this issue.

But I will have this in mind when I implement the solution (if I can do it, not clear yet). Saving some internals seems to be necessary for the workaround.

@oheil oheil mentioned this issue Dec 18, 2022
@oheil
Copy link
Contributor

oheil commented Dec 19, 2022

The proposed PR does basically this:
From original:

<?xml version="1.0" encoding="UTF-8"?>
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="300pt" height="300pt" viewBox="0 0 300 300" version="1.1">
<defs>
...
<rect x="0" y="0" width="16777215" height="16777215" style="fill:rgb(0%,69.803922%,93.333333%);fill-opacity:1;stroke:none;"/>
...
</defs>
<g id="surface35">
<use xlink:href="#surface38" transform="matrix(1,0,0,1,150,150)"/>
</g>
</svg>

to tweaked version:

...
<rect class="luxor_adjusted" x="0" y="0" width="300" height="300" style="fill:rgb(0%,69.803922%,93.333333%);fill-opacity:1;stroke:none;" transform="matrix(1.0,0.0,0.0,1.0,-150.0,-150.0)"/>
...

Any questions?

@oheil
Copy link
Contributor

oheil commented Dec 19, 2022

@hustf can you test current master? More tests are better... Actually I still expect some cases, where it isn't working, but I hope, that it just fails silently without crashing and without doing anything to the problematic output. This was my goal with this implementation: solve the issue where it is safe and do nothing where something unexpected happens. But this is difficult to test.

@hustf
Copy link
Contributor Author

hustf commented Dec 19, 2022

Will do

@oheil
Copy link
Contributor

oheil commented Dec 28, 2022

@hustf all changes, including last error #250 in master now.
If you like, you may check your scripts again. I would appreciate some more independent tests.

@hustf
Copy link
Contributor Author

hustf commented Jan 1, 2023

Hello, and sorry for the delay! I just had too much fun to stop early!

I believe it may be time to close this issue, as the fix itself works absolutely flawlessly. Some halfway related problems floated in the way for straight sailing, though. See 4) and 5)

As a byproduct of this, I believe pursuing 'adaptive scaling' workflow with Luxor.jl may actually be worthwhile. In order to not increase the initial treshold to using Luxor, I propose putting such functionality in a submodule, to be 'unlocked' by: using Luxor.AdaptiveScaling. I hope to open a separate issue for this tomorrow.

I had a lot of fun giving this a whirl! The fix works brilliantly! It is not strictly limited to 'background' per se. In the drawings below, we can see how blends, too, can be used as a background - the fix applies! Although svg and png blends render differently:

Svg:

Png:

Lots of other tests and examples sits here. If anybody would look into this immediately:

From 'adaptive_scaling.jl': 115, linked above:

    if endswith(filename, ".svg")
        # Ref. https://github.com/lobingera/Rsvg.jl/issues/26 - the fix seems to be 
        # implemented for large strings, but not for reading directly from file.
        st = read(filename, String);
        rimg = readsvg(st)
        # The following line failed for a large file. The smallest file with failure was 9801Kb.
        #rimg1 = readsvg(filename) 

With the fix above, we can work with extremely large svg files. I believe a similar fix applies to 'Luxor/images/_readsvgfile(fname).

There's also a problem painting large svgs onto png surfaces. During Cairo.paint(), the memory allocations are way too large and lead to crashes. This "avoidance fix", too, probably belongs somewhere in Luxor.

'adaptive_scaling.jl':351

    # Crashes experienced during Cairo.paint() within the following call to snapshot.
    # Writing the corresponding svg was OK. The .svg file size was 6237 kB.
    if filesize(fsvg) > 6237000
        @warn "The $fsvg file size was $(filesize(fsvg)/1000)kB > 6237kB. Rendering this as a png may allocate too much memory. 
                     Png output is dropped, and the svg is returned instead."
        return res

I'm linking this svg drawing where the above warning was issued. I was not able to paint the svg file on a png surface using Luxor / Cairo. The large svg works fine in every way, but the png version is actually made with Gimp after loading the svg version. To illustrate how large the svg actually is: One Luxor point corresponds to one physical cm.

@hustf
Copy link
Contributor Author

hustf commented Jan 2, 2023

Maybe closing this is premature. While trying to narrow down 4), now #254, I triggered a regexp error.

It would be nice to see if this occurs on my system only:

using Luxor
width, height, file  = 600, 600, "heavyparse2.svg"
Drawing(width, height, file)
background("teal")
for i  in range(0, 1, length  = 10)
    x = i * 0.4 * width
    y = i * 0.6 * height
    origin(x, y)
    color = Luxor.LCHuv(100, 100, i*255)
    sethue(color)
    settext("Julia")
    julialogo(; action=:fillpreserve)
end
finish()
ERROR: PCRE.exec error: match limit exceeded
Stacktrace:
 [1] error(s::String)
   @ Base .\error.jl:35
 [2] exec(re::Ptr{Nothing}, subject::String, offset::Int64, options::UInt32, match_data::Ptr{Nothing})
   @ Base.PCRE .\pcre.jl:197
 [3] exec_r_data
   @ .\pcre.jl:210 [inlined]
 [4] match(re::Regex, str::String, idx::Int64, add_opts::UInt32)
   @ Base .\regex.jl:385
 [5] iterate
   @ .\regex.jl:703 [inlined]
 [6] iterate
   @ .\regex.jl:701 [inlined]
 [7] _adjust_background_rects(buffer::Vector{UInt8})
   @ Luxor C:\Users\frohu_h4g8g6y\.julia\dev\Luxor\src\drawings.jl:715
 [8] finish()
   @ Luxor C:\Users\frohu_h4g8g6y\.julia\dev\Luxor\src\drawings.jl:854
 [9] top-level scope
   @ c:\Users\frohu_h4g8g6y\.julia\dev\Luxor\test\parse-svg.jl:39

If this is indeed reproducible, pfitzseb found a fix here.

@oheil
Copy link
Contributor

oheil commented Jan 2, 2023

@hustf great finding. No, it's not only you, I didn't check for now but I got those errors during development too. I already did some fine tuning but you found another one, nice! These type of errors are really a problem with RegExs, as they can't easily be forced.
I will solve it today (in a few hours) and do a PR to master. Perhaps, checking pfitzsebs fix will help for a more generic solution so it is more safe for the future. Also didn't check for now, time is currently short, but later it's better.

@oheil
Copy link
Contributor

oheil commented Jan 2, 2023

Done: #255

@hustf
Copy link
Contributor Author

hustf commented Jan 2, 2023

Great, but I just found another one, while you were working. I'll try to reproduce with the fix in #255.

If you inspect Malformed_output.svg, you'll find that

  • The file opens in browsers, inkskape, gimp, etc.
  • The file crashes readsvg(filename)
  • The file does not crash when reading it as a string, like below. Ref. the fix 4) above.
st = read(filename, String);
rimg = readsvg(st)

So I assume we use a more lenient parser version when reading from a string rather than a file.

Inspecting the file, we see the difference, which is not acceptable to readsvg(filename):

<?xml version="1.0" encoding="UTF-8"?>
<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" width="800pt" height="734pt" viewBox="0 0 800 734" version="1.1">
<defs>
<g id="surface507" clip-path="url(#clip1)">
<rect class="luxor_adjusted" x="0.0" y="0.0" width="800.0" height="734.0" style="fill:rgb(93.333333%,91.372549%,91.372549%);fill-opacity:1;stroke:none;" transform="matrix(253.3064895447798,47.1272179402461,-47.1272179402461,253.3064895447798,-43616.42399848523,-168561.0908834971)"/>

Similarly produced, but smaller files, did not have 'class="luxor_adjusted`. Is the class supposed to be a temporary insertion? I'm not sure.

I'm now going to try and reproduce with #255!

@oheil
Copy link
Contributor

oheil commented Jan 2, 2023

I can work with it:

filename="Malformed_output.svg"
svg=readsvg(filename)
Drawing(NaN, NaN, :rec);
placeimage(svg)
width, height, file  = 600, 600, "test.svg"
snapshot(;fname=file,cb=BoundingBox(Point(-300,-300),Point(width,height)))
finish()

no errors and test.svg opens well.

The file crashes readsvg(filename)

Not for me.

The file does not crash when reading it as a string, like below. Ref. the fix 4) above.

Yes, this is another issue. You did a fork so I do nothing here. I think you can change the code savely as you proposed by first reading the file into a String and provide this String to _readsvgstring(str).

class="luxor_adjusted"

I have added this to put a stamp on the changes which are done by the patch for this issue here to be sure that we can recognize and find the changed lines in the svg. It should not be a problem as it is valid SVG syntax.

@hustf
Copy link
Contributor Author

hustf commented Jan 2, 2023

Hm, a mystery! I'll try this on a mac tomorrow. It's not cool if this is a windows/ mac thing, because we have enough of those already. It may also be related to the Pango version I'm forced to use on Windows for text rendering.

Meanwhile, with the new PR #255, the file produced is a little different, but still crashes my Windows rsvg. Here is the new file:
Malformed_output2.svg

I also updated 'adaptive_scaling.jl' so that it now crashes if I run 'snowblind - whirl.jl', while producing file 106.svg.

@oheil
Copy link
Contributor

oheil commented Jan 2, 2023

Here is the new file:
Malformed_output2.svg

Still working fine for me, no error.

@oheil
Copy link
Contributor

oheil commented Jan 2, 2023

I downloaded your repository... it's a bit complex. Trying 'snowblind - whirl.jl' I get:

julia> snap(overlay, cb, outscale; pt, scale = outscale, text)
ERROR: TaskFailedException
Stacktrace:
 [1] wait
   @ .\task.jl:345 [inlined]
 [2] fetch
   @ .\task.jl:360 [inlined]
 [3] snap(f_overlay::Function, cb::BoundingBox, scalefactor::Float64; fkwds::Base.Pairs{Symbol, Any, Tuple{Symbol, Symbol, Symbol}, NamedTuple{(:pt, :scale, :text), Tuple{Point, Float64, String}}})
   @ Main c:\Temp\infinite_source\adaptive_scaling.jl:352
 [4] top-level scope
   @ REPL[42]:1

    nested task error: XML parse error: error code=201 (3) in (null):2:53: Namespace prefix svg on ellipse is not defined

Not sure if this has something to do with this issue.

@hustf
Copy link
Contributor Author

hustf commented Jan 3, 2023

EDIT: Please disregard the response below. I started up the macintosh computer now, and the error message indicates you hit some temporary debug code. You already figured out that you had to "Pkg.free Pango". I hit some errors, too, and will look further into making this work on macos tonight.

_That's a helpful error message from reading one of our 'finished' svgs!

I believe the parser on your system may not be SVG 2 compliant. And the svg parser on my system config is not either, though it failed to show this good an error message.

Would it be easy to retain the XML namespace tags during finish? There could be other solutions as well, by changing the headers.

SVG used to be more clearly a self-defining format, hence the namespace requirements. Later, css was implemented since no one really went to the trouble of also providing the XML schemas with namespace definitions. I don't know much about it, but when inserting
scripts in svgs, I found I would need to formulate scripts as valid XML. So scripting svgs is not widespread or recommended._

@oheil
Copy link
Contributor

oheil commented Jan 5, 2023

Resolved in master. Issue should be closed. If anything happens it's better to create new issues. The existing issues are getting cluttered a bit.

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

No branches or pull requests

3 participants