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

Save/load graph JSON #63

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

natemoo-re
Copy link

@natemoo-re natemoo-re commented Jun 7, 2019

#35 seemed like a fairly simple implementation, so I decided to take a crack at it.

I understand that you may have different plans for this feature, but I figured this would open up a conversation about the possible implementation.

Features

  • Adds serialization via a toJSON method on Anchor and Vector. This is natively called by JSON.stringify, so JSON.stringifying an illustration will work as expected.
  • Adds serialization util via Zdog.exportGraph. It's really just calling JSON.parse(JSON.stringify(model)) but could be hand-rolled if you want.
  • Adds a type property to each Zdog primitive. There may be a cleaner way to do this, but the JSON needs to store the primitive type as a string so that it can be rehydrated.
  • Adds importGraph option to Anchor object. Users can pass in
    1. A JSON object generated by Zdog.exportGraph
    2. A string specifying a relative or absolute path to a json file. This seemed like a cool feature which would allow people to easily share models via url. It's just a simple fetch call for modern browsers, but I realize that this may introduce some unwanted overhead for legacy browsers.

In Progress

  • Code style (Need to double check for consistency)
  • Versioning? What happens if Zdog's JSON format changes in the future as new features are added?
  • Compression. JSON result is fairly verbose, although I made sure to trim down what I could (single-magnitude vectors to numbers, default options not included, etc). I'm sure there are some interesting ways to optimize the output.

@desandro
Copy link
Member

desandro commented Jun 7, 2019

Whoa! Thanks for taking a shot at this. This PR is pretty impressive, as it implements the feature without adding too much bloat. Nice work!

A couple notes

  • I'd like to have IE11 support for Zdog v1. Let's remove fetch for now.
  • You can check code style with npm install and make lint

Some blue-sky thinking: what if Zdog's save/load file structure was built out of SVG instead of JSON? I like SVG as it outputs a visual file that users can easily view. I don't this SVG is appropriate for saving/loading data as an SVG file as it would require building a parser and duplicating data in the file source code (like for path data). I'm spitballing here... what if the file was .svg, but it had the JSON embedded in a hidden node. So users sees a 'thumbnail' in the SVG, but the real data is in the JSON within the file? Maybe that's for a future feature.

@natemoo-re
Copy link
Author

natemoo-re commented Jun 8, 2019

Thanks! I've removed fetch and am working through some lint errors.


The SVG idea is very cool—I will have to play around with it! Off the top of my head, I think the JSON could probably be stringified and embedded as a [data-zdog] attribute on the root svg? JSON is probably the most straightforward solution for the time being, though.

@desandro
Copy link
Member

Re-thinking this: SVG filetype save/load can be a future feature. It still relies on JSON save/load so that should be the focus.

Also: I think this feature would be better served as a plugin. I think you should build it out directly in Zdog for now, but I'll likely break it out into its own package when its ready for prime time.

@natemoo-re natemoo-re marked this pull request as ready for review June 11, 2019 11:34
@natemoo-re natemoo-re changed the title WIP: Save/load graph JSON Save/load graph JSON Jun 11, 2019
desandro added a commit that referenced this pull request Jun 14, 2019
Thx @natemoo-e for cluing me in #63
@jcoc611
Copy link

jcoc611 commented Aug 14, 2019

I agree that it makes sense for this to be a plugin/extension, but for the sake of making it easy to implement, could we add just the Class.type = 'Class' metadata? Otherwise it's not straightforward to get the subclass names. This is because of the getSubclass(...) implementation, as shape.constructor.name is 'Item' for all subclasses.

In terms of using the svg as the saved format, I think it makes more sense for saving the 2D representation, as it doesn't inherently have the 3D information in it (neither does the canvas but you could save a 2D picture from it).

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

Successfully merging this pull request may close these issues.

None yet

3 participants