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

Redrawing chart after updating Datasets doesn't work consistently #259

Closed
asmirn1 opened this issue Sep 16, 2020 · 15 comments · Fixed by #260
Closed

Redrawing chart after updating Datasets doesn't work consistently #259

asmirn1 opened this issue Sep 16, 2020 · 15 comments · Fixed by #260
Assignees
Labels
bug:minor workaround possible

Comments

@asmirn1
Copy link

asmirn1 commented Sep 16, 2020

Hi! Thank you for your work on ChartFX.

I'm having an issue with redrawing a chart after updating its datasets. I update datasets as follows:

public void update(List<DataPoint> dataPoints) {
    renderer.getDatasets().clear();
    DefaultDataSet dataSet = new DefaultDataSet("DataSet");
    for (DataPoint dataPoint : dataPoints) {
            double retTime = dataPoint.getRetTime();
            double intensity = dataPoint.getIntensity();
            dataSet.add(retTime, intensity);
    }
    renderer.getDatasets().add(dataSet);
}

where renderer is ErrorDataSetRenderer added to an XYChart.

When I call this function, sometimes I get a correct plot and sometimes I get a blank background. Here are a few examples:

Screen Shot 2020-09-16 at 12 45 03 PM

Screen Shot 2020-09-16 at 12 45 14 PM

Screen Shot 2020-09-16 at 12 45 26 PM

Screen Shot 2020-09-16 at 12 45 36 PM

Screen Shot 2020-09-16 at 12 45 45 PM

I was able to solve this issue by calling chart.layout() after I update datasets, as follows:

public void update(List<DataPoint> dataPoints) {
    renderer.getDatasets().clear();
    DefaultDataSet dataSet = new DefaultDataSet("DataSet");
    for (DataPoint dataPoint : dataPoints) {
            double retTime = dataPoint.getRetTime();
            double intensity = dataPoint.getIntensity();
            dataSet.add(retTime, intensity);
    }
    renderer.getDatasets().add(dataSet);

    chart.layout()
}

The latter code works and displays all plots correctly whenever the legend is visible (i.e. chart.setLegendVisible(true)). However, when the legend isn't visible, then the plots are still sometimes displayed and sometimes not. I want my users to be able to hide the legend, so I guess I need another solution for my problem.

Maybe, I just don't call a correct function to update charts. What is the recommended ways to redraw plots after updating datasets?

@milo-gsi
Copy link
Contributor

Hi Aleksandr,

until the real experts show up,
you might want to test if invalidating the registered Dataset does the job.

https://github.com/GSI-CS-CO/chart-fx/blob/9e996d8cb1cf791eb22508520b7297393129cfce/chartfx-samples/src/main/java/de/gsi/chart/samples/TimeAxisSample.java#L43

@RalphSteinhagen
Copy link
Member

RalphSteinhagen commented Sep 17, 2020

First, thanks for your interest in chart-fx @asmirn1!

In addition to what @milo-gsi already mentioned, you may also want to replace the .clear(...) and then .add(...) with a setAll(...) which does both and also avoids the double-notification, or modify the already attached data set in-place.

The latter is -- especially for large data sets or fast updates -- more memory efficient and faster, also because many listeners are attached to the data sets directly. The renderer.getDatasets() are ObservableLists where adding/removing of data sets is also propagated but (depending on the thread that invokes them) not always deterministically executed (JavaFX- and data update-thread concurrencies). You may also find the notes on thread-safety useful.

N.B. If modifying in-place, its always good to first grab the DataSet's write lock while modifying, disable/mute the auto-notification while modifying multiple points since this avoids redundant UI updates, modify the data through adding/setting in the DataSet (either individually or in bulk), re-enable the auto-notification, and since the auto-notification may have been muted issue an explicit notify as @milo-gsi mentioned.

The setter in the 'DoubleDataSet` -- which you could also use equally in your case -- illustrates this a bit more in detail:
https://github.com/GSI-CS-CO/chart-fx/blob/9e996d8cb1cf791eb22508520b7297393129cfce/chartfx-dataset/src/main/java/de/gsi/dataset/spi/DoubleDataSet.java#L382-L415

The critical part is:

dataSet.lock().writeLockGuard(() -> {
  // [..] modify the data set w/o triggering events
});
fireInvalidated(new UpdatedDataEvent(dataSet)); // trigger update-event explicitly

We should add (better write) this to the documentation... ;-)

