Can PointProvider.getLargestPoint be removed?

[Tyler-Lopez]

Edit 2:

I've separated out the layer padding changes to use an ExtraStore here #941

Edit:

I have thought on this some, and I am thinking the ideal would be that if CartesianLayerPadding could (doesn't always have to be, but the capability should be there, I think) provided using ExtraStore. Then, I think that getLargestPoint as it pertains to determining horizontal or vertical insets should be removed (it may still be needed for point spacing) and incorporated into CartesianLayerPadding. This is something I am actively considering, but these are my thoughts at the moment. The rationale here is the incorrect padding included in this message and also that we won't always have points symmetrically at the start, end, top, and bottom. Maybe we just need to pad the point on the end or top but not the start or bottom.

Original Message

Hi there!

I would like to propose that PointProvider.getLargestPoint be removed in favor of consolidating it into CartesianLayerPadding. I may be missing something here, and haven't given this too much thought from the implementation perspective. I did think this was interesting though and wanted to share for thoughts and if there was anything here I wasn't considering.

I would note here that I recognize this is putting the responsibility of the client to know how much they should inset the chart by for points. Given the change to remove the automatic Segmented HorizontalLayout, I wonder if that is ok and perhaps even a path further in the direction the maintainers are going for?

Examples

In these example, CartesianChartHost is given a background color of Gray so we can see its borders. The points are all 24.dp big, and the line thickness is 24.dp.

You would thus expect that we need to have the chart inset 12.dp as for both the points and the line, that is how much will stick out.

Whenever I say "not PointProvider.getLargestPoint" what I mean is that I am setting it to return null.

Case 1: With CartesianLayerPadding AND PointProvider.getLargestPoint

There is too much space by 12.dp because the additional padding added by the point and the additional padding added by CartesianLayerPadding are cumulative to 24.dp.

Case 2: With PointProvider.getLargestPoint AND NOT CartesianLayerPadding

There is a perfect amount of space, because CartesianLayerPadding would have been 12.dp and PointProvider.getLargestPoint provides that.

Case 3: With neither CartesianLayerPadding OR PointProvider.getLargestPoint

The points are clipped, and the line also is not given the appropriate room.

Case 4: With CartesianLayerPadding AND NOT PointProvider.getLargestPoint

There is a perfect amount of space, because PointProvider.getLargestPoint would have been 12.dp and CartesianLayerPadding provides that.

[Tyler-Lopez]

I do notice here it is necessary for the vertical inset, which is interesting - so that isn't covered here.

vico/vico/core/src/main/java/com/patrykandpatrick/vico/core/cartesian/layer/LineCartesianLayer.kt

Lines 594 to 611 in 7066e4b

	override fun updateInsets(
	context: CartesianMeasuringContext,
	horizontalDimensions: HorizontalDimensions,
	model: LineCartesianLayerModel,
	insets: Insets,
	) {
	with(context) {
	val verticalInset =
	(0..<model.series.size)
	.mapNotNull { lineProvider.getLine(it, model.extraStore) }
	.maxOf {
	max(it.thicknessDp, it.pointProvider?.getLargestPoint(model.extraStore)?.sizeDp.orZero)
	}
	.half
	.pixels
	insets.ensureValuesAtLeast(top = verticalInset, bottom = verticalInset)
	}
	}

Would it ever be considered to move this vertical inset into the layer padding object?

For now, for my purposes, I am thinking that I will have getLargestPoint always return null and then factor it into my layer padding calculations. I will also look into overriding that vertical inset function. One other thing is that it is might want to, instead of setting the vertical inset based on the "largest point", look for the point at the largest Y-value specifically and use that.

Imagine a chart that just goes down with a point at the end, I would not want to factor that point into the vertical inset, is what I'm thinking.

[Tyler-Lopez]

I have a WIP PR here to address this issue which retains getLargestPoint per its requirement to derive point spacing, but which would allow for manual top and bottom padding.

[Gowsky]

Hello, @Tyler-Lopez.

The rationale here is the incorrect padding included in this message

Could you elaborate? While I understand that the extra padding is unwanted, you're the one adding it. getLargestPoint adds 12 dp, and you add another 12 dp via CartesianLayerPadding, so you end up with 24 dp. Why add the extra 12 dp in the first place if it's not desired? As you've demonstrated, you can avoid doing so, and then you get the desired padding of just the 12 dp required by the point.

and also that we won't always have points symmetrically at the start, end, top, and bottom. Maybe we just need to pad the point on the end or top but not the start or bottom.

Is this referring cases like the one you mentioned here — when there's no point along a particular edge that would require an inset?

look for the point at the largest Y-value specifically and use that.

This could cause the insets to change during data updates, as when the y values change, a different point may end up having the largest y value. Such a change in the insets would cause the chart to suddenly resize. Wouldn't that look somewhat unpleasant?

Would it ever be considered to move this vertical inset into the layer padding object?

Insets and padding are different. Specifically, insets are outside of the CartesianLayers (they're like margins), and padding is inside. Currently, there's horizontal padding (customized via CartesianLayerPadding), horizontal insets, and vertical insets. We plan on adding vertical padding (also customized via CartesianLayerPadding), which will be useful for data labels.

External inset customization could be added, but then it should be done for both horizontal and vertical insets, and most likely via a new API, not CartesianLayerPadding. This can be considered once the use case has been clarified.

[Tyler-Lopez]

Hi @Gowsky! Thanks so much for your thoughts and questions here! I also want to apologize, between this discussion and the two PR my thoughts here might be a bit of a mess.

Could you elaborate? While I understand that the extra padding is unwanted, you're the one adding it.

Agreed. As such, I have a nice solution that works for the particular (and common) case where a line series has the same point at the start x-value and the end x-value. The solution involves simply considering the size of the point when deriving CartesianLayerPadding. This is already something the clients can handle on their own by simply not providing any padding if the point is so large that its radius is > than the line thickness halved. Vico works great here!

I do think needing to know that getLargestPoint is adding padding under the hood is a touch weird from the client perspective. It seems rationale to me that, since you have decided to make clients provide their own padding for the line thickness, that it would be consistent for clients to provide their own padding for the points.

However, because getLargestPoint applies padding symmetrically - even if that point isn't near one or both of the edges, it makes that awesome client discretion afforded through CartesianLayerPadding not work.

when there's no point along a particular edge that would require an inset?

Exactly!

Here is a quick attempt at a visualization of my understanding here:

I have a line chart with a point at the second x-value. Why would I have this? This would be because sometimes in chart designs points are used as an indicator of other information, like denoting this is a special data point.

I want the chart to be bounded by the green lines, however, getLargestPoint is invoked under the hood and pushes every axis out by the red line, which is the radius of the largest point. This results in the actual bounds being the pink lines, and there is nothing that I can do, as a client, (to my understanding) to prevent this.

I will note that, in constructing this message, I am realizing that I think I could hack this manual control over insets a little bit by not using a line's point provider but instead using persistent markers. That has the problem, of course, of always being atop all of the data, though.

Such a change in the insets would cause the chart to suddenly resize. Wouldn't that look somewhat unpleasant?

I need to chew on this a bit more, but my first thought here is: yes, it could. However, with the point-padding exposed to clients, it could be up to their discretion whether to dynamically change the vertical inset (which could look unpleasant, but more tightly and accurately bound stagnant data) or to keep it consistent. Giving this discretion to the clients is the main thing I'm after here.

This can be considered once the use case has been clarified.

Let me know if you have any more questions I could help with! I'll chew more on the point about insets/padding.

[Gowsky]

It seems rationale to me that, since you have decided to make clients provide their own padding for the line thickness, that it would be consistent for clients to provide their own padding for the points.

The lack of automatic behavior preventing lines from being trimmed at the start and end is an oversight. We'll look into adding it. It's already in place at the top and bottom: insets equal to half of the line thickness are added to prevent clipping.

I will note that, in constructing this message, I am realizing that I think I could hack this manual control over insets a little bit by not using a line's point provider but instead using persistent markers.

A persistent marker generally seems more appropriate for this use case. It's not exactly a hack.

That has the problem, of course, of always being atop all of the data, though.

To clarify, is this an actual requirement or a hypothetical one? It seems rather obscure, but perhaps I'm missing a common use case.

Giving this discretion to the clients is the main thing I'm after here.

The question is whether this justifies removing the automatic padding, which is desired in the majority of cases. Wouldn't it be better to add the ability to customize it without entirely removing the automatic behavior? This would let you achieve your desired result without making the API harder to use in more common cases.

[Tyler-Lopez]

To clarify, is this an actual requirement or a hypothetical one?

The requirement to have points of variable appearance/size along a line convey meaningful information is an actual requirement. It is a frustrating design, in my opinion - but a requirement none the less. Vico also supports it very well, other than the padding (in my experience) with the pattern of the point provider.

Wouldn't it be better to add the ability to customize it without entirely removing the automatic behavior? This would let you achieve your desired result without making the API harder to use in more common cases.

Would love this! The direction I proposed here regarding entirely removing the automated padding was out of my understanding that you intended to remove the automatic horizontal padding with the line thickness, and so it seemed that removing the automatic point padding would be consistent with that.

[Gowsky]

The requirement to have points of variable appearance/size along a line convey meaningful information is an actual requirement.

I was referring to having a single point while also having an additional layer that's supposed to overlap the point. You mentioned that the solution with the persistent marker (which is the most natural approach for a single point) has the issue of "always being atop all of the data." I was wondering if this fact is getting in the way of an actual use case.

Would love this!

Then we'll take this approach. We'll check how it relates to your PRs and comment there.