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

Line renders both `topFill` & `bottomFill` colors for negative data #835

Open mak1nt0sh opened 1 month ago

mak1nt0sh commented 1 month ago

How to reproduce

fill = LineCartesianLayer.LineFill.double(
                            topFill = fill(MaterialTheme.colorScheme.primary),
                            bottomFill = fill(MaterialTheme.customColors.negative)
                        )

Data for this view:

xSeries = [19936, 19937, 19938, 19939, 19940, 19941, 19942]
ySeries = [500.00, -5, -5, -5, -5, -25, -25]

Observed behavior

lineissue

Line colors are split and default marker mixes the colors of both fill colors

Expected behavior

The color of the line and marker should be distinct for positive/negative values.

Vico version(s)

2.0.0-alpha.27

Android version(s)

Any

Gowsky commented 1 month ago

Hi @mak1nt0sh, you seem to describe an additional issue here compared to your comment. The comment mentioned the marker problem, but here you've also described that the y = -5 line segment has split colors. With a single horizontal split it's geometrically impossible for it to never occur as the line has some thickness.

Here's the current state. image

For the y = -5 line segment to not be split we'd have to shift the split upward. But then we'd be back to the problem described in #784. image

A possible solution would be to split the line vertically. Is it what you expect? image

mak1nt0sh commented 1 month ago

Can't it always draw above or below the threshold line? Anything above or equal to the splitY value would push the line to draw on the border and if <splitY draw below?

Sorry I'm on my phone rn, can't draw these beautiful sketches <3

Edit:

I guess it would require shifting the line by small offset equal to half the line width?

mak1nt0sh commented 1 month ago

Also the #809 is still not fixed.

Screenshot 2024-08-09 at 14 38 56

The bottom (negative line) is completely drawn with topFill.

Data for this view:

[19936, 19937, 19938, 19939, 19940, 19941, 
[500, 500, 500, -2, -2, -2, -2, -2, -202]
Gowsky commented 1 month ago

Can't it always draw above or below the threshold line? Anything above or equal to the splitY value would push the line to draw on the border and if <splitY draw below?

In this case, the position of the line wouldn't reflect the data and there would be misalignments with axis guidelines, HorizontalLines, other layers, etc.

Also the https://github.com/patrykandpatrick/vico/issues/809 is still not fixed.

I believe it is.

The bottom (negative line) is completely drawn with topFill.

It's not. If you zoom into the line you can see it starts fading to red.

You don't see the pure red, because the sliver is less than a pixel in height. In a high enough resolution, you'd see this.

It's the same story as above. The current state. image

Again, for the y = -2 line segment to be fully red we'd have to move the split upwards and we would be back to the issue #784. image

Once again, it would require vertical splits.

mak1nt0sh commented 1 month ago

I just checked a few apps like Yahoo finance and CoinMarketCap and it seems they both have different rendering ways. I guess I'll leave it up to you to decide the best way to go forward.

Yahoo seems to render by direction and stack the lines:

markup_1000000751.png

CoinMarketCap splits by x:

Screenshot_20240810-192919.png

mak1nt0sh commented 3 weeks ago

Is more information needed? (I can see the label for this issue). I'm currently stuck on 2.0.0-alpha.22 due to this issue.

Gowsky commented 3 weeks ago

Sorry for the wait. Regarding the color mixing problem, we have all the required information for now. We will look into fixing this.

Regarding the edge case with the splits:

Yahoo seems to render by direction and stack the lines:

This is slightly suboptimal, because with round caps, the split positioning is a bit imprecise, and with butt caps gaps can appear.

CoinMarketCap splits by x:

The charts don't seem to be zoomable, or pannable so we weren't able to see how the splitting works. But we checked TradingView and it seems to use vertical splits.

vertical splits

This is what I mentioned above, and it seems to be the best choice. We'll look into implementing this. However, this is an enhancement and not a bug fix - AreaFill.double now works as documented, even if it’s not suited for the edge case in question. (Note that horizontal splits have always been used, so the problem with the edge case affects all versions released to date, including 2.0.0-alpha.22, although it might occur in slightly different instances.) Therefore, we’ll use this issue to track the color mixing problem, and we’d ask you to open a discussion in the “Ideas” category for the switch to vertical splits. It’s fine to just link this issue.