Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
benjamindamm opened this issue Sep 3, 2024 · 5 comments
Open

Comments

@benjamindamm
Copy link

benjamindamm commented Sep 3, 2024

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:
#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
Copy link
Contributor

illetid commented Sep 3, 2024

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
Copy link
Author

benjamindamm commented Sep 3, 2024

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
Copy link
Author

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
Copy link
Author

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
Copy link
Contributor

illetid commented Sep 30, 2024

Hi, sorry for the late response. We had already identified that behavior internally before #1588 and tried to fix it #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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants