?
Solved

Simple multi-threading synchronization question

Posted on 1998-08-12
17
Medium Priority
?
254 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
[X]
Welcome to Experts Exchange

Add your voice to the tech community where 5M+ people just like you are talking about what matters.

  • Help others & share knowledge
  • Earn cash & points
  • Learn & ask questions
  • 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 400 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
Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

 
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

Independent Software Vendors: We Want Your Opinion

We value your feedback.

Take our survey and automatically be enter to win anyone of the following:
Yeti Cooler, Amazon eGift Card, and Movie eGift Card!

Question has a verified solution.

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

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…
In this post we will learn different types of Android Layout and some basics of an Android App.
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.
Do you want to know how to make a graph with Microsoft Access? First, create a query with the data for the chart. Then make a blank form and add a chart control. This video also shows how to change what data is displayed on the graph as well as form…
Suggested Courses

765 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