patrykandpatrick / vico

A light and extensible chart library for Android.
https://patrykandpatrick.com/vico/wiki
Apache License 2.0
2.07k stars 125 forks source link

`LineCartesianLayer.PointProvider` async from chart rendering #833

Closed mak1nt0sh closed 1 month ago

mak1nt0sh commented 1 month ago

How to reproduce

  1. Set up a CartesianChartHost with a LineCartesianLayer and use point provider
  2. Set the pointProvider to display when data contains only one point else null
  3. Render the chart with a new dataset containing multiple points
@Composable
private fun rememberPointProvider(data: List<Float>): LineCartesianLayer.PointProvider? {
    return remember(data) {
        if (data.size == 1) {
        // Return a point provider for single data point
    LineCartesianLayer.PointProvider.simple(
                shape = LineChartPoint(),
                size = 6f,
                color = MaterialTheme.colorScheme.primary.toArgb()
            )
        } else {
  // Return null if there's more than one data point
            null
        }
    }
}

Observed behavior

When the point provider is set to null, the chart renders asynchronously and incorrectly displays points on all data points from the previous dataset.

https://github.com/user-attachments/assets/3fce82ec-7e66-42a9-a854-00b6f2cc1dc0

Expected behavior

The chart should render correctly with the new dataset, without showing points from the previous dataset.

Vico version(s)

2.0.0-alpha.27

Android version(s)

Any

Additional information

No response

Gowsky commented 1 month ago

@mak1nt0sh this is an incorrect usage of the API. As described in the wiki data updates are handled asynchronously and you shouldn't directly use the data passed to the latest Transaction to perform chart configuration. For this use case, you should create a custom PointProvider implementation with conditional behavior based on the arguments of getPoint and getLargestPoint. This will involve using an extra that stores your condition.

mak1nt0sh commented 1 month ago

Got you. It works using extraStore, however the Api of custom LineCartesianLayer.PointProvider is a bit confusing. If you can please add more documentation on public fun getLargestPoint(extraStore: ExtraStore): Point?

For everything to work I have to pass something into getLargestPoint cause if I pass null the point renders, but is split in half:

Screenshot 2024-08-09 at 10 16 36

And for some charts without axis defined it renders only quarter of the point:

Screenshot 2024-08-09 at 10 17 39
@Composable
private fun rememberPointProvider(showPoint: ExtraStore.Key<Boolean>): LineCartesianLayer.PointProvider {
    val positiveColor = MaterialTheme.colorScheme.primary
    val negativeColor = MaterialTheme.customColors.negative

    return remember {
        object : LineCartesianLayer.PointProvider {
            override fun getLargestPoint(extraStore: ExtraStore): LineCartesianLayer.Point? {
                return if (extraStore[showPoint]) {
                    LineCartesianLayer.Point(
                        component = ShapeComponent(
                            shape = Shape.Pill,
                            color = Color.TRANSPARENT
                        )
                    )
                } else null
            }

            override fun getPoint(
                entry: LineCartesianLayerModel.Entry,
                seriesIndex: Int,
                extraStore: ExtraStore
            ): LineCartesianLayer.Point? {
                return if (extraStore[showPoint]) {
                    LineCartesianLayer.Point(
                        component = ShapeComponent(
                            shape = Shape.Pill,
                            color = if (entry.y < 0) negativeColor.toArgb() else positiveColor.toArgb()
                        ),
                        sizeDp = 6f
                    )
                } else null
            }
        }
    }
}
Gowsky commented 1 month ago

however the Api of custom LineCartesianLayer.PointProvider is a bit confusing. If you can please add more documentation on public fun getLargestPoint(extraStore: ExtraStore): Point?

What do you need to be clarified? The documentation says that it's supposed to return the largest Point.

For everything to work I have to pass something into getLargestPoint cause if I pass null the point renders, but is split in half:

Yes, because then you're not returning the largest Point.

Also, you forgot to specify the Point size in getLargestPoint. And it's not good for performance to return a new Point for every function invocation. Here's the fixed code.

@Composable
private fun rememberPointProvider(
  showPoint: ExtraStore.Key<Boolean>
): LineCartesianLayer.PointProvider {
  val positivePoint =
    rememberPoint(
      component = rememberShapeComponent(Shape.Pill, MaterialTheme.colorScheme.primary),
      size = 6.dp,
    )
  val negativePoint =
    rememberPoint(
      component = rememberShapeComponent(Shape.Pill, MaterialTheme.customColors.negative),
      size = 6.dp,
    )

  return remember {
    object : LineCartesianLayer.PointProvider {
      override fun getLargestPoint(extraStore: ExtraStore): LineCartesianLayer.Point? =
        if (extraStore[showPoint]) positivePoint else null

      override fun getPoint(
        entry: LineCartesianLayerModel.Entry,
        seriesIndex: Int,
        extraStore: ExtraStore,
      ): LineCartesianLayer.Point? =
        when {
          !extraStore[showPoint] -> null
          entry.y >= 0 -> positivePoint
          else -> negativePoint
        }
    }
  }
}
mak1nt0sh commented 1 month ago

