Add Support for Coordinate Conversion

[WilliamHYZhang]

This is going to be an in-progress PR for coordinate conversion. Note that at some points, the PR build is going to fail. Resolves #76

[WilliamHYZhang]

Ok, overhauled all of the annotations to have an array of Coordinate. Coordinate is a new struct defined with Point and coordinateSpace members. draw now passes coordinates[x].point for locations. Nothing changes on the user side.

[WilliamHYZhang]

Ok, now I'm very confused on how I'm supposed to write the function in HelperFunction.swift. I'm passing in an array of Coordinates to the function, but how do I determine the absolute point for each Coordinate without knowing more information? I looked at LineChart.swift to refer to what @KarthikRIyer told me in a previous PR, but now the LineChart fixes PR has overwritten the file.

Some things I am wondering:

• Is origin shifted/unshifted always shift/unshift by a set amount?
• Will the absolute point change dependent on renderer?
• How to find out x and y scaling?

[KarthikRIyer]

@WilliamHYZhang I think currently the origin for the plot bounding box is shifted/unshifted by a set amount. But the origin of the actual plot is calculated at Line 227 of LineChart.swift. The absolute point shouldn't depend on the renderer. The scaling logic should be in the calculateMarkers() function on Line 271 of LineChart.swift if I understand correctly. Why do you have an array of Coordinates in the annotation? Instead replace the location with a Coordinate. Add a coordinate space parameter to the constructor and then you can initialize the location. You can then process the Coordinate accordingly. If you need the available width and height that is calculated in the boundsDidChange() function on Line 216 of LineChart.swift. If you're thinking about the shifting/unshifting of the origin of the plot bounding box, I think you can simply scale the points and draw them using renderer.withAdditionalOffset on Line 473 of GraphLayout.swift.

This is what I could understand by a brief look at the updated code. I haven't had a chance to go through it in detail. @karwa can you please confirm?

[WilliamHYZhang]

@KarthikRIyer thanks for the detailed comments, appreciate it very much. I'll look through the parts of LineChart.swift where you specified.

I was under the impression that this conversion was to be done in GraphLayout.swift. Some annotations don't have location, such as arrow, which has start and end. In order to generalize the conversion we can add these to an array and just have GraphLayout.swift iterate through it and convert all of the coordinates.

[KarthikRIyer]

Some annotations don't have location, such as arrow, which has start and end. In order to generalize the conversion we can add these to an array and just have GraphLayout.swift iterate through it and convert all of the coordinates.

Although this might work, but it will make the code a lot harder to read and understand. I'd recommend having separate variables. For drawing simple stuff like boxes/arrows I don't think the overhead would be too much. In case we come across a situation where we need to draw many polylines/polygons we could think of a something like EBOs in OpenGL. But for now it would be better to keep it simple.

[WilliamHYZhang]

So have a separate case for dealing with arrows? My main concern is that the GraphLayout.swift doesn't know the specific annotation, just the fact that it is of type Annotation. Will we have to downcast it?

[KarthikRIyer]

I'm not sure I understand you correctly. I was thinking could have some HelperFunction that takes in a Coordinate/Coordinates and returns the Point/Points in image space. So we convert the locations or points to image space, do the necessary calculations and use the result to draw the annotations. The type of annotation wouldn't matter. The annotation protocol wouldn't change. We do stuff specific to each annotation inside it's struct.

[WilliamHYZhang]

So this conversion would take place in the specific annotation itself? Previously I was told to convert it in GraphLayout.swift, in which case the type of the annotation isn't known.

[KarthikRIyer]

Previously I was told to convert it in GraphLayout.swift

I might have forgotten or overlooked this discussion. Could you please point me to where this was discussed?

[WilliamHYZhang]

I think it might make sense for us to define a CoordinateSpace enum with similar coordinate spaces, and a Coordinate type which holds a Point and CoordinateSpace. Then the GraphLayout would resolve these in to absolute positions and offsets for the renderer.

From #76

