two cross lines issue when I have a timer to call UpdateAll periodically

[asmwarrior]

We have discuss this issue in another issue(debug assert from wxWidgets when I drag a mpBitmapLayer), but let me create a new one here, because that one is for the bitmap issue.

When debugging, I found that when UpdateAll() function get called, the cross line is cleared.

Because there is a function call:

void mpWindow::UpdateAll()
{
	if (UpdateBBox())
	{
        ......
	}

	if (m_magnetize)
	  m_magnet.ClearPlot(*this);

	Refresh(false);
}

In the void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event)), we don't re-create this cross line.

I relieve that if cross line is enabled, when we call UpdateAll(), it got cleared. But we have to mark is as "temporarily cleared", because in the OnPaint event, we will draw the whole window, but after that, do we need to "re-create" it?

[asmwarrior]

I add some log messages in the OnPaint or other functions, and got some log.txt file to see what is the problem.

I think the problem happens in such case:

When I call the UpdateAll, the cross line get cleared, and later in the OnPaint event handler, the window get repaint.

Now, here comes the bug, after that time, if I move the mouse, I see in some situation, I see the mpMagnet::ClearPlot get called (mainly in the mouse move event handler), and in this case, the m_Inside is true, so that:

void mpMagnet::ClearPlot(mpWindow &w)
{
	if (m_Inside)
	{
		wxClientDC dc(&w);
		dc.SetPen(*wxBLACK_PEN);
		dc.SetLogicalFunction(wxINVERT);
		dc.DrawLine(m_mousePosition_old.x, m_plot_size.y, m_mousePosition_old.x, m_plot_size.height);
		dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
		m_Inside = false;
		dc.SetLogicalFunction(wxCOPY);
	}
}

In this case, the cross line get drawed by the XOR method(wxINVERT).

So, I think the solution is that whether the cross line get draw is a status, and maybe, we need a bool variable in the mpMagnet class.

I mean if it get cleared, we don't need to clear it again.

[asmwarrior]

I debugged a while by using the logs. I found that in some case, I got such sequence:

21:53:05: mpWindow::UpdateAll
21:53:05: mpMagnet::ClearPlot, m_Inside = true
21:53:05: mpMagnet::ClearPlot, m_Inside = false!!!
21:53:05: mpMagnet::Plot enter >>>>>>> 
21:53:05: mpMagnet::Plot draw COPY method!!!!!!
21:53:05: mpMagnet::Plot leave <<<<<<<<<< 
21:53:05: mpWindow::OnPaint paint on the memory DC
21:53:05: mpWindow::OnPaint Paint all****
21:53:05: mpWindow::OnPaint memory DC copy to screen

When UpdateAll get called, it just clear the cross line, but the mouse event still happens before the OnPaint event, so that when OnPaint event happens, the status of the cross line could be either shown or not. But when OnPaint happens, we have to force the status of the magnet to its initial status.

This is the patch, pay attention to the function call m_magnet.SetInitState(), I just set its value to the initial status.

diff --git a/mathplot/mathplot.cpp b/mathplot/mathplot.cpp
index 5478907..847bc1b 100644
--- a/mathplot/mathplot.cpp
+++ b/mathplot/mathplot.cpp
@@ -2844,6 +2844,8 @@ void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event))
 		delete m_buff_dc;
 	}
 	m_repainting = false;
+
+	m_magnet.SetInitState(); // the window get repaint, so set the magnet to initial status
 }
 
 void mpWindow::SetMPScrollbars(bool status)
@@ -4560,6 +4562,7 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
 		dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
 		m_Inside = false;
 		m_rightClick = false;
+		m_IsShown = false;
 	}
 
 	if (m_domain.Contains(mousePos))
@@ -4577,6 +4580,7 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
 				dc.SetLogicalFunction(wxCOPY);
 				m_mousePosition_old = mousePos;
 				m_Inside = true;
+				m_IsShown = true;
 			}
 		}
 	}