Let us know if this helps with your issue.

@RalphSteinhagen
Copy link
Member

until the real experts show up,

@milo-gsi you are a bit too modest... ;-)

You write very nice data acquisition applications used in production with ten thousands of data points that are updating at 25 Hz... maybe we should show-case a bit more what you and others are doing with this little lib.

@asmirn1
Copy link
Author

asmirn1 commented Sep 17, 2020

@milo-gsi @RalphSteinhagen

Thank you for your quick replies!

I've tried your suggestions and what worked for me is the following

public void update(List<DataPoint> dataPoints) {
    renderer.getDatasets().clear();
    DefaultDataSet dataSet = new DefaultDataSet("DataSet");
    for (DataPoint dataPoint : dataPoints) {
            double retTime = dataPoint.getRetTime();
            double intensity = dataPoint.getIntensity();
            dataSet.add(retTime, intensity);
    }
    Platform.runLater(() -> dataSet.fireInvalidated(null));
    renderer.getDatasets().add(dataSet);

    chart.layout()
}

Calling dataSet.fireInvalidated(null) without chart.layout() still results in some blank plots (although much less frequent than when I call chart.layout() without dataSet.fireInvalidated(null)). Calling both dataSet.fireInvalidated(null) and chart.layout() seems to solve the issue.

@RalphSteinhagen Thank you for your suggestions about modifying in-place and using .lock() method. With my data, ChartFX shows a great performance even when I use "lazy" clearing and creating new datasets. But I'll keep in mind the modifying in-place option.

I really like ChartFX. It's definitely the best option for plotting charts in JavaFX, that I tried so far. However, when I have any issues with ChartFX, I find myself going through a lengthy process of trial and error. Is there any other documentation for ChartFX other than the code examples on Github?

@RalphSteinhagen
Copy link
Member

Is there any other documentation for ChartFX other than the code examples on Github?

