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

Saving and loading graphs #38

Closed
jcageman opened this issue Jul 27, 2020 · 7 comments
Closed

Saving and loading graphs #38

jcageman opened this issue Jul 27, 2020 · 7 comments

Comments

@jcageman
Copy link
Contributor

jcageman commented Jul 27, 2020

Would it be possible to extend the library to also load & save graphs? Would be very useful for testing purposes!
Of course the saving should also be possible from code and viewer, such that it's easy to dump a graph and load it up in the viewer

@jcageman jcageman changed the title Saving and loading graphs within the viewer Saving and loading graphs Jul 27, 2020
@roy-t
Copy link
Owner

roy-t commented Jul 27, 2020

The editor is quite simple at the moment and doesn't support displaying arbitrary graphs. I think all the classes used are easily serializable by the built in serializers. Have you tried that? Otherwise I might be able to take a look at this request. :)

@jcageman
Copy link
Contributor Author

jcageman commented Jul 27, 2020

I tried serializing the grid to json, but the nodes matrix isn't serialized already. The format would't need to be efficiënt anyhow. As long as the editor would support grids up to 1000x1000 it should be good enough for testing. Might also be worth looking into graph viz for example just as an idea for general graphs.

@jcageman
Copy link
Contributor Author

Would you have any thoughts how you would want it implemented? i could give it a go and submit a pull request?

@roy-t
Copy link
Owner

roy-t commented Jul 31, 2020

Hey, I was going to respond. But I see you went ahead already. But to be honest I was very much in doubt if this should be part of this library. Let me explain :).

I see the concrete Node and Edge classes as examples, or for quick prototyping. While I expect games to implement INode and IEdge via their own classes. And games having their own ideas on how to serialize them.

That leaves serialization of value in two scenarios:

  • In the editor
  • In prototypes

In case of the editor it might be nice. But the editor is mainly there tho show you how this library works. You can't make arbitrary graphs with it. So I'm not sure if a save/load feature is needed as you can always quickly recreate what you had in a few clicks.

In case of prototyping it might add real value. As you give users a quick way to load/save the graph. In that case also having the feature in the editor is nice because it gives people an example.

I've looked at your PR and I like it a lot more than I thought I would. The code is clean and having the separate types for serialization is very nice. However given the above ideas I have a couple of suggestions (If you agree).

  • Use one of the built-in serialization methods .net offers so we don't add an external dependency just for serialization (I know Newtonsoft is awesome, but one of the promises of this library is no external dependencies :)).
  • To signal better that serialization is a separate feature don't use extension methods, but use normal static methods
  • Add a single unit tests (create/save/load) as a smoke test so that new features don't accidentally break serialization

Other than that I like the code a lot!

@jcageman
Copy link
Contributor Author

I'll rewrite my PR given your feedback and I'll write some unit tests to support it 👍 Which serialization method would you prefer? I thought json would be nice since it's readable at least (especially in case of issues when format changed).

@jcageman
Copy link
Contributor Author

Just submitted my latest changes, please have another look

@roy-t
Copy link
Owner

roy-t commented Aug 2, 2020

Hey, changes look great. I've merged the changes and updated the version. Thanks for contributing! :D

@roy-t roy-t closed this as completed Aug 2, 2020
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

2 participants