stanch / reftree

Automatically generated diagrams and animations for Scala data structures
http://stanch.github.io/reftree/
GNU General Public License v3.0
587 stars 36 forks source link

Add rendering of diagrams in arbitrary formats #6

Closed sakshamsharma closed 7 years ago

sakshamsharma commented 7 years ago

Should exist. For obvious reasons.

Another reason why this is needed: Some operating systems have a buggy Graphviz which breaks PNG support for dot on those (for example: NixOS).

Note: I did not run sbt test due to the requirement for bintray credentials. But copied the source into the source tree of my own project which uses reftree, and the changes did work that way.

stanch commented 7 years ago

Thanks! A few doubts:

Should exist. For obvious reasons.

I’m afraid the reasons are not obvious to me :) JPG is not well suited for machine graphics, especially ones with crisp edges, like these diagrams (see https://en.wikipedia.org/wiki/Ringing_artifacts).

Some operating systems have a buggy Graphviz which breaks PNG support for dot on those (for example: NixOS).

Is there an issue opened somewhere?

Note: I did not run sbt test due to the requirement for bintray credentials.

Could you elaborate? I just cloned the repo on a new work machine that does not have any bintray credentials and sbt test ran without problems.

stanch commented 7 years ago

By the way, would you be ok with generating SVG and then using something like inkscape to convert to PNG?

sakshamsharma commented 7 years ago

Sorry, I didn't see your comments before updating the PR. I'll address them:

  1. Allowing arbitrary formats to be used, depending on the person's use case, seems like a very good thing to have. What if I want svg or postscript, for instance?
  2. Here is one example of such a bug. A friend (who runs NixOS) encountered a similar bug recently (while using reftree for dot).
  3. Weird. I shall try later when I get time (or let me know if you've done that by now). But hey, I tested by (ahem) copying the source into my tree and importing it as a local library (I know this ain't an excuse for not testing, I'm sorry I was a bit busy today).

I've updated the PR to allow any arbitrary format. Since there is no harm done in allowing formats dictated by the user, and dot may add more formats, we should not try to dictate which ones are allowed (IMHO). Let me know if this goes down good with you, or we can work out some better solution :smile:

stanch commented 7 years ago

Hey, sorry for the delay! I like the way this is shaping :)

Allowing arbitrary formats to be used, depending on the person's use case, seems like a very good thing to have. What if I want svg or postscript, for instance?

I agree! But please do not use JPEG ;)

But hey, I tested by (ahem) copying the source into my tree and importing it as a local library

My concern was not that you did not run the tests (after all, there are just a few), rather that the something in the repo might have not been configured correctly. So I would appreciate if you try to do it again and paste the error you are getting. By the way, running sbt tut is way more important, I should document that somewhere... It takes a while, but it exercises all kinds of diagrams and animations and you can check in git status that no rendering changes were introduced.

Going to comment inline with some suggestions (mostly cosmetic).

sakshamsharma commented 7 years ago

Great! BTW that test thing was my fault. It was an issue with sbt-coursier of the likes of this.

I commented out this line from project/build.sbt to make it work:

addSbtPlugin("io.get-coursier" % "sbt-coursier" % "1.0.0-M14")

Perhaps because I have different versions of packages somewhere.

Anyway, there's no point in specifically removing JPEG. If someone really wants that, they can just provide it as a string parameter.

Update: Noted your points. Will update in a short time.

sakshamsharma commented 7 years ago

Aah I'll fix up those changes. test and tut worked fine now. And apologies for not noticing your coding style, no problem!

sakshamsharma commented 7 years ago

Updated! Of note, I noticed the binary files in images/ had changed in this commit. I tried looking at some, but didn't notice any visible difference. Honestly, I don't see any reason they should not change (creating new files by dot). I tried using your master code and ran tut on that as well, which changed the images too. So the git status method to determine changes isn't correct. Regardless, test did work.

Edit: Why should demo.md also change everytime tut is run? Isn't this quite annoying?

stanch commented 7 years ago

I tried using your master code and ran tut on that as well, which changed the images too.

Are there changes in both static diagrams and animations? I used to have some non-determinism with the animations, but that should have been fixed. Maybe what is happening is that your dot is not the same as mine. Or are you saying that you get different output between the two branches on your machine? What if you re-run sbt tut, is it at least deterministic in a given branch? Sorry to bother you with these experiments, it’s just out of curiosity :) I can probably test this on my work machine anyway.

Regardless, test did work.

Awesome!

Edit: Why should demo.md also change everytime tut is run? Isn't this quite annoying?

It is! There are two problems actually: 1) The demo.md file is rendered every time. Just remembered that there is a tut-quick in tut 0.4.7. I should update the dependency, as that would certainly help in some cases. I also thought about making the README more basic so that it doesn’t have to be generated by tut and would take much less time to update, but I am still not sure if not having the samples in the README is a good idea. 2) The demo.md file changes every time it’s rendered. That’s because some objects include the hash when they are printed, and that hash changes across compilation runs. I suppose this problem would be solved with https://github.com/tpolecat/tut/issues/83.

stanch commented 7 years ago

Thanks a lot for your work! Going to issue a new release with this feature.

stanch commented 7 years ago

Download :tada:

sakshamsharma commented 7 years ago

tut is fast enough. Takes about 60 seconds on my laptop.

Or are you saying that you get different output between the two branches on your machine?

No, not that. I meant that when I run tut on your master (no changes of mine), even then all the images get marked as modified by Git. I think it is because of different dot versions.

What if you re-run sbt tut, is it at least deterministic in a given branch?

Yes, running a tut multiple times does not modify the images every time if the branch stays same.

Thanks a lot for your work!

You're welcome!

Going to issue a new release with this feature.

Great! Finally I can remove the source from my project :smile:

stanch commented 7 years ago

Nice! I had a look at https://raw.githubusercontent.com/sakshamsharma/tipsy/master/sample.png and I think you might want to import reftree.contrib.SimplifiedInstances.list ;)

sakshamsharma commented 7 years ago

Aah so that is what I was looking for! Thanks! I had this:

  implicit def listDrawer = ToRefTree[List[ParseTree]] {
    case x => RefTree.Ref(x, x.map(_.refTree)).rename("List")
  }

to do that, but I don't remember whether it worked or not.

Just one more thing, is this the right way to draw my custom case classes?

RefTree.Ref(x, Seq(exp.refTree, op.refTree)).rename("UnaryExpr")
stanch commented 7 years ago

It is the right way, yes. Unless you want to auto-derive that, then no declaration is needed (assuming that instances for each field type also can be auto-derived).