Solved

Simple multi-threading synchronization question

Posted on 1998-08-12
17
246 Views
Last Modified: 2013-11-20
Hi,

I am working on my first multi-threaded program. It generates a star field on the screen which is continually moving around; tough to explain.

Anyway, I thought that I would create a separate thread for the drawing of the stars. The main program thread would perform all of the calculations for where the stars are moving to.

So, when drawing, it leaves garbage behind. I know I am doing it right because I have done many programs like this (i.e. erase prev star, draw new star, calculate new pos, loop), and the problem hasn't started occurring until I tried multi-threading.

What I think is happening is the stars coordinates are being updated at the same time they're being drawn, which is kind of what I want (i.e. I want the program to go faster :-)).

I think I can solve the problem using something like CSemaphore, CMutex, or CCriticalLock and the CSingleLock classes, but I don't understand how to use them.

Here is a large part of the source code:

void CSpaceSaverDlg::OnTimer(UINT nIDEvent)
{ int i;
  int j = 0;

  dat->s = stars;
  dat->num = m_Number;
  dat->wnd = this;

  AfxBeginThread (ThreadProc, dat); // Passing global variable?  That's life.
  // Sleep (50); // Fixes some
  // Sleep (100); // Fixes all the way.

  if (rand () % 1000 == 0)
  { m_Direction = rand () % 8; }

  for (i = 0; i < m_Number; i++)
  { CSingleLock sLock (&stars[i].m, FALSE);
    if (sLock.Lock () == 0)
    { MessageBox ("Error");
      break;
    }

    stars[i].px = stars[i].x;
    stars[i].py = stars[i].y;

    switch (m_Direction)
    { case (0): stars[i].x += stars[i].z / m_Speed;
                break;

      case (1): stars[i].x += stars[i].z / m_Speed;
                stars[i].y += stars[i].z / m_Speed;
                break;

      case (2): stars[i].y += stars[i].z / m_Speed;
                break;

      case (3): stars[i].x -= stars[i].z / m_Speed;
                stars[i].y += stars[i].z / m_Speed;
                break;

      case (4): stars[i].x -= stars[i].z / m_Speed;
                break;

      case (5): stars[i].x -= stars[i].z / m_Speed;
                stars[i].y -= stars[i].z / m_Speed;
                break;

      case (6): stars[i].y -= stars[i].z / m_Speed;
                break;

      case (7): stars[i].y -= stars[i].z / m_Speed;
                stars[i].x += stars[i].z / m_Speed;
                break;
    }

    if (stars[i].x > m_MaxX)
    { stars[i].x -= m_MaxX; }
    else if (stars[i].x < 0.0)
    { stars[i].x += m_MaxX; }

    if (stars[i].y > m_MaxY)
    { stars[i].y -= m_MaxY; }
    else if (stars[i].y < 0.0)
    { stars[i].y += m_MaxY; }
    sLock.Unlock ();
  }

  CDialog::OnTimer(nIDEvent);
}

UINT ThreadProc (LPVOID pParam)
{ Data* dat = (Data *) pParam;

  if (dat == NULL)
  { return -1; }   // illegal parameter

  CDC *dc = dat->wnd->GetDC ();
  CBrush blankbrush (RGB (0, 0, 0));
  CBrush brush;
  int i;

  int m_Number = dat->num;
  Star *stars = dat->s;

  for (i = 0; i < m_Number; i++)
  { CSingleLock sLock (&stars[i].m, FALSE);
    if (sLock.Lock () == 0)
    { dat->wnd->MessageBox ("Error");
      break;
    }

    brush.CreateSolidBrush (stars[i].color);

    dc->SelectObject (blankbrush);
    dc->Ellipse ((int) stars[i].px - 2, (int) stars[i].py - 2, (int) stars[i].px + 2, (int) stars[i].py + 2);
    dc->SelectStockObject (NULL_BRUSH);
    //dc->SetPixel ((int) stars[i].px, (int) stars[i].py, RGB (0, 0, 0));

    dc->SelectObject (brush);
    dc->Ellipse ((int) stars[i].x - 2, (int) stars[i].y - 2, (int) stars[i].x + 2, (int) stars[i].y + 2);
    dc->SelectStockObject (NULL_BRUSH);
    //dc->SetPixel ((int) stars[i].x, (int) stars[i].y, stars[i].color);

    brush.DeleteObject ();
    sLock.Unlock ();
  }

  blankbrush.DeleteObject ();

  return 0;    // thread completed successfully
}

/////////////////////////////
// Here is the stars structure:

#ifndef STARH
#define STARH

#include <afxmt.h>

class Star
{ public:
  void Init (double, double);

  volatile double x, y, z;
  volatile double px, py;
  volatile COLORREF color;

  CMutex m;
};

#endif

// And the stars.cpp implementation:
#include "stdafx.h"
#include "star.h"

