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

Sharing Node instance across different SVGViews causes infinite loop #488

Closed
lubiluk opened this issue Oct 11, 2018 · 12 comments
Closed

Sharing Node instance across different SVGViews causes infinite loop #488

lubiluk opened this issue Oct 11, 2018 · 12 comments
Assignees
Milestone

Comments

@lubiluk
Copy link

lubiluk commented Oct 11, 2018

Hi, I use Macaw (great library) in such a way that I download SVG files from the internet to memory cache, turn them into Macaw Nodes, manipulate them to alter their colors and then use them as icons in a table view.

The issue

I noticed that when one Node instance appears in more than one SVGView the main thread enters an infinite loop (somewhere in drawRect).

Discussion

It looks like Macaw doesn't really like using Nodes directly. For instance, once a node is set on MacawView, there's no way to nil it out, so I had to use an awkward construction like this:

if let icon = model.icon {
    titleCell.iconSvgView.node = icon
} else {
    titleCell.iconSvgView.fileName = nil
}

I can of course try to redesign my code so it saves images on the disk but isn't it worth discussing why a flow that would be typical for UIImage does not work for Macaw.Node?

@lubiluk lubiluk changed the title Sharing Node instance across different MacawViews causes infinite loop Sharing Node instance across different SVGViews causes infinite loop Oct 11, 2018
@amarunko
Copy link
Contributor

Hi, @lubiluk, you can use MacawView instead of SVGView, but get Node from SVGparser:

open class func parse(path: String, ofType: String = "svg") throws -> Node

But avoid of parsing SVG each time, it can take some resources, better cache parsed nodes.

@lubiluk
Copy link
Author

lubiluk commented Oct 11, 2018

@amarunko Thanks for the suggestion. Unfortunately, MacawView also enters an infinite loop when shares a node with another MacawView (or SVGView).

Tested like so:

class ViewController: UIViewController {
    @IBOutlet weak var view1: MacawView!
    @IBOutlet weak var view2: MacawView!
    
    override func viewDidLoad() {
        super.viewDidLoad()

        let url = URL(string: "https://css-tricks.com/wp-content/uploads/2015/05/kiwi.svg")!
        let svg = try! String(contentsOf: url, encoding: .utf8)
        
        let node = try! SVGParser.parse(text: svg)
        
        view1.node = node
        view2.node = node
    }
}

Macaw version 0.9.3.

@amarunko
Copy link
Contributor

Did you try copying of node for quick fix?

func copyNode(_ referenceNode: Node) -> Node? {
        let pos = referenceNode.place
        let opaque = referenceNode.opaque
        let visible = referenceNode.visible
        let clip = referenceNode.clip
        let tag = referenceNode.tag

        if let shape = referenceNode as? Shape {
            return Shape(form: shape.form, fill: shape.fill, stroke: shape.stroke, place: pos, opaque: opaque, clip: clip, visible: visible, tag: tag)
        }
        if let text = referenceNode as? Text {
            return Text(text: text.text, font: text.font, fill: text.fill, stroke: text.stroke, align: text.align, baseline: text.baseline, place: pos, opaque: opaque, clip: clip, visible: visible, tag: tag)
        }
        if let image = referenceNode as? Image {
            return Image(src: image.src, xAlign: image.xAlign, yAlign: image.yAlign, aspectRatio: image.aspectRatio, w: image.w, h: image.h, place: pos, opaque: opaque, clip: clip, visible: visible, tag: tag)
        }
        if let group = referenceNode as? Group {
            var contents = [Node]()
            group.contents.forEach { node in
                if let copy = copyNode(node) {
                    contents.append(copy)
                }
            }
            return Group(contents: contents, place: pos, opaque: opaque, clip: clip, visible: visible, tag: tag)
        }
        return .none
    }

About content scale - very strange, it can be a bug, did you set content mode after updating node?

@amarunko
Copy link
Contributor

I will try to test case with one shared node for 2 or more MacawViews. I hope we can fix this loops.

@lubiluk
Copy link
Author

lubiluk commented Oct 11, 2018

Thanks, I was searching for a way to copy a node.

Content mode works well if I set it in code, it just ignores the value set in interface builder. SVGView however respects setting from interface builder.

@lubiluk
Copy link
Author

lubiluk commented Oct 12, 2018

Hey, I think the issue with infinite loop is somewhat more complex. I kept getting problems with that, so I decided to convert everything to UIImage before using on screen. Unfortunately now my app freezes in the code that converts SVG into UIImage.

What I've found so far:

  1. Multiple MacawViews / SVGViews on screen cause problems when share one Node
  2. Multiple MacawViews / SVGViews on screen cause problems when they don't share Nodes but nodes are reassigned to them
  3. SVGParser causes problems when used in parallel in two or more threads.

I'm attaching a screenshot from profiler. NodesMap seems to be guilty somewhat in case of SVGParser. For Macaw view the tree in profiler is so complex that I don't know what's going on yet.

screen shot 2018-10-12 at 16 05 35

I hope you find this useful.

@amarunko
Copy link
Contributor

Unfortunately, SVGParser works well only on the main thread, we plan some multithreading work a little bit later. Thanks for deeper investigation. Did you use the release version of Macaw?

@lubiluk
Copy link
Author

lubiluk commented Oct 13, 2018

I tried release version 0.9.3 as well as latest master. Both installed with carthage.

@f3dm76
Copy link
Collaborator

f3dm76 commented Nov 29, 2018

Hello lubiluk,
We introduced some major changes to inner Macaw organization which allowed the usage of one node in multiple places. You are welcome to try it out. Please let us know if there are still problems.

@ystrot ystrot self-assigned this Feb 14, 2019
@ystrot ystrot added this to the 0.9.4 milestone Feb 14, 2019
@ystrot
Copy link
Member

ystrot commented Feb 14, 2019

Should be working in the 0.9.4 release.

@ystrot ystrot closed this as completed Feb 14, 2019
@K-Be
Copy link
Contributor

K-Be commented Feb 27, 2019

I've faced with same issue in the 0.9.4 release.

@Sega-Zero
Copy link

Still facing this

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

6 participants