[KarthikRIyer]

Oh ok. @karwa any thoughts?

[karwa]

Hmm... well we need some way to inject the coordinate conversion logic in to the annotation, so it knows where to draw its points.

One idea might be to have a protocol, let's call it a CoordinateResolver, which knows how to convert a Coordinate in to a location for the renderer:

protocol CoordinateResolver {
  func resolve(_ coordinate: Coordinate) -> Point
}

We could have GraphLayout.Results conform to this protocol (since it has all the information about where the plot is drawn inside the figure, etc), and then we pass it in to the Annotation as an extra parameter to the Annotation.draw(Renderer) call.

I think this makes sense: plots shouldn't draw outside of their bounding rectangle, so they're only given a size and don't know about their location inside the figure. Annotations definitely do want to draw inside or outside of the figure, so they need this additional information whereas the plots don't. (of course, plots can draw outside of their rectangle -- we don't clip them. But the border, axes markers, labels, etc might be drawn on top of their data if they do that)

[WilliamHYZhang]

Thank you for your ideas! They are very helpful.

I like the idea of a function to resolve the coordinate, but I'm a bit confused why this has to be a protocol.

we pass it in to the Annotation as an extra parameter to the Annotation.draw(Renderer) call.

Could you provide some psuedocode for how we would implement this part?

[KarthikRIyer]

@WilliamHYZhang I'm not sure how a protocol might help, maybe if we have a protocol we could have a customized implementation if we require it someplace else.

You could have GraphLayout.Result conform to this protocol and as @karwa said it knows everything abouy where the plot is drawn in the figure. In the drawForeground function where you've called the drawAnotations function you could have something like:

drawAnnotations(results: results, renderer: renderer)

In GraphLayout.Results you need to implement the function resolve that will return a Point in the image space based on the data available in the Coordinate. And in draw Annotation you'll resolve each point before drawing like Point a = results.resolve(coordinate: location).

In the resolve function it could be something like this:

struct Results {
...

func resolve(_ coordinate: Coordinate) -> Point{
    switch(coordinate.coordinateSpace){
            case .ndc:
            //code to convert
            return point
            ...

    }
}

}

[karwa]

Yes, basically this. The annotations should have a method:

func drawAnnotations(resolver: CoordinateResolver, renderer: Renderer)

and then we pass in the results object or whatever as our resolver. As for why it's a protocol - because it's useful to abstract the resolving function. It documents exactly what we need from the layout object and allows us to maybe use different resolvers or layout objects in the future.

One thing that we'll need to think about is how we deal with data coordinates. Technically, our chart data is defined in terms of T/U generic parameters, which means the data coordinates should also be generic... whiiiiich also means Coordinate has to be generic, and every annotation which uses a Coordinate would have to be generic. So it's just horrible. You don't need to worry about those right now, but it's something we're going to have to figure out eventually.

We might eventually have to make Coordinate a protocol. That could work... 🤔

[WilliamHYZhang]

Alright, I'll try to scrape together a proof of concept first, and then we can work on the details with the coordinate dealing stuff.

[WilliamHYZhang]

Tried to implement the basic structure of the idea, however builds failing saying there are undeclared types, is this because I defined them in another file? Confused with these errors, don't know what I'm doing wrong.

Also, how would the arguments/variables used in calculateMarkers correspond the variables that we have access to in Results?

From what I've been able to gather, it seems as if x, y corresponds to coordinate.point, however I don't know how xLabels and yLabels should correspond to, they are arrays of string and I don't see similar in Results, perhaps LabelSize? Finally, for the bounds in LineGraph, would it correspond to totalSize maybe?

Thanks for clarification.

[karwa]

With regards to how to calculate the coordinates:

• totalSize is the entire size of the figure. Renderer coordinates go from 0..<totalSize.width/height. Anything outside of that may/may not be drawn, or may be clipped/overlapped/whatever.
• plotBorderRect tells you where the plot is in renderer coordinates, and how big it is. So if you have a plot-relative coordinate, it should be offset by the origin.

Referring to the coordinate spaces in matplotlib:

'figure points'   : points from the lower left corner of the figure
'figure pixels'   : pixels from the lower left corner of the figure
'figure fraction' : 0,0 is lower left of figure and 1,1 is upper, right
'axes points'     : points from lower left corner of axes
'axes pixels'     : pixels from lower left corner of axes
'axes fraction'   : 0,0 is lower left of axes and 1,1 is upper right
'offset points'   : Specify an offset (in points) from the xy value
'offset pixels'   : Specify an offset (in pixels) from the xy value
'data'            : use the axes data coordinate system

Using this, you should be able to implement 4 of these relatively easily ({figure/axis}{points/fraction}). Figure points don't need any transformation and axes points only need the offsetting described above.

[WilliamHYZhang]

Regarding the coordinate space, I thought we only had two? .ndc and I don't know the name of the second one. Is this referring to the origin shifted/unshifted?

Update: Fixed build errors and, all there is left to do is implement the resolver.

[WilliamHYZhang]

Implemented the resolve function cases for both figurePoints and axesPoints, however @karwa may you please elaborate on the different between a point and a pixel for the figurePixels and axesPixels that I'm also planning to implement. Thank you!

The most important coordinate case, however, is data. This is so that we can pass in coordinate system data to plot the annotations in. However, how can we accurately scale if we only know totalSize and plotBorderRect?

[karwa]

We don't really have a distinction between points and pixels at the moment. Typically, points are the high-level coordinates that you use to lay things out, and then they get converted to pixels based on how you render the image (e.g. if you render at 2x, like for a Retina display, each 1x1 square of points contains 4 pixels).

You don't need to worry about data coordinates right now IMO - that's significantly more complicated, because our data is defined using generic types. GraphLayout only cares about things like the axes and labels, so it can give the containing Plot object a good size to lay out/draw in; it doesn't know anything about what kind of data is being drawn, or the scale, or anything like that.

Eventually we'll need the Plot objects themselves to resolve data coordinates. But as I said, it gets complicated due to generics. We might end up having a separate DataCoordinate<X, Y> type and making Coordinate a protocol, or we might do something different. That part of the API needs some careful thought and experimentation.

[WilliamHYZhang]

Ah, ok. I'll leave the current cases in this PR as they are. This skeletal coordinate implementations should be good to merge once the build fix PR is merged.

In the meantime, is there any way we can scale the points just for the annotations, so we don't have to essentially "guess and check" to find the correct locations to pass?

[karwa]

In the meantime, is there any way we can scale the points just for the annotations, so we don't have to essentially "guess and check" to find the correct locations to pass?

Do you mean for the points/pixel thing? I think we should just leave that out for now. We can add it once it's actually supported, but for now there isn't any point to adding it.

The only changes I would make to this PR are to implement the *fraction coordinate spaces and remove the data coordinate space. It's fine for a personal development branch to have things like that temporarily, but the master branch should just have things that work and are supported. Other than that, it looks great 👍

[WilliamHYZhang]

Do you mean for the points/pixel thing? I think we should just leave that out for now. We can add it once it's actually supported, but for now there isn't any point to adding it.

I was referring to a workaround of the data coordinate type resolver, since as of now we're still passing in absolute locations to set up the annotations, which isn't ideal as it results in a lot of guesswork to figure out the absolute locations relative to the plot coordinates on the user side.

I'll update the PR with the fraction coordinate spaces and remove data coordinate space tomorrow hopefully after the build tests start passing again.

[boronhub]

@WilliamHYZhang have you tried merging the latest commits ? It allows the tests to pass.

[WilliamHYZhang]

Yep, thanks @karwa for test fixes.

[WilliamHYZhang]

Alright, added resolving for fraction support as well. @KarthikRIyer @karwa should be good to merge on your review.

[KarthikRIyer]

Looks good @WilliamHYZhang ! Thanks for your work! Merging.