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

Axis updating issue fix for limited points while maintaining the fix the for #1255 #1417

Conversation

gayanSandamal
Copy link
Contributor

@gayanSandamal gayanSandamal commented Oct 22, 2024

Description

This fixes the issue #1411 while maintaining previous fix done for #1255

Screen.Recording.2024-10-22.at.11.14.53.mov

Related Issue(s)

#1411

Docs update is not necessary

Checklist

  • I have read the contribution guidelines
  • Suitable unit/system level tests have been added and they pass
  • Documentation has been updated
    • Upgrade instructions
    • Configuration details
    • Concepts
  • Changes flowforge.yml?
    • Issue/PR raised on FlowFuse/helm to update ConfigMap Template
    • Issue/PR raised on FlowFuse/CloudProject to update values for Staging/Production

Labels

  • Includes a DB migration? -> add the area:migration label

@gayanSandamal gayanSandamal linked an issue Oct 22, 2024 that may be closed by this pull request
@gayanSandamal gayanSandamal self-assigned this Oct 22, 2024
Copy link
Collaborator

@joepavitt joepavitt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whilst I can see that this works, I'm not sure I understand why the code changes that were made, were made. I was expecting something in limitDataSize() to be changed, but it hasn't.

Can you talk me through the changes you've made here, and the logic behind why those changes fix the problem please?

@gayanSandamal
Copy link
Contributor Author

Whilst I can see that this works, I'm not sure I understand why the code changes that were made, were made. I was expecting something in limitDataSize() to be changed, but it hasn't.

Can you talk me through the changes you've made here, and the logic behind why those changes fix the problem please?

Hi @joepavitt ,

Thanks for your feedback! Let me walk you through the changes and the reasoning behind them.

The issue we were addressing had two aspects:

  1. Preserving point data across updates: In the previous implementation, when a new x-value was encountered, the logic always initialized an array of known x-values (xLabels) with undefined objects. This could lead to problems where the first data point wouldn’t draw a connecting line properly (especially for new series or topics). This happened because gaps in the dataset resulted in incomplete arrays, affecting rendering.

  2. Handling the new x-values when removeOlderPoints is enabled: I modified the initialization of the data array. Now, if removeOlderPoints is enabled, we extend the array length by one to accommodate the newest point before discarding old ones. This ensures that we’re correctly aligning the incoming data points to the corresponding x-values without any skips.

let data = Array(xLabels.length).fill({});
if (this.props.removeOlderPoints) {
  data = Array(xLabels.length + 1).fill({});
}
  1. Fixing the handling of new x-values dynamically: If a new x-value arrives and removeOlderPoints is active, we store it at the tail of the array rather than introducing undefined gaps:
if (this.props.removeOlderPoints) {
  data[xLabels.length] = datapoint;
} else {
  xLabels.push(datapoint.x);
  data.push(datapoint);
}

This change smoothen the transitions in the chart without skipping the first data point when switching topics or adding new series dynamically.

@joepavitt
Copy link
Collaborator

joepavitt commented Oct 23, 2024

@gayanSandamal the logic you've detailed here doesn't really address the why the problem was occurring. The key function for trimming data is limitDataSize, the logic for which data points to retain is stored here.

In actual fact, the reason your code changes fix the problem is because you're not adding the datapoint.x to the array of xLabels stored on the chart in the branch of the new if statement you've introduced. In preventing that action, the line-charts work again.

The actual fix here is in limitDataSize, and is to ensure that the length of xLabels on the chart is the same as the data points when rendering scatter/line charts. Alternatively, I actually think we have no need to be storing xLabels at all for scatter/line charts, I'm playing locally, and see no need/benefit for them at all.

This can be quickly checked by removing the xLabels.push() line from the code base, without any of the changes you've included here, which fixes the cut-off points.

The line was rendering too long as we were keep storage of all xLabel values, but not a corresponding x,y pair to associate with the label.

@joepavitt joepavitt closed this Oct 29, 2024
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

Successfully merging this pull request may close these issues.

Chart: Axis not updating correctly with point/time limits
2 participants