Thanks! Regarding the documentation, I found it hard to understand why it requires to return the largest point at all. Wouldn't it always be for the largest Y value? Or is it the largest point by size? This is what I meant.

Gowsky commented 1 month ago

Thanks for the feedback, @mak1nt0sh. It's the Point that's the greatest in size. I don't think it would be accurate to interpret "the largest Point" in any other way.

In the case of the Point with the greatest y value, it's the y value that would be the largest, not the Point. The documentation refers to the Point itself as the largest. Besides, a Point doesn't have any intrinsic y value — it only stores style information. A single Point can be used at multiple y values.

getLargestPoint is used to determine the padding required by the LineCartesianLayer for the points not to be clipped, as in the image you shared. Without this function, all points would have to be checked, even those that are outside of the viewport and are skipped during drawing. This would result in worse performance.

This is omitted in the documentation because it's irrelevant to the caller. All the caller has to do is respect the contract and return the largest Point. The LineCartesianLayer will use the returned value at its own discretion.

By the way, we briefly saw a comment from you about an ExtraStore crash. It now seems to be gone. As you probably realized, the problem resulted from incorrect ExtraStore.Key management on your end. The ExtraStore.Key could be recreated while the CartesianChartModelProducer remained unchanged. Two related things:

  1. Based on the code above, it looks like you're passing the ExtraStore.Key around. It may be easier to move it to the file level or to an object. You'll then be able to access it more easily, and you'll have no risk of incorrectly recreating it.
  2. It's best to run Transactions from the same level at which the CartesianChartModelProducer is stored. For example, if the CartesianChartModelProducer is in the ViewModel, you should feed it with data from there, not from the composable where CartesianChartHost is used.
mak1nt0sh commented 1 month ago

Thanks for the clarification! It might be beneficial to include a broader KDoc explanation that explicitly mentions how getLargestPoint is used for padding within the LineCartesianLayer. This would provide more context to developers, especially those unfamiliar with the internal workings, and help clarify the function's role beyond just returning the largest Point.

What do you think?

Gowsky commented 1 month ago

I’ve already addressed this above. Asking ChatGPT to paraphrase your point with no consideration for my response isn’t going to move the conversation forward. The rest of this message was written by ChatGPT, but after it was properly prompted.

While I understand the intention behind including additional context in the KDoc, doing so might actually detract from the clarity and purpose of the interface function. The key objective of the getLargestPoint function is to adhere to its defined contract—returning the largest Point—without making assumptions about its usage. Providing details on how it is currently utilized within LineCartesianLayer would blur the line between what the function guarantees and how it happens to be employed at the moment.

Such details are internal and subject to change; exposing them could lead to unintended dependencies or misuse by developers who might start to rely on this specific implementation context. Maintaining a clear separation ensures the function remains flexible and true to its contract, allowing for future adaptability.

Ultimately, the focus should remain on what the function delivers, not how it’s consumed.

mak1nt0sh commented 1 month ago

Understood, I get a bit passive aggressive vibes from you. Regarding GPT I frequently use it, cause English is not my main language and I have limitations expressing my thoughts concisely.

As I had to use the getLargestPoint in my own code base, as it is a public for all, I simply wanted to understand the point of this function and suggested to make its purpose more clear through documentation.

I have no intention to question your workflow, just wanted to make a point as this is a public project and some developers may benefit from this.

Gowsky commented 1 month ago

I’m open to your feedback and have nothing against ChatGPT as such, but please ensure that you’re addressing my comments, not just repeating your initial suggestion.

I understand what you’re suggesting and that you believe it would help clarify the function’s purpose. My point is that there’s a clear contract, and the consumer shouldn’t be concerned with implementation details that are subject to change. You can always find out how LineCartesianLayer uses getLargestPoint by checking the source code. The function’s documentation includes information that is relevant during implementation. This is the standard for documentation of interfaces, which define contracts by definition.

Could you please explain under what circumstances the consumer would require this information? The largest Point should be returned either way. From the consumer’s point of view, what difference does it make how it’s used?