tradingview / lightweight-charts

Performant financial charts built with HTML5 canvas
https://www.tradingview.com/lightweight-charts/
Apache License 2.0
9.47k stars 1.63k forks source link

Issue with Weight Calculation for Second Rendered Label on Time Axis #1689

Open benjamindamm opened 2 months ago

benjamindamm commented 2 months ago

Problem Statement The current implementation of the fillWeightsForPoints function in the lightweight-charts library has a flaw that affects the correct calculation of tick mark weights, particularly for the second rendered label on the time axis. When prevTime is null, the fallback mechanism can result in totalTimeDiff being zero for the first point. This leads to an incorrect calculation of averageTimeDiff, which in turn causes inaccurate weight assignments for subsequent labels.

Issue Details In particular, this issue manifests when the time difference for the first label is estimated. The current logic can cause the label to be positioned incorrectly, potentially appearing in an arbitrary or unintended location on the time axis. This behavior is especially problematic when precise labeling is required, as it can result in misleading or confusing chart outputs.

Steps to Reproduce Render a chart with a minimal number of time points, particularly focusing on the first few labels. Observe how the second label is rendered in relation to the first label. Note that the weight assigned to the second label may not reflect the intended time interval, leading to incorrect positioning.

Proposed Temporary Solution To address this, I implemented a conditional check that ensures the time difference is only calculated when prevTime is not null. This change forces the first label to always be displayed with a weight that corresponds to a year. While this is not a comprehensive solution and may not be appropriate for every case, it highlights the underlying issue and prevents the most egregious positioning errors.

I created a pull request for this: https://github.com/tradingview/lightweight-charts/pull/1688

Here you can find a Stackblitz which reproduces the issue: https://stackblitz.com/edit/stackblitz-starters-zzpxjk?file=src%2Fapp%2Fapp.component.ts

Screenshot 2024-09-03 at 16 38 10
illetid commented 2 months ago

Hi, it is possible to address this issue with some additional code from the user side. So at first you need to extend DefaultHorzScaleBehavior and implement shouldResetTickmarkLabels


import {
    TickMark,
    defaultHorzScaleBehavior,
} from 'lightweight-charts';

const DefaultHorzScaleBehavior  = defaultHorzScaleBehavior();

export class LightweightChartHorzScaleBehavior extends DefaultHorzScaleBehavior {
    private _lastHash: string = '';

    public override shouldResetTickmarkLabels(items: TickMark[]): boolean {
        const itemsHash = this.calculateItemsHash(items);
        const res = itemsHash !== this._lastHash;
        this._lastHash = itemsHash;
        return res;
    }
    private calculateItemsHash(items: TickMark[]): string {
        return items.reduce((hash: string, item: TickMark) => hash + item.index, ''); // Simplified hash logic
    }
}

then you need to use createChartEx and pass there your changed LightweightChartHorzScaleBehavior

// use LightweightCharts.something or just import it like import {createChartEx} from 'lightweight-charts'
const chart = LightweightCharts.createChartEx(container, new LightweightMiniChartHorzScaleBehavior(), {});
function convertTime(t) {
    if (LightweightCharts.isUTCTimestamp(t)) {
        return t * 1000;
    }
    if (LightweightCharts.isBusinessDay(t)) {
        return new Date(t.year, t.month, t.day).valueOf();
    }
    const [year, month, day] = t.split('-').map(tm => parseInt(tm, 10));
    return new Date(year, month, day).valueOf();
}

const converted = new Map();
function convertTimeWithMemory(t) {
    const existingAnswer = converted.get(t);
    if (existingAnswer) {
        return existingAnswer;
    }
    const answer = new Date(convertTime(t));
    converted.set(t, answer);
    return answer;
}
let lastDate;
let lastTickMarkType;
chart.timeScale().applyOptions({
    tickMarkFormatter: (time, tickMarkType) => {
        let answer = null;
        const date = convertTimeWithMemory(time);
        if (lastDate && lastTickMarkType !== undefined && date.valueOf() > lastDate.valueOf() && tickMarkType > lastTickMarkType) {
            if (tickMarkType === LightweightCharts.TickMarkType.Month) {
                // check year is the same
                if (date.getFullYear() !== lastDate.getFullYear()) {
                    answer = '';
                }
            } else if (tickMarkType === LightweightCharts.TickMarkType.DayOfMonth) {
                // check month is the same
                if (date.getMonth() !== lastDate.getMonth() || date.getFullYear() !== lastDate.getFullYear()) {
                    answer = '';
                }
            }
        }
        if (answer !== '') {
            lastDate = date;
            lastTickMarkType = tickMarkType;
        }
        return answer;
    },
});

In this example we are just hiding the tickmark but you can change it to show the data you need

benjamindamm commented 2 months ago

Thank you, I had already discovered that myself. Nevertheless, I am very grateful for your detailed explanation. It still seems strange to me, and to be honest, it feels like a bug.

The data series is very simple—365 data points, all at midnight. It should be straightforward to determine how the labels should be displayed.

I think it makes sense to consider the entire data series to determine the appropriate weighting. Relying solely on the previous and current points will likely always fall short.

I didn’t really want to interfere with the mechanism because I know that if I do, I’ll have to implement all the details that are already working well. It’s possible to write completely custom logic in this way, but there are so many small details that are not easy to handle.

I’d be happy to implement a solution and post it here in case anyone else encounters similar issues.

benjamindamm commented 2 months ago

Will there ever be a solution for this issue, or do you not consider it a problem on your side?

It’s quite a lot of logic that needs to be re-implemented. I don’t quite see why it makes sense for the second label to stand out from the rest.

Of course, I would be happy if there were an out-of-the-box solution, and I'd be glad to contribute in any way I can.

benjamindamm commented 1 month ago

The error still persists and also occurs in production.

I find it hard to imagine that I'm the only one experiencing this. I've provided a StackBlitz where one can clearly reproduce the problem. The proposed solution is very complex and has many implications, some of which are too far-reaching.

illetid commented 1 month ago

Hi, sorry for the late response. We had already identified that behavior internally before https://github.com/tradingview/lightweight-charts/issues/1588 and tried to fix it https://github.com/tradingview/lightweight-charts/pull/1591 but ended up adding shouldResetTickmarkLabels hook so users can change the behavior by themselves. Currently, our focus is on the v5 release, but we can revisit this issue afterward. Thanks for your investigation and patience!