@asmirn1 unfortunately not (yet). This is a long-standing effort/request (see e.g. #255) which we'd like to follow-up but couldn't find the time yet because of other (internally/unrelated) reasons.

Any help on that front (ie. good beginners -> advanced users tutorial) would be highly appreciated.

Please feel cordially invited to contribute to that effort! 😁

@RalphSteinhagen
Copy link
Member

@asmirn1 regardless you may want to modify your code to (wrong line ordering, duplicate clear/add, missing event mapped to 'null').

public void update(List<DataPoint> dataPoints) {
    DoubleDataSet dataSet = new DoubleDataSet("DataSet");
    for (DataPoint dataPoint : dataPoints) {
            double retTime = dataPoint.getRetTime();
            double intensity = dataPoint.getIntensity();
            dataSet.add(retTime, intensity);
    }
    renderer.getDatasets().setAll(dataSet);
    // dataSet.fireInvalidated(new UpdatedDataEvent(dataSet)); // should be OPTIONAL/not needed

    // chart.layout(); // should not be needed (not recommended)
}

If you could let me know if this workd for you!? 😄

@asmirn1
Copy link
Author

asmirn1 commented Sep 17, 2020

@RalphSteinhagen
The code that I wrote above is not the actual code that I'm using. In the actual code, I create several datasets. That's why I was using clear() and add() methods.

The actual code looks more like this:

public void updateChart() {
    ...
    List<DataSet> dataSets = new ArrayList<>();
    for (Feature feature : features)
        dataSets.add(addToRetTimeIntensityChart(feature));
    renderer.getDatasets().setAll(dataSets);
    ...
}

private DataSet addToRetTimeIntensityChart(Feature feature) {

        Chromatogram chromatogram = feature.getChromatogram();
        if (chromatogram == null) return null;

        DefaultDataSet dataSet = new DefaultDataSet(feature.toString());
        for (DataPoint dataPoint : chromatogram.getDataPoints()) {

            double retTime = dataPoint.getRetTime();
            double intensity = dataPoint.getIntensity();
            dataSet.add(retTime, intensity);

            totalIntensitiesBuffer.compute(retTime, (k, v) -> (v != null) ? v + intensity : intensity);
        }
        return dataSet;
}

As you see, I've tried to modify the code to use renderer.getDatasets().setAll(dataSets), but it didn't resolve the issue. To resolve the issue, I still need to call dataset.fireInvalidated() and chart.layout().

@RalphSteinhagen
Copy link
Member

RalphSteinhagen commented Sep 17, 2020

@asmirn1 would you perhaps have a MVP example that for this? Can you reproduce this with one of the samples in the master branch and/or 11.2.0 tagged version? Your Java version? OS? Any additional info would be helpful to track this down.

@asmirn1
Copy link
Author

asmirn1 commented Sep 18, 2020

@RalphSteinhagen
I've created a small example to reproduce the issue: https://github.com/asmirn1/chartfx-plot-issue
My Machine: MacBook, MacOS Catalina, Version 10.15.6.
Java 13, javafx-sdk-14.0.2.1, ChartFX 11.1.5.

Here is a short video of the issue:
Screen Recording chartfx-plot-issue.mov.zip

I haven't tried different version of ChartFX. I'll let you know the results when I try though.

@RalphSteinhagen
Copy link
Member

@asmirn1 thanks.:+1: I'm having a look at it.

@RalphSteinhagen RalphSteinhagen self-assigned this Sep 19, 2020
RalphSteinhagen added a commit that referenced this issue Sep 19, 2020
N.B. optional animation of currentLowerBound caused caching issues -> removed animation functionality, recompute scale whenever the min, max, or axisLength changes, and moved caching of offset (default getDisplayPosition(..) impl) to AbstractAxisParameter.AbstractAxisParameter

Since most user care for performance rather than animation, the animation feature should be moved to another generic Axis implementation that wraps the original axis and that translates the actual min/max to a time-filtered min/max that is forwarded to the delegate Axis.
@RalphSteinhagen
Copy link
Member

@asmirn1 this was a proper bug related to caching of the scale parameter in AbstractAxis for animated Axis. This is fixed now in the fixIssue259 branch.

Your usage didn't have anything directly to do with the bug. However, your MVP and the way you tested -- walking through the list using keyboard events -- nicely emphasized and helped to localize the bug. N.B. We usually tend to do mouse-based test or automatically via the TestFX robot (which acts similar) and didn't catch this because the mouse typically generates more than one event and the bug occurred only for the first update event. Thanks again for the MVP. In the end, this helped a lot!

Will do a PR for the fixed branch soon.

Meanwhile, if you could confirm that also fixes the issue/symptoms in your application. Thanks in advance and much appreciated.

@RalphSteinhagen RalphSteinhagen linked a pull request Sep 19, 2020 that will close this issue
@RalphSteinhagen RalphSteinhagen added the bug:minor workaround possible label Sep 19, 2020
RalphSteinhagen added a commit that referenced this issue Sep 19, 2020
N.B. optional animation of currentLowerBound caused caching issues -> removed animation functionality, recompute scale whenever the min, max, or axisLength changes, and moved caching of offset (default getDisplayPosition(..) impl) to AbstractAxisParameter.AbstractAxisParameter

Since most user care for performance rather than animation, the animation feature should be moved to another generic Axis implementation that wraps the original axis and that translates the actual min/max to a time-filtered min/max that is forwarded to the delegate Axis.

N.B. suppresses false 'cache==null' -- called from static initializer
@asmirn1
Copy link
Author

asmirn1 commented Sep 21, 2020

@RalphSteinhagen I'm glad that it was helpful. Thank you for your quick responses and fixes.

To test the new code, should I compile the master branch or the version 11.2.0 already contains this bug fix?

@RalphSteinhagen
Copy link
Member

@asmirn1 for the time being you'd need to test against the 'fixIssue259' branch.

@wirew0rm
Copy link
Member

If you do not want to compile yourself, you can also use the snapshot repository as described here. Just enable the snapshot repository and use fixIssue259-SNAPSHOTfor the version.

RalphSteinhagen added a commit that referenced this issue Sep 21, 2020
N.B. optional animation of currentLowerBound caused caching issues -> removed animation functionality, recompute scale whenever the min, max, or axisLength changes, and moved caching of offset (default getDisplayPosition(..) impl) to AbstractAxisParameter.AbstractAxisParameter

Since most user care for performance rather than animation, the animation feature should be moved to another generic Axis implementation that wraps the original axis and that translates the actual min/max to a time-filtered min/max that is forwarded to the delegate Axis.

N.B. suppresses false 'cache==null' -- called from static initializer
@asmirn1
Copy link
Author

asmirn1 commented Sep 21, 2020

Hi all,

I just want to confirm that with the version fixIssue259, everything works as expected.

Thank you!

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

Successfully merging a pull request may close this issue.

4 participants