Issue #15 open

Boxes with + corners cause a memory leak.

jason_m
created an issue

I have the following small test diagram

        .------------.
        |            |
        |   A Box!   +<----.   .-------------.
        |            |     |   |             |
        '-----+------'     |   |   Another   +--------+
              ^            '-->+     Box     |        |
              |                |             |        |
              |                '-------------'        |
              v                                       |
            .-+-.                                     |
            |   |                                     |
            '---'                        +------------+-----+
                                         |                  |
                                         |                  |
                                         |                  |
                                         |                  |
                                         +------------------+

However, (with 1GB allocated to PHP!), I get the following runtime error:

PHP Fatal error:  Allowed memory size of 1073741824 bytes exhausted (tried to allocate 523800 bytes) in /Users/jason/Projects/asciitosvg/ASCIIToSVG.php on line 2018 

(also reports line: 2011 on occasion.)

Assume there's a memory leak in the drawing loop.

Comments (22)

  1. Devon O'Dell repo owner

    A while ago, I changed the semantics of '+' so that it could no longer be used as a box corner. There are ambiguities when it can be both a box corner and a line intersection point. Use '#' instead of '+' for the corners of your lower right box, and this should be fixed.

  2. jason_m reporter
    • changed status to open

    you may want to consider trapping the error anyway.

    The other thing you may want to consider is that Emacs artist-mode, ditaa, etc. all use + as a squared corner, it's also more intuitive than #.

    Also your docs currently state:

     +: Boxed, angular, 90 degree corners for lines (plus)
    
  3. jason_m reporter

    You may also want to think about using the + as just a corner char on boxes and polylines (where they still work), and use a # for line intersections.

    Just to follow long established user patterns. (ie. Artist Mode has been kicking around for 18 years or so.)

  4. Devon O'Dell repo owner

    Those are all fair points. I'll make the line walker detect recursion and bail with (hopefully) some useful debugging info. I'll also clarify the documentation to make it more obvious that + is for lines.

    If you have a good idea on how to remove the ambiguity between + for box corners, I'd be happy to make them supported for those again.

  5. jason_m reporter

    Is there a problem with switching intersection to use # instead?

    Having to re-draw a bunch of ascii art from over the years doesn't sit well for me.

    I may just patch it in and add a command line switch for "classic style"

    Problem is really that squared corner lines use a + so the # is just inconsistent.

    Intersections in ditaa use + for all three cases, so I'm not sure what the friction is, I'm happy to have a crack at fixing it.

  6. Devon O'Dell repo owner

    Apparently I fixed whatever bug that was causing the friction; seems like it's as simple as adding '+' back to the isBoxCorner method. I'm going to do some more testing and apply this change for boxes if it works, along with the recursion catching.

  7. Devon O'Dell repo owner

    Ah, I lied. I remember why I made the distinction. Since lines can pass (all the way through) boxes, there is ambiguity in the size and bounds of the box when you don't make the distinction.

    Consider:

       ^  ^
       |  |
    +--+--+--+
    |  |  |  |
    |  |  |  |
    +--+--+--+
       |  |
       v  v
    

    This just looks weird. Additionally, consider the following intersecting lines:

        ^   ^
        |   |
    <---+---+--->
        |   |
    <---+---+--->
        |   |
        v   v
    

    I think we need to come to a consensus on the behavior in these ambiguous scenarios. My proposal is:

    1) Try to find the largest box possible. This means we follow the box to the last potential corner, marking all previous corners. If we cannot complete the box from the farthest corner, we backtrack to the previous one. Example of why this is necessary:

    +------+---+
    |      |   |
    |      | <-+
    |      |
    +------+
    

    2) If all four corners of an enclosed region continue in their respective directions, do not create a box. I.e. iff NW continues north and west, NE continues north and east, SE continues south and east, SW continues south and west.

    These two rules seem like they'll probably be relatively easy to implement in wallFollow. Backtracking will be a bit more difficult, but whatever. I can't think of any other examples of box detection would lead to ambiguity (except when considering diagonals, which I don't want to do right now... one step at a time).

    Can you think of any other cases / special heuristics that need considering?

  8. jason_m reporter

    The usual advice in ditaa for those case is to have a pass through such as...

        ^   ^
        |   |
    <----------->
        |   |
    <----------->
        |   |
        v   v
    

    or

        ^   ^
        |   |
    <----------->
        |   |
    <---|---|--->
        |   |
        v   v
    

    etc. ie. the solution is to document it away.

  9. Devon O'Dell repo owner

    I don't like that solution. One of the reasons I wrote ASCIIToSVG is because I really didn't like ditaa's line intersection handling with respect to boxes and each other. Despite the (current) inconvenience of having to use # as a box corner, a2s already handles those intersections much better than ditaa does. It's obviously a "hard" problem, but it should be solved.

    Also, I'm not necessarily shooting for 1:1 compatibility with ditaa. Because it doesn't do intersections well doesn't mean a2s shouldn't :)

  10. jason_m reporter

    Mind you, I just tried this monstrosity

                 +-----+-------+
                 |     |       |
                 |     |       |
            +----+-----+----   |
    --------+----+-----+-------+---+
            |    |     |       |   |
            |    |     |       |   |     |   |
            |    |     |       |   |     |   |
            |    |     |       |   |     |   |
    --------+----+-----+-------+---+-----+---+--+
            |    |     |       |   |     |   |  |
            |    |     |       |   |     |   |  |
            |   -+-----+-------+---+-----+   |  |
            |    |     |       |   |     |   |  |
            |    |     |       |   +-----+---+--+
                 |     |       |         |   |
                 |     |       |         |   |
         --------+-----+-------+---------+---+-----
                 |     |       |         |   |
                 +-----+-------+---------+---+
    

    and it renders out fine

  11. jason_m reporter

    If I do App::Asciio style shading

                 +-----+-------+
                 |     |       |
                 |     |       |
            +----+-----+----   |          
    --------+----+-----+-------+---+     
            |    |     |.......|   |     
            |    |     |.......|   |     |   |
            |    |     |.......|   |     |   |
            |    |     |.......|   |     |   |
    --------+----+-----+-------+---+-----+---+--+
            |    |     |.......|   |.....|   |  |
            |    |     |.......|   |.....|   |  |
            |   -+-----+-------+---+-----+   |  |
            |    |     |       |   |     |   |  |
            |    |     |       |   +-----+---+--+
                 |     |       |         |   |
                 |     |       |         |   |
         --------+-----+-------+---------+---+-----
                 |     |       |         |   |
                 +-----+-------+---------+---+
    

    I just get text for the .s

  12. Devon O'Dell repo owner

    Sorry, I meant drop shadows, and I forgot they're on by default.

    Just got a JVM installed to test Ditaa for that "monstrosity" :). I think Ditaa's output is broken. There are arguably no boxes in that drawing. Even if there are boxes, I can argue that the boxes picked by Ditaa are the wrong ones. Ideally, boxes can't share edges. Indeed, with the input you provided, a2s only renders lines. Like I said, my goal isn't to render the same way as Ditaa does for the same input (otherwise, I'd just use Ditaa).

    My philosophy is that if it renders ambiguously, it also looks ambiguous in text form. I think you have a valid point that + should be considered for box corners, but this isn't something I'm going to be able to fix tonight. Backtracking is too slow. I'll need a different approach, different rules, or a different box finder. Or a different language.

    I'm going to think about this some more, but in the mean time # as a box corner will solve the problems. Maybe it will work to find all of the smallest non-intersecting boxes and handle those. Another alternative is to require all distinct line segments to have at least one marker. Non-directional lines in ASCII diagrams seem pointless to me.

    For many sane drawings, you can edit ASCIIToSVG.php and remove '+' from isBoxEdge() and get reasonable output.

    If you have better ideas and want to submit a patch, I'd be welcome to looking at it, but depending on the behavior, I might not merge it in. (For instance, if you made the "monstrosity" drawing output the same thing visually as Ditaa outputs, I wouldn't really be a fan of that.) I'm sure there's a reasonable compromise.

  13. jason_m reporter

    Thanks Devon,

    • Regarding the monstrosity I'd expect to have to make boxes in there, explicit with a class annotation of some sort, more usually for a diagram like the one below (so that there's no ambiguity, about what is and isn't a box.)

    • Don't worry about a quick fix, there's no expectation on my part, I really appreciate all the feedback.

    • Ultimately there are several ways to solve this, one of which is to patch artist-mode and give it a couple of new box methods for a2s.

    • Non directional lines have a place, for example...

    When it comes to ascii diagramming there's a lot more possibilities than the typical graphviz or plantuml output. (oh that reminds me, you may want to add those two to your other ascii (or text to graph) references list in the docs.) - just being able to throw together a small arbitrary diagram is a huge help.

    • I'll check the isBoxEdge() and see, but I want to study the code at length, I should have time to devote in a week or two, it would be nice to have a more complete understanding, and I'd definitely like to be able to contribute to such a useful tool.

    I just wanted to add that so far what you have here is a fantastic tool, and I really want to thank you for taking the time to put it together, and for taking the time to talk to me about it, as I say, it's a fantastically useful tool, and for me personally the SVG output is nth to the power more useful than PNG or even PS/EPS output.

    doing this with Emacs and a browser on auto-refresh (+ watching the ascii text > svg folder) is great.

    Thanks again.

  14. Devon O'Dell repo owner

    I'm sorry I didn't reply to this sooner -- it's been a bit busy around these parts. You provide some really great examples here. I think that A2S can already do the boxes in such a way as to support the VM / CPU diagram you show.

    Maybe the "easiest" answer is to make line intersections use # instead of box corners.

    You say: "Ultimately there are several ways to solve this, one of which is to patch artist-mode and give it a couple of new box methods for a2s." What do you mean by this? I'd be interested in any / all ideas you have; you clearly have a more diverse application for this tool than I do, given your provided examples :)

    Regarding non-directional lines and markers, perhaps there is some character that could be useful for signifying the beginning of a line? The marker wouldn't necessarily have to signify direction, it could simply serve to signify a line. (It still feels kind of hacky.) Any way to distinctly identify a line as separate from a box would solve this problem entirely.

    Also, nyancat is awesome.

  15. jason_m reporter

    Hi Devon, I did look into patching artist-mode but its not a straightforward task at all (artist.el Emacs lisp)

    Parsing '+' corners would be the only solution that works for me.

    I've been crazy busy too and haven't had a good opportunity to look into this much further either.

  16. Log in to comment