void Star::Init (double MaxX, double MaxY)
{ x = (double) (rand () % (int) MaxX);
  y = (double) (rand () % (int) MaxY);
  z = (double) (rand () % (int) 384 + 128.0);
  px = 0.0;
  py = 0.0;

  color = RGB ((int) z / 2, (int) z / 2, (int) z / 2);

  //color = RGB (255, 255, 255);
  return;
}


What am I doing wrong?  Why is garbage still being left behind?

Thank you for any help you can provide.  Note that if the solution is more complex than it seems, I can increase the number of points.
0
Comment
Question by:thresher_shark
  • 12
  • 5
17 Comments
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320677
Edited text of question
0
 
LVL 2

Accepted Solution

by:
SamratAshok earned 100 total points
ID: 1320678
One of the things missing from your code is name of the Mutex object.
Each mutex needs to be named uniquely as pointing to a particular star. So obvious name choice is some fixed string concatenated by arrayindex. That could be the cause of whole garbage problem.

You will have to do it at the time of construction of mutex object, so looks like you will have to create mutexes objects all over the heap.

Some other suggesstions (First one is most important )

* Avoid changing x,y,z several times, use a temporary value and assign at the end
when values are ready.

* Try adding a get/ set function for (x,y,z) parameters in the star class.

* Instead of setting and painting all stars in one thread, do a star per thread and
then maybe reduce the timer-slice. (you will need atleast one more global variable
to keep track of next star to be changed)

* Instead of using thread to repaint the screen, simple invalidate portion of screen (with hints ) which will generate WM_PAINT and move the painting over there.

Hope this helps :-)


0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320679
Thank you for your ideas.  I hadn't thought about the name of the mutex.  How would you recommend I generate a unique name for every star?

I also see what you mean about adjusting the stars coordinates all the time, as opposed to your idea which would be better in this case.

Thanks for your help.
0
PRTG Network Monitor: Intuitive Network Monitoring

Network Monitoring is essential to ensure that computer systems and network devices are running. Use PRTG to monitor LANs, servers, websites, applications and devices, bandwidth, virtual environments, remote systems, IoT, and many more. PRTG is easy to set up & use.

 
LVL 6

Author Comment

by:thresher_shark
ID: 1320680
Shoot, forget the first paragraph of the above comment.  I didn't read your answer thoroughly :-)
0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320681
Well, this is strange, after trying your suggestions, the problem still occurs.

I have named each mutex individually.  I then changed the calculating code so it uses temporary variables.  Here is the new calculating code:

void CSpaceSaverDlg::OnTimer(UINT nIDEvent)
{ int i;
  int j = 0;
  double xinc = 0.0, yinc = 0.0, ztemp = 0.0;

  dat->s = stars;
  dat->num = m_Number;
  dat->wnd = this;

  AfxBeginThread (ThreadProc, dat); // Passing global variable?

  // Sleep (50); // Fixes some
  // Sleep (100); // Fixes all the way.

  if (rand () % 1000 == 0)
  { m_Direction = rand () % 8; }

  for (i = 0; i < m_Number; i++)
  { CSingleLock sLock (stars[i].m, FALSE);
    if (sLock.Lock () == 0)
    { MessageBox ("Error");
      break;
    }

    xinc = stars[i].x;
    yinc = stars[i].y;
    ztemp = stars[i].z;

    stars[i].px = xinc;
    stars[i].py = yinc;

    switch (m_Direction)
    { case (0): xinc += ztemp / m_Speed;
                break;

      case (1): xinc += ztemp / m_Speed;
                yinc += ztemp / m_Speed;
                break;

      case (2): yinc += ztemp / m_Speed;
                break;

      case (3): xinc -= ztemp / m_Speed;
                yinc += ztemp / m_Speed;
                break;

      case (4): xinc -= ztemp / m_Speed;
                break;

      case (5): xinc -= ztemp / m_Speed;
                yinc -= ztemp / m_Speed;
                break;

      case (6): yinc -= ztemp / m_Speed;
                break;

      case (7): yinc -= ztemp / m_Speed;
                xinc += ztemp / m_Speed;
                break;
    }

    if (xinc > m_MaxX)
    { xinc -= m_MaxX; }
    else if (xinc < 0.0)
    { xinc += m_MaxX; }

    if (yinc > m_MaxY)
    { yinc -= m_MaxY; }
    else if (yinc < 0.0)
    { yinc += m_MaxY; }

    stars[i].x = xinc;
    stars[i].y = yinc;
    //stars[i].z = ztemp; // No changes to z so no need to change
    sLock.Unlock ();
  }

  CDialog::OnTimer(nIDEvent);
}

/////////////////////////////////
//
// I have not changed the drawing code at all.  Here is the new // star structure (for now):
//
/////////////////////////////////
/////// Stars.cpp:

#include "stdafx.h"
#include "star.h"

