Dataset update for wxLineChart

[Anton-V-K]

It should be possible to update data in wxLineChart.
Specific case of the general feature #30.

[xleclercq]

I've started working on this again. The difficulty is that first the data classes need to be redesigned to be more generic and not chart specific. In turn for this to be possible the various styles and options need to be moved out of the data classes. So currently working on a theme system to manage the options.

[Anton-V-K]

Thanks for the reply.
I've implemented a straightforward simple solution for wxLineChart (and related classes) to allow adding more data to an existing chart. It works quite good when adding data in same thread (f.i., using timer, tested in WxLine sample).
However adding data from another thread turned out to be a tricky thing - wxLineChart re-creates some of its nested objects when being updated, which causes a conflict during drawing.
The simplest solution (a kind of workaround) would be to add a mutex into wxLineChart, so it protects the internal data when DoDraw is executed. However I'm not sure whether such approach complies with wxWidgets/wxCharts coding guidelines :)

[xleclercq]

Multithreading... yikes. OK so as a general rule from my experience with wxWidgets and other UI frameworks is that there should only be one GUI thread. If you start to update the UI from multiple threads it gets really messy and in general it's completely unnecessary as a single thread should be more than enough to update the UI. So I don't think there should be a need to make wxLineChart itself thread-safe. For instance it's not clear that calling DoDraw from a thread that is not the main GUI thread would actually be safe anyway. GUI elements manipulation functions should be called from the same thread all the time.
Having said that there might be a need to make the dataset threadsafe because updating the dataset in a worker thread may be a realistic scenario. We would then somehow signal the chart asynchronously for the need to redraw.
Does that make sense from a purely theoretical point of view?

Now the charts have not been designed with updates in mind (a necessary limitation to start with a reasonable scope for the project). So they may wastefully re-create some nested objects, I don't mind optmizing for that use-case. I just don't think there should be a need for thread-safety.

[xleclercq]

Now it's clear it's going to take a while to implement it the way I have in mind so in the meantime, you know, do whatever works for your project but I'm unlikely to take a PR that puts mutexes in wxLineChart.

[Kvaz1r]

The simplest solution (a kind of workaround) would be to add a mutex into wxLineChart, so it protects the internal data when DoDraw is executed. However I'm not sure whether such approach complies with wxWidgets/wxCharts coding guidelines :)

Simplest solution - use mutex for protect only method Update in user code.

[Anton-V-K]

I'm unlikely to take a PR that puts mutexes in wxLineChart.

OK, I agree with that.
How about changing access from private to protected for the wxLineChart-methods like DoDraw (and probably DoSetSize and DoFit)?
It is needed to call them from the derived class, which will be equipped with a mutex.
A code sample to illustrate the approach:

class wxLineChartSafe : public wxLineChart
{
protected:
    virtual void DoDraw(wxGraphicsContext &gc, bool suppressTooltips)
    {
        std::lock_guard<std::mutex> lock(m_mutex);
        wxLineChart::DoDraw(gc, suppressTooltips);
    }
private:
    std::mutex m_mutex;
};

[Anton-V-K]

I've managed to reproduce the issue with multi-threading conflict in the modified WxLine sample. It tends to happen when data update frequency is relatively high (every 50 ms on my hardware), but may as well happen at less frequencies occasionally.
Here is the stack trace of the data thread:

wxVector<wxChartLabel>::push_back Line 458	C++
wxVector<wxChartLabel>::AssignDispatch<wxChartLabel const * __ptr64> Line 631	C++
wxVector<wxChartLabel>::assign<wxChartLabel const * __ptr64> Line 342	C++
wxChartAxis::SetLabels Line 121	C++
wxChartNumericalAxis::UpdateLimits Line 111	C++
wxChartGrid::UpdateAxisLimit Line 247	C++
wxLineChart::AddData Line 247	C++
wxLineChartCtrl::AddData Line 83	C++
WxLineFrame::DataThread::Entry Line 104	C++
wxThread::CallEntry Line 356	C++
...

At the same moment the main thread is busy with updating labels for the same axis:

...
wxStrlen Line 655	C++
wxCharTypeBuffer<wchar_t>::wxCharTypeBuffer<wchar_t> Line 260	C++
wxWCharBuffer::wxWCharBuffer Line 385	C++
wxGDIPlusContext::GetTextExtent Line 2113	C++
wxChartUtilities::GetTextSize Line 107	C++
wxChartLabel::UpdateSize Line 114	C++
wxChartLabelGroup::UpdateSizes Line 46	C++
wxChartAxis::UpdateLabelSizes Line 80	C++
wxChartGrid::Fit Line 133	C++
wxLineChart::DoDraw Line 378	C++
wxChart::Draw Line 42	C++
wxChartCtrl::OnPaint Line 46	C++
wxAppConsoleBase::HandleEvent Line 658	C++
wxAppConsoleBase::CallEventHandler Line 670	C++
wxEvtHandler::ProcessEventIfMatchesId Line 1398	C++
wxEventHashTable::HandleEvent Line 1004	C++
wxEvtHandler::TryHereOnly Line 1593	C++
wxEvtHandler::TryBeforeAndHere Line 3892	C++
wxEvtHandler::ProcessEventLocally Line 1526	C++
wxEvtHandler::ProcessEvent Line 1499	C++
wxEvtHandler::SafelyProcessEvent Line 1617	C++
wxWindowBase::HandleWindowEvent Line 1540	C++
wxWindow::HandlePaint Line 5021	C++
wxWindow::MSWHandleMessage Line 3052	C++
wxWindow::MSWWindowProc Line 3823	C++
wxWndProc Line 2909	C++
...

I see 3 ways how to deal with such situation:

1. Allow derived classes to implement own synchronization mechanism (as proposed earlier).
2. Change the approach for adding/updating data to queue the new/updated data, so the UI-related elements aren't rebuilt until the repaint happens in the context of the main thread (I guess, such approach will require quite a lot of re-work in wxLineChart and related classes).
3. Queuing new data at user level, so it is added/updated in the context of main thread.
4. Any other options?

With my application I would go for option 3, so the data thread won't be waiting for potentially slow UI updates. But the option 1 seems to be easier in usage for simple cases.

[xleclercq]

Here is the statement about the UI updates needed to happen on the GUI thread. See https://docs.wxwidgets.org/3.1/overview_thread.html

When writing a multi-threaded application, it is strongly recommended that no secondary threads call GUI functions. The design which uses one GUI thread and several worker threads which communicate with the main one using events is much more robust and will undoubtedly save you countless problems (example: under Win32 a thread can only access GDI objects such as pens, brushes, device contexts created by itself and not by the other threads).

[xleclercq]

Anyway yes this does require a big redesign. I'm not fond of option 1 simply because that's simply letting end users having to fix a non-trivial problem by themselves.
Note that it's fine to modify the data in itself on any thread, it's just the redraw that needs to happen on a specific thread. Option 2 is actually my preferred option and what I had in mind but the devil is in the implementation details.
Option 3 essentially works as it is because the application will make sure that from the chart's point of view there is no multithreading taking place. That's a valid option but I think it will lead to the application having to maintain 2 datasets, one in the worker thread and one in the chart.

Anyway option 2 is actually what I want to implement. In terms of redesign well I always knew the current design is version 0.1 and was meant to be improved once I have a better idea of what works and doesn't.

[xleclercq]

I'm unlikely to take a PR that puts mutexes in wxLineChart.

OK, I agree with that.
How about changing access from private to protected for the wxLineChart-methods like DoDraw (and probably DoSetSize and DoFit)?
It is needed to call them from the derived class, which will be equipped with a mutex.
A code sample to illustrate the approach:

class wxLineChartSafe : public wxLineChart
{
protected:
    virtual void DoDraw(wxGraphicsContext &gc, bool suppressTooltips)
    {
        std::lock_guard<std::mutex> lock(m_mutex);
        wxLineChart::DoDraw(gc, suppressTooltips);
    }
private:
    std::mutex m_mutex;
};

Well as mentioned before that would actually still not be safe because the drawing may happen on a different thread. I don't really mind making all methods protected but it's simply not a safe solution.