@@ -4586,7 +4590,7 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
 
 void mpMagnet::ClearPlot(mpWindow &w)
 {
-	if (m_Inside)
+	if (m_Inside && m_IsShown)
 	{
 		wxClientDC dc(&w);
 		dc.SetPen(*wxBLACK_PEN);
@@ -4594,6 +4598,7 @@ void mpMagnet::ClearPlot(mpWindow &w)
 		dc.DrawLine(m_mousePosition_old.x, m_plot_size.y, m_mousePosition_old.x, m_plot_size.height);
 		dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
 		m_Inside = false;
+		m_IsShown = false;
 		dc.SetLogicalFunction(wxCOPY);
 	}
 }
diff --git a/mathplot/mathplot.h b/mathplot/mathplot.h
index dfa4761..3b263e7 100644
--- a/mathplot/mathplot.h
+++ b/mathplot/mathplot.h
@@ -1538,11 +1538,19 @@ class mpMagnet
 		{
 			m_Inside = false;
 			m_rightClick = false;
+			m_IsShown = false;
 		}
 		~mpMagnet()
 		{
-			;
 		}
+
+		void SetInitState()
+		{
+			m_Inside = false;
+			m_rightClick = false;
+			m_IsShown = false;
+		}
+
 		void UpdateBox(wxCoord left, wxCoord top, wxCoord width, wxCoord height)
 		{
 			m_domain = wxRect(left, top, width, height);
@@ -1562,6 +1570,7 @@ class mpMagnet
 		wxPoint m_mousePosition_old;
 		bool m_Inside;
 		bool m_rightClick;
+		bool m_IsShown;
 };
 
 /** Canvas for plotting mpLayer implementations.

The above patch fixes the issue.

But I think there is still one thing remains. After the repaint event, if mouse is not moved, no cross line is shown. So, we need another way to emulate a mouse move event? Or similar things to draw the cross line again.

[GitHubLionel]

Hi,
You go to fast, for the moment, I just push the last corrections.
For this problem, I need to test your patch.
In fact, when you paint, refresh or updateAll, you don't need to draw the cross. We just need to say to mpMagnet to not redraw the cross if the mouse move because the background has changed. I suppose that your boolean m_IsShown play that role, I will test.

[asmwarrior]

Yes, I think after we call UpdateAll(), we need to "stop/disable" the drawing of mpMagnet.

But in some cases, some events still happens before the OnPaint event, so that the there is still a chance the cross lines were drawn again.

For those reason, I think we can either "make a clean reset of mpMagnet" after the "OnPaint", which is what my patch does.

Further more, which I don't implement is that I think a better way is we can "disable the drawing feature of mpMagnet" in the UpdateAll() function(Not just clear the cross lines), and "re-enable the drawing feature of mpMagnet" after "OnPaint". This way, we just drop all the events between "UpdateAll()" and "OnPaint" for drawing or clearing the cross lines.

[GitHubLionel]

Hi

Ok, I reorganize the process. I also redraw the cross after the paint. Now the cross is still visible if we zoom, pan and select rectangle. I test with a repaint in a timer (the original sample3 with a moving car) and it work.
Same time, I change some words and some functions.

[asmwarrior]

I just test the latest master ahead code, I see that, if I don't have a timer to refresh the window, I still see two crosses when I zoom the window, so this is the first bug. And the second bug is that if I start the timer, which call UpdateAll() every 3 seconds, I still see sometimes, there are two crosses in the window, this only goes back to one cross when the OnPaint() happens.

[GitHubLionel]

I can not reproduce this bug, neither with MathPlotConfig (the sinus), neither with MathPlotDemo or with sample3 (a moving car with a timer 25 ms).
UpdateAll() call Refresh() who call OnPaint(), so all the scene is replotted, so no need to erase the cross before. We just need to keep in memory if the cross was drawn before to repaint it after (that is the goal of the ReInitDrawn() method).
May be you can try Refresh(true); in line 3011.
Note that we can use the flag m_repainting at beginning of UpdateAll() to prevent the cross drawing.

[asmwarrior]

I think I can reproduce this bug, I'm using the MathPlotConfig (the sinus). I don't have any wxTimer to refresh the window. I just keep the mouse moving, also sometimes, I scroll(zoom) the window. You can have a look at my screen cast video below.

two-crosses-2023-05-16.mp4

EDIT: it looks like my Firefox can't play the attached mp4 file on the web, but I can confirm that you can just download the mp4 file to your local disk, and use some player(I'm using vlc player) to play it.

[asmwarrior]

		void UpdateBox(wxCoord left, wxCoord top, wxCoord width, wxCoord height)
		{
			m_domain = wxRect(left, top, width, height);
			m_plot_size = wxRect(left, top, width + left, height + top);
		}

When reading the code, I see the above code.
Those two member variable shared the same left and top. And m_plot_size is actually not a wxSize, but a wxRect. Do you think it is redundant?

I also draw a ascii art to demonstrate:

//  +---------------------------------+
//  |  Plot_size_XY(width,height)     |
//  |  +--------------------------+   |
//  |  |     |                    |   |
//  |  |     |                    |   |
//  |  |-----|--------------------|   |
//  |  |     |MousePos            |   |
//  |  |     |                    |   |
//  |  |     |                    |   |
//  |  +--------------------------+   |
//  +---------------------------------+
//             mPWindow

[GitHubLionel]

I think I can reproduce this bug, I'm using the MathPlotConfig (the sinus). I don't have any wxTimer to refresh the window. I just keep the mouse moving, also sometimes, I scroll(zoom) the window. You can have a look at my screen cast video below.

No, for me, no problem even moving faster the mouse when zooming or moving pan faster in all directions.
I only can have this problem one or two times when I disabled double buffering (and if I move mouse very faster) and so have flickering.
What is strange on your video is that the cross is not always drawing when the mouse move. For me, the cross is always visible.

[GitHubLionel]

Those two member variable shared the same left and top. And m_plot_size is actually not a wxSize, but a wxRect. Do you think it is redundant?

Not redundant but the word m_plot_size is maybe not the best.

[asmwarrior]

When using the log, I can see the bug happens here:

18:01:41: mpMagnet::Plot enter >>>>>>> 
18:01:41: mpMagnet::Plot draw COPY method!!!!!!
18:01:41: mpMagnet::Plot leave <<<<<<<<<< 
18:01:41: mpWindow::UpdateAll
18:01:41: mpWindow::OnPaint paint on the memory DC
18:01:41: mpWindow::OnPaint Paint all****
18:01:41: mpWindow::OnPaint memory DC copy to screen
18:01:41: mpMagnet::UpdatePlot m_IsWasDrawn=true****
18:01:41: mpMagnet::ClearPlot, m_InDrawn = true
18:01:41: mpMagnet::ClearPlot, m_InDrawn = true
18:01:41: mpMagnet::Plot enter >>>>>>> 
18:01:41: mpMagnet::Plot draw COPY method!!!!!!
18:01:41: mpMagnet::Plot leave <<<<<<<<<< 

You see, when mpMagnet::UpdatePlot get called, the m_IsWasDrawn is true.

You got mpMagnet::ClearPlot, m_InDrawn = true called 2 times later.

18:01:43: mpMagnet::ClearPlot, m_InDrawn = true
18:01:43: mpMagnet::Plot enter >>>>>>> 
18:01:43: mpMagnet::Plot draw XOR method, m_Inside(true)
18:01:43: mpMagnet::Plot draw COPY method!!!!!!
18:01:43: mpMagnet::Plot leave <<<<<<<<<< 
18:01:43: mpWindow::OnPaint paint on the memory DC
18:01:43: mpWindow::OnPaint Paint all****
18:01:43: mpWindow::OnPaint memory DC copy to screen
18:01:43: mpMagnet::UpdatePlot m_IsWasDrawn=false!!!!
18:01:43: mpMagnet::ClearPlot, m_InDrawn = true
18:01:43: mpMagnet::Plot enter >>>>>>> 
18:01:43: mpMagnet::Plot draw COPY method!!!!!!
18:01:43: mpMagnet::Plot leave <<<<<<<<<< 

In another case, the mpMagnet::ClearPlot called once, which draw the extra cross.

I will post the patch(I used the debug log) below:

From 2d7c6a65f0150e3874d17a3a01732063f5fa0087 Mon Sep 17 00:00:00 2001
From: hidehide <hidehide@hidehide.com>
Date: Sun, 14 May 2023 22:09:11 +0800
Subject: [PATCH] add the debug code

---
 MathPlotConfig/MathPlotConfigDemo.cpp | 14 +++++++++++++-
 mathplot/mathplot.cpp                 | 20 ++++++++++++++++++--
 2 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/MathPlotConfig/MathPlotConfigDemo.cpp b/MathPlotConfig/MathPlotConfigDemo.cpp
index 3c21a1f..66166da 100644
--- a/MathPlotConfig/MathPlotConfigDemo.cpp
+++ b/MathPlotConfig/MathPlotConfigDemo.cpp
@@ -38,6 +38,8 @@
 
 #include "mathplot.h"
 
+#include <wx/ffile.h>
+
 // Define a new application type, each program should derive a class from wxApp
 class MyApp: public wxApp
 {
@@ -94,7 +96,8 @@ enum
 // the event tables connect the wxWidgets events with the functions (event
 // handlers) which process them. It can be also done at run-time, but for the
 // simple menu events like this the static method is much simpler.
-wxBEGIN_EVENT_TABLE(MyFrame, wxFrame) EVT_MENU(Demo_Quit, MyFrame::OnQuit)
+wxBEGIN_EVENT_TABLE(MyFrame, wxFrame)
+EVT_MENU(Demo_Quit, MyFrame::OnQuit)
 EVT_MENU(Demo_About, MyFrame::OnAbout)
 wxEND_EVENT_TABLE()
 
@@ -177,6 +180,15 @@ MyFrame::MyFrame(const wxString &title) :
 	wxSizer *sizer = new wxBoxSizer(wxVERTICAL);
 	sizer->Add(m_plot, 1, wxALL | wxEXPAND, 5);
 	SetSizer(sizer);
+
+	wxLog* logger = new wxLogStream(&std::cout);
+    wxLog::SetActiveTarget(logger);
+
+    FILE *logFile = fopen("log.txt", "w");
+    wxLogChain* logToFile = new wxLogChain(new wxLogStderr(logFile));
+    wxLog::SetActiveTarget(logToFile);
+
+
 }
 
 void MyFrame::CreatePlot(void)
diff --git a/mathplot/mathplot.cpp b/mathplot/mathplot.cpp
index 207d114..dfef747 100644
--- a/mathplot/mathplot.cpp
+++ b/mathplot/mathplot.cpp
@@ -2754,6 +2754,7 @@ void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event))
 	// J.L.Blanco @ Aug 2007: Added double buffer support
 	if (m_enableDoubleBuffer)
 	{
+		wxLogMessage("mpWindow::OnPaint paint on the memory DC");
 		// Recreate Bitmap if sizes have changed
 		if (m_last_lx != m_scrX || m_last_ly != m_scrY)
 		{
@@ -2790,6 +2791,7 @@ void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event))
 
 	// Draw all the layers:
 	m_repainting = true;
+	wxLogMessage("mpWindow::OnPaint Paint all****");
 
 	mpFunctionType function;
 	mpScaleType scale;
@@ -2809,6 +2811,7 @@ void mpWindow::OnPaint(wxPaintEvent &WXUNUSED(event))
 	// If doublebuffer, draw now to the window:
 	if (m_enableDoubleBuffer)
 	{
+		wxLogMessage("mpWindow::OnPaint memory DC copy to screen");
 		dc.Blit(0, 0, m_scrX, m_scrY, trgDc, 0, 0, wxCOPY);
 		m_buff_dc->SelectObject(wxNullBitmap);
 		delete m_buff_dc;
@@ -3004,7 +3007,7 @@ void mpWindow::UpdateAll()
 			}
 		}
 	}
-
+	wxLogMessage("mpWindow::UpdateAll");
 	if (m_magnetize)
 	  m_magnet.ReInitDrawn();
 
@@ -4539,9 +4542,10 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
 	wxClientDC dc(&w);
 	dc.SetPen(*wxBLACK_PEN);
 	dc.SetLogicalFunction(wxINVERT);
-
+	wxLogMessage("mpMagnet::Plot enter >>>>>>> ");
 	if (m_IsDrawn)
 	{
+		wxLogMessage("mpMagnet::Plot draw XOR method, m_Inside(true)");
 		dc.DrawLine(m_mousePosition_old.x, m_plot_size.y, m_mousePosition_old.x, m_plot_size.height);
 		dc.DrawLine(m_plot_size.x, m_mousePosition_old.y, m_plot_size.width, m_mousePosition_old.y);
 		m_IsDrawn = false;
@@ -4558,6 +4562,7 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
 			}
 			else
 			{
+				wxLogMessage("mpMagnet::Plot draw COPY method!!!!!!");
 				dc.DrawLine(mousePos.x, m_plot_size.y, mousePos.x, m_plot_size.height);
 				dc.DrawLine(m_plot_size.x, mousePos.y, m_plot_size.width, mousePos.y);
 				dc.SetLogicalFunction(wxCOPY);
@@ -4567,6 +4572,8 @@ void mpMagnet::Plot(mpWindow &w, const wxPoint &mousePos)
 		}
 	}
 
+	wxLogMessage("mpMagnet::Plot leave <<<<<<<<<< ");
+
 	dc.SetLogicalFunction(wxCOPY);
 }
 
@@ -4574,6 +4581,7 @@ void mpMagnet::ClearPlot(mpWindow &w)
 {
 	if (m_IsDrawn || m_IsWasDrawn)
 	{
+		wxLogMessage("mpMagnet::ClearPlot, m_InDrawn = true");
 		wxClientDC dc(&w);
 		dc.SetPen(*wxBLACK_PEN);
 		dc.SetLogicalFunction(wxINVERT);
@@ -4583,6 +4591,10 @@ void mpMagnet::ClearPlot(mpWindow &w)
 		m_IsWasDrawn = false;
 		dc.SetLogicalFunction(wxCOPY);
 	}
+	else
+	{
+		wxLogMessage("mpMagnet::ClearPlot, m_IsDrawn = false!!!");
+	}
 }
 
 void mpMagnet::UpdatePlot(mpWindow &w, const wxPoint &mousePos)
@@ -4592,8 +4604,12 @@ void mpMagnet::UpdatePlot(mpWindow &w, const wxPoint &mousePos)
 		// Mouse position change when pan operation
 		if (m_rightClick)
 			m_mousePosition_old = mousePos;
+
+        wxLogMessage("mpMagnet::UpdatePlot m_IsWasDrawn=true****");
 		ClearPlot(w);
 	}
+	else
+        wxLogMessage("mpMagnet::UpdatePlot m_IsWasDrawn=false!!!!");
 }
 
 //-----------------------------------------------------------------------------
-- 
2.37.0.windows.1

[GitHubLionel]

Well, it is a mystery ! You can not have in same time m_IsWasDrawn and m_IsDrawn to true because when m_IsWasDrawn is set to true, in same time m_IsDrawn is set to false in the ReInitDrawn() function.

I push the code with my log function (I simply use a define to have log in console).
Then I have this log :

mpMagnet::Plot enter >>>>>>> m_IsDrawn = false
mpMagnet::Plot draw COPY method!!!!!!
mpMagnet::Plot leave <<<<<<<<<<
mpWindow::UpdateAll
mpWindow::OnPaint enter >>>>> paint on the memory DC
mpWindow::OnPaint Paint all****
mpWindow::OnPaint leave <<<<< memory DC copy to screen
mpMagnet::UpdatePlot m_IsWasDrawn = true
mpMagnet::ClearPlot m_IsDrawn = false
mpMagnet::ClearPlot m_IsDrawn = true
mpMagnet::Plot enter >>>>>>> m_IsDrawn = false
mpMagnet::Plot draw COPY method!!!!!!
mpMagnet::Plot leave <<<<<<<<<<
mpMagnet::ClearPlot m_IsDrawn = true

As you can see, the first call of ClearPlot, m_IsDrawn is false.

[asmwarrior]

Sorry, I don't understand the logic of the drawing. I mean: What does the variable m_IsWasDrawn used for?

[GitHubLionel]

When we call UpdateAll (and then OnPaint), you must remember if the cross was drawn : that's the purpose of m_IsWasDrawn. So at the end of OnPaint, we call UpdatePlot to redraw the cross only if m_IsWasDrawn is true.