Add functions to get base64 png image from AGGRenderer

[KarthikRIyer]

#13

[KarthikRIyer]

This string displays a message "Use AGG Renderer to get base64 image"

[marcrasi]

This is a good thing to include as a comment in the code, so that future code readers know what this string is :)

(This is probably irrelevant, because of my suggestion to delete this method.)

[marcrasi]

I suggest not adding this as a function in the protocol, so that you don't have to define implementations in renderers that do not support it, and so that users can statically see (e.g. in the autocomplete suggestions for a renderer) whether it supports it.

Extra thoughts. No need to do any of these in this PR:

Actually, I have the same opinion about drawOutput -- it shouldn't be in the protocol, so that renderers that don't support saving output to a file don't have to implement it.

Of course, if you do this, then it won't be possible to have methods like LinePlot.drawGraphAndOutput(filename: String, renderer: Renderer), because that function won't have access to a renderer.drawOutput method. I don't think this is a big problem -- for now, the user can just do:

let renderer = ...
let plot = ...
plot.drawGraph(renderer: renderer)
renderer.drawOutput(filename: "abc")

Then later, there are ways to improve the api so that the user doesn't have to make so many calls to draw a graph and save it to a file. One idea: make a protocol for plots

protocol Plot: {
  func draw(on renderer: Renderer)
}

Then, it's possible to write functions that "draw and save" plots using renderers. e.g.

// maybe this is a top-level function inside swiftplot
func saveAGGPng(for plot: Plot, to filename: String) {
  let renderer = AGGRenderer()
  plot.draw(on: renderer)
  renderer.save(to: filename)
}

// maybe this is a function inside EnableIPythonDisplay in swift-jupyter
func display(plot: Plot) {
  let renderer = AGGRenderer()
  plot.draw(on: renderer)
  let base64Png = renderer.base64Png()
  ... logic to show the png on jupyter ...
}

[marcrasi]

ooo even better, those "saveAGGPng" functions could be methods in a protocol extension:

extension Plot {
  func saveAGGPng(to filename: String) { ... }
}

extension Plot {
  func display() { ... }
}

so that the user can just do

let plot = ...
plot.saveAGGPng(to: "abc")
plot.display()

[KarthikRIyer]

Regarding the drawOutput function, I don't think the name indicates that the renderer need to save the image. So maybe if we have OpenGL the code inside this method can draw the output to a window on the screen?

[marcrasi]

The argument indicates that it's a function to save an image though, and what if other renderers need different types of arguments to draw their output (e.g. something that draws to a screen needs a screen position)?

Stepping back a bit, I think that the concepts of passing configuration to the renderer (e.g. the filename to save to) and deciding how to receive the image (e.g. as a saved file or as a returned string), should be part of the specific renderer implementation, not part of the abstract renderer interface. This way, renderer implementations have complete freedom to define how exactly they are going to draw their plots, without being constrained by existing methods in the renderer interface.

[marcrasi]

Does agg have an api that gives you data, so that you do not need to save it to disk and then read it to disk?

If you really do have to save it and then read it, could you generate a temporary filename automatically so that the user does not have to specify a filename? Also, would it be possible to do this temporary filename generation, and file reading, and base64 encoding, in Swift? Foundation might have APIs that make it easyish (https://nshipster.com/temporary-files/). Not sure if they're supported on Linux Foundation, but they probably are because most of Foundation works on Linux nowadays.

[KarthikRIyer]

To get the base64 we need a properly encoded png file. I'm saving pngs using lodepng, so I do not know what goes on under the hood. So I think we'll need to save a file to the disk.

Regarding doing the base64 encoding in Swift, I'll give it a try.

[BradLarson]

For lodepng, would lodepng_encode_memory() allow you to do a direct-to-memory encoding, instead of writing to a file?

[KarthikRIyer]

Thanks a lot @BradLarson
This worked! Now we won't have to save it to the disk.

[KarthikRIyer]

Now that we don't need to save it, is it fine if the base64 encoding takes place in cpp itself?

[BradLarson]

Base64 encoding seems like something that might be used elsewhere (for encoding from other renderers), so I could see an argument for leaving that in Swift. As Marc said, it's relatively simple to do in Swift by leveraging Foundation:

https://stackoverflow.com/a/35360697/19679

and I'm pretty sure that all works on Linux and other platforms.

[marcrasi]

Yay that we don't have to save it to disk! I do still think that doing the encoding in Swift would be the best. It'll be less code, which is usually a very good thing, because it's easier to maintain less code.

[KarthikRIyer]

Shifted it to Swift and updated PR.
This worked: https://stackoverflow.com/a/11251478/9126612

[marcrasi]

This is a good thing to include as a comment in the code, so that future code readers know what this string is :)

(This is probably irrelevant, because of my suggestion to delete this method.)

[KarthikRIyer]

The last commit actually introduced a bug. I'll correct it.

[KarthikRIyer]

I've updated the AGG Renderer code to support SubPlots.
I also found there was a problem in changing the plot size in SVG too. I've solved the SVG probem.
I still haven't been able to solve the problem with AGG, i.e. to change the plot size. At the moment AGGRenderer also has a memory leak problem.

[KarthikRIyer]

Also I'd like to test the current work in Jupyter notebook and also add an example for the same, for which I'll have to add a tag. So, can I do that? If so, what should I use, because obviously this isn't an actual release.

[KarthikRIyer]

@marcrasi @BradLarson could you please review?

[BradLarson]

Sorry for the slow review, it's a holiday here in the U.S., and the weather has been particularly nice this weekend.

In general, I'm fine with the additions here. However, I'd suggest a reworking of the package arrangement in a future pull request. Rather than having LinePlot, etc. as separate modules, they really should be under one SwiftPlot module for the entire framework. That SwiftPlot module can have the renderers, etc. be dependencies of it, and then all the examples should just need the SwiftPlot module as a single dependency.

Alternatively, the SwiftPlot module could have everything but the specific Renderers, with each Renderer (and all C or C++ submodules) being an individual module that you import separately in addition to SwiftPlot. That might allow for more modularity, with people being able to define their own Renderers that are outside of the framework, yet that comply with the Renderer protocol.

[KarthikRIyer]

That's alright. Do you mean Memorial Day?
The weather in India's scorching hot, with temperatures going up to 44℃! You just don't feel like getting out of the house.

Thanks for the review. I too have been thinking it would be nice to have to just import SwiftPlot. I'll make the changes in the next PR.

[BradLarson]

@KarthikRIyer - My thinking is that the generic Renderer interface would be part of the SwiftPlot module. Specific instances of Renderers (AGGRenderer, SVGRenderer) could potentially be modules outside of SwiftPlot, and they would depend on SwiftPlot to build (and use import SwiftPlot within them).

Whether or not to have external renderers as separate modules or as part of SwiftPlot is something I go back and forth on. It may depend on how Renderers are selected by the end user. Do we want them to manually import AGGRenderer when they want to use that?

We're going to need to have conditional inclusion of Renderers based on platform, because on iOS we may not want the weight of an AGGRenderer, and if we create a CGRenderer (for Core Graphics) it wouldn't be supported on non-Apple platforms. This could be done, even if these were part of the framework, by using conditional compilation (#if os(iOS), etc.), but I don't know if we want to be more explicit about that.

[KarthikRIyer]

I think if having separate modules for each renderer does give us the modularity to implement renderers not included in the package, we should go for that. In the end, the user will have to write something like
var svg_renderer : SVGRenderer = SVGRenderer() and pass it to the plot to draw. So why not write an extra line to import it...