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

Decouple graph structure from views #125

Closed
Rupert-RR opened this issue May 29, 2017 · 10 comments
Closed

Decouple graph structure from views #125

Rupert-RR opened this issue May 29, 2017 · 10 comments
Assignees
Milestone

Comments

@Rupert-RR
Copy link

Hi guys!

Firstly, thank you for the library — it's definitely something that is needed!

My immediate needs are for parsing and rendering SVG images as static images.
I know that this is possible using a MacawView (or SVGView), but this seems unnecessarily heavy for rendering images, if we are not actually using views (views are expensive).

How would you feel about enabling rendering without the view dependencies?

  • we could start by decoupling the scene models from view concerns (basically the touch methods);
  • NodeRenderer should have its AnimationCache as optional (since it is only ever used optionally anyway), which would allow us to create renderers which do not depend on views;
  • we could abstract out some of the rendering logic from MacawView into a protocol with default implementations, allowing us to make MacawView conform to the protocol so that we can use the same logic, while also being able to make a different, non-view class which can render without a view.

Incidentally, I think we ought to be able to use some dependency injection in the view / animation methods to get rid of the nodesMap and parentsMap singletons.

Let me know what you think. I would be happy to try to create some PRs for it, if you think this is a direction you might like to take. Otherwise I would probably try creating my own library, with this as a good starting point.

@ystrot
Copy link
Member

ystrot commented May 30, 2017

Hey!

This is definitely on our roadmap and we would be happy if you can work on a PR. Your idea is right -
we need to have view-independent renderer with optional animation cache/nodesMap/etc. which can draw scene on any Graphics Context. Please let us know if you have any other questions.

@Rupert-RR
Copy link
Author

Rupert-RR commented May 30, 2017

Thanks for getting back to me.
I have already started making the animation cache optional and am making the nodesMap a generic injected dependency. This does mean, however, that I need to make the NodesMap class public because we need to be able to inject it in the Array extension method group( _: Transform, _: Bool, _: Double, _: Locus?, _: Effect?, _: Bool, _: [String]) which is public.
It will also affect the public signatures of the animation classes, since they will become generic. Is this ok for you?

@vhailor13
Copy link
Contributor

Hello, @Rupert-RR ,
As I can see, it is a better way to make NodesMap - internal optional property for Node's classes. So we still incapsulate singleton's maps from users and be able to inject proper instances using MacawView and similar wrappers around CGContext

@Rupert-RR
Copy link
Author

We can do that, but in which case we cannot have NodesMap as a generic, which means tying it to MacawView, which seems a bit pointless when all that is needed for the mapped object is an object with a node property.

@vhailor13
Copy link
Contributor

NodesMap still can be generic, something like:

class NodesMap<T: AnyObject> {
let map = NSMapTable<Node, T>(keyOptions: NSMapTableWeakMemory, valueOptions: NSMapTableWeakMemory)

  • rename all methods to something more general:

getView() -> getContainer()
add(_ node: Node, view: MacawView) -> add(_ node: Node, container: T)

@Rupert-RR
Copy link
Author

But we can't add a generic property to a class without specifying the concrete type used by the property.

weak var nodesMap: NodesMap<T>?

will not work, T must be a concrete type.

And if we don't provide a nodesMap parameter in the Array extension method group( _: Transform, _: Bool, _: Double, _: Locus?, _: Effect?, _: Bool, _: [String]) then all uses of that method will result in a Group with a nil nodesMap (we could add an internal method with that parameter but then it just means that public uses of the method may have the surprising effect of not linking to the node map).

@vhailor13
Copy link
Contributor

Good point, going to try quick workaround using protocol:

protocol MacawContainer: AnyObject {
var node: Node { get set }
}

class NodesMap {
let map = NSMapTable<Node, AnyObject>(keyOptions: NSMapTableWeakMemory, valueOptions: NSMapTableWeakMemory)

@vhailor13
Copy link
Contributor

Seems like protocol is good solution, here is some updated logic:

Archive.zip

@Rupert-RR
Copy link
Author

I considered using a protocol, but then you end up having to cast to the MacawView in the animation method, to get the layer.
But it's probably a good way to start with, since it avoids changing any public API.
I'll have a look again after lunch.

@ystrot ystrot self-assigned this Nov 20, 2018
@ystrot ystrot added this to the 0.9.4 milestone Nov 20, 2018
@ystrot
Copy link
Member

ystrot commented Nov 20, 2018

This issue can be closed since we have Node.toNativeImage method which doesn't use any view for rendering.

@ystrot ystrot closed this as completed Nov 20, 2018
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

3 participants