Skip to content

Adapt chart line output to accept multiple filters#78

Merged
cyberbit merged 4 commits into
cyberbit:mainfrom
cozyGalvinism:multi_series_line_plot
Dec 31, 2024
Merged

Adapt chart line output to accept multiple filters#78
cyberbit merged 4 commits into
cyberbit:mainfrom
cozyGalvinism:multi_series_line_plot

Conversation

@cozyGalvinism

Copy link
Copy Markdown
Contributor

As outlined in #77, this is my proposed API change for ChartLineOutputAdapter.lua. I also amended the filter structure to

{
    [1] = {
        name = 'my_metric',
        color = colors.red
    }
}

The only caveat so far is an error that pops up right at the start of series, apparently by a nil value in the plot data? I cannot figure out where it's coming from... It doesn't affect the output but if I can solve it, I will amend the PR.

@cozyGalvinism

Copy link
Copy Markdown
Contributor Author

I have fixed the error. It came from the fact that I thought the plotter could draw lines based on their local minimums and maximums, but that doesn't seem to be the case. I have amended the chartLines to use the global minimum and maximum instead and the error resolved itself.

@cyberbit cyberbit self-requested a review December 31, 2024 02:42
@cyberbit

Copy link
Copy Markdown
Owner

Looks to be basically working for me! There are some small changes I will recommend once I figure out how to submit them.

image

@cyberbit cyberbit left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Just a few minor changes but otherwise lgtm!

Comment thread src/telem/lib/output.lua Outdated
Comment thread src/telem/lib/output/plotter/ChartMultiLineOutputAdapter.lua
@cyberbit cyberbit merged commit df1eab9 into cyberbit:main Dec 31, 2024
@cyberbit

Copy link
Copy Markdown
Owner

Looks great, merged! Thanks again for contributing 🩵

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.

2 participants