void Star::Init (double MaxX, double MaxY, int index)
{ x = (double) (rand () % (int) MaxX);
  y = (double) (rand () % (int) MaxY);
  z = (double) (rand () % (int) 384 + 128.0);
  px = 0.0;
  py = 0.0;

  color = RGB ((int) z / 2, (int) z / 2, (int) z / 2);
  //color = RGB (255, 255, 255);

  CString temp;
  temp.Format ("NameOfMutex%d", index);
  m = new CMutex (FALSE, temp);
  return;
}

Star::~Star ()
{ delete m;
}

///////////////////////////
// Stars.h

#ifndef STARH
#define STARH

#include <afxmt.h>

class Star
{ public:
  ~Star ();
  void Init (double, double, int);

  volatile double x, y, z;
  volatile double px, py;
  volatile COLORREF color;

  CMutex *m;
};

#endif


/////

But now for the really strange part.  If I un-rem the line in the Init function that sets the star's color to 255, 255, 255, it WORKS!  But otherwise it doesn't work!

It is probably something very simple, that I am just missing, but I don't know.

In any case, thank you very much for your help.
0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320682
If you want, I can email you the whole project and you can see this "phenomenon" in action.
0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320683
Here is some information I collected:

color = RGB (255, 255, 255); // Works
color = RGB (254, 254, 254); // Doesn't work
color = RGB (253, 253, 253); // Doesn't work
color = RGB (252, 252, 252); // Doesn't work

color = RGB (128, 128, 128); // Works
color = RGB (129, 129, 129); // Doesn't work

color = RGB (192, 192, 192); // Works

It seems things that have to do with a power of two work well.  But 255 doesn't have much to do with that except 2^8 - 1 = 255.  Anyway... I still don't understand.  Thanks for your help.
0
 
LVL 2

Expert Comment

by:SamratAshok
ID: 1320684
This color thing is really strange. Send me the code if you can as an attachment.
My email address is cdjindia@bestweb.net

Also lemme know what compiler and what version are you using for this project.
 
0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320685
I have sent the project.  I have Visual C++ with SP3.  I hope your mail reader understands the MIME format for email.
0
 
LVL 2

Expert Comment

by:SamratAshok
ID: 1320686
I  have lost your email address. please send a fake mail either at
cdjindia@bestweb.net or cdjindia@hotmail.com and I can mail you the
new code.


0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320687
I have sent you the message.
0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320688
Have you sent the new code yet?  I am just dying to see what the problem is :-)
0
 
LVL 2

Expert Comment

by:SamratAshok
ID: 1320689

Yes, I sent it already on Friday. Earliest I can send it again is now on Monday because I don't have that file at home machine. I sent it to your @gte account.


0
 
LVL 2

Expert Comment

by:SamratAshok
ID: 1320690
On second thoughts (to keep you from dying ... :-; )

Implementation that you have for painting the stars is not actually as per microsoft API
specifications.
0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320691
It might not have arrived yet because it's large, the mail servers probably have priorities and all kinds of other stuff to prevent themselves from getting too overloaded.
0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320692
Hello again,

I found a small problem with my drawing code, I was not releasing the DC after I was done with it, however the garbage problem is still ocurring.  Can you please be more specific with what the problem was?  After all, anyone following this thread would probably like to know, especially if they had to pay for it :-)

Thank you so much for all of your help so far.
0
 
LVL 6

Author Comment

by:thresher_shark
ID: 1320693
Adjusted points to 100
0

Featured Post

Does Powershell have you tied up in knots?

Managing Active Directory does not always have to be complicated.  If you are spending more time trying instead of doing, then it's time to look at something else. For nearly 20 years, AD admins around the world have used one tool for day-to-day AD management: Hyena. Discover why

Question has a verified solution.

If you are experiencing a similar issue, please ask a related question

Suggested Solutions

Title # Comments Views Activity
Different colored text in ComboBox without Subclassing 8 62
Question regarding Copy/Paste 16 96
Expand to include initial dialog with two choices. 9 78
Path of Workbook 3 79
This is to be the first in a series of articles demonstrating the development of a complete windows based application using the MFC classes.  I’ll try to keep each article focused on one (or a couple) of the tasks that one may meet.   Introductio…
Introduction: Database storage, where is the exe actually on the disc? Playing a game selected randomly (how to generate random numbers).  Error trapping with try..catch to help the code run even if something goes wrong. Continuing from the seve…
This Micro Tutorial will teach you how to censor certain areas of your screen. The example in this video will show a little boy's face being blurred. This will be demonstrated using Adobe Premiere Pro CS6.
This video will show you how to get GIT to work in Eclipse.   It will walk you through how to install the EGit plugin in eclipse and how to checkout an existing repository.

831 members asked questions and received personalized solutions in the past 7 days.

Join the community of 500,000 technology professionals and ask your questions.

Join & Ask a Question