EventManager functions : errors ?

    This site uses cookies. By continuing to browse this site, you are agreeing to our Cookie Policy.

    • EventManager functions : errors ?

      Hi,
      I'm using the EventManager code, but i feel there are some strange things : are they errors, or did i misunderstand it ?

      In EventManager::delListener() :

      Source Code

      1. // brute force method, iterate through all existing mapping
      3. // entries looking for the matching listener and remove it.
      7. for ( EventListenerMap::iterator it = m_registry.begin(),
      9. itEnd = m_registry.end(); it != itEnd; it++ )
      11. {
      13. unsigned int const kEventId = it->first;
      15. EventListenerTable & table = it->second;
      19. for ( EventListenerTable::iterator it2 = table.begin(),
      21. it2End = table.end(); it2 != it2End; it2++ )
      23. {
      25. if ( *it2 == inListener )
      27. {
      29. // found match, remove from table,
      31. table.erase( it2 );
      35. // update return code
      37. rc = true;
      41. // and early-quit the inner loop as addListener()
      43. // code ensures that each listener can only
      45. // appear in one event's processing list once.
      47. break;
      49. }
      51. }
      53. }
      Display All


      The function doesn't check if the deleted listener is registered for the same event type as the one passed as parameter : is it normal ? (i personnaly added a type checking :

      Source Code

      1. for( EventListenerMap::iterator it = mRegistry.begin(); it != mRegistry.end(); ++it )
      2. {
      3. EventTypeID currEvtTypeID = it->first;
      4. EventListenerList& currListenerList = it->second;
      6. if( currEvtTypeID == _eventType.getID() || _eventType.getStr() == EventType::WILCARD_EVENT_TYPE )
      7. {
      8. // Type matches, look for listener
      9. for( EventListenerList::iterator it2 = currListenerList.begin(); it2 != currListenerList.end(); ++it2 )
      10. {
      11. if( *it2 == _listener )
      12. {
      13. /* Found : remove it, update isRemoved, and break this "sub-loop"
      14. as we're sure a listener can only appear once in a EventListenerList
      15. Note : forgetting the "break" would cause a segfault, as it2 still
      16. points to a removed element */
      17. currListenerList.erase( it2 );
      18. break;
      19. }
      20. }
      21. }
      22. }
      Display All


      In EventManager::trigger() :

      Source Code

      1. bool EventManager::trigger (
      3. Event const & inEvent ) const
      5. {
      7. if ( ! validateType( inEvent.getType() ) )
      9. return false;
      13. EventListenerMap::const_iterator itWC = m_registry.find( 0 );
      17. if ( itWC != m_registry.end() )
      19. {
      21. EventListenerTable const & table = itWC->second;
      25. bool processed = false;
      29. for ( EventListenerTable::const_iterator it2 = table.begin(),
      31. it2End = table.end(); it2 != it2End; it2++ )
      33. {
      35. (*it2)->HandleEvent( inEvent );
      37. }
      39. }
      43. EventListenerMap::const_iterator it =
      45. m_registry.find( inEvent.getType().getIdent() );
      49. if ( it == m_registry.end() )
      51. return false;
      55. EventListenerTable const & table = it->second;
      59. bool processed = false;
      63. for ( EventListenerTable::const_iterator it2 = table.begin(),
      65. it2End = table.end(); it2 != it2End; it2++ )
      67. {
      69. if ( (*it2)->HandleEvent( inEvent ) )
      71. {
      73. // only set to true, if processing eats the messages
      75. processed = true;
      77. }
      79. }
      83. return processed;
      85. }
      Display All


      The way the boolean value is handled seems strange : shouldn't be something like this (function is exited as soon as the event is consumed) ?

      Source Code

      1. bool EventManager::trigger ( Event const & inEvent ) const
      2. {
      3. if ( ! validateType( inEvent.getType() ) )
      4. return false;
      6. EventListenerMap::const_iterator itWC = m_registry.find( 0 );
      8. if ( itWC != m_registry.end() )
      9. {
      10. EventListenerTable const & table = itWC->second;
      11. for ( EventListenerTable::const_iterator it2 = table.begin(), it2End = table.end(); it2 != it2End; it2++ )
      12. {
      13. if( (*it2)->HandleEvent( inEvent ) ) return true;
      14. }
      15. }
      17. EventListenerMap::const_iterator it = m_registry.find( inEvent.getType().getIdent() );
      19. if ( it == m_registry.end() )
      20. return false;
      22. EventListenerTable const & table = it->second;
      23. bool processed = false;
      25. for ( EventListenerTable::const_iterator it2 = table.begin(),
      26. it2End = table.end(); it2 != it2End; it2++ )
      27. {
      28. if ( (*it2)->HandleEvent( inEvent ) )
      29. {
      30. // only set to true, if processing eats the messages
      31. return true;
      32. }
      33. }
      34. return false
      35. }
      Display All


      And, last, in EventManager::abortEvent() :
      for ( EventQueue::iterator it = m_queues[m_activeQueue].begin(), itEnd = m_queues[m_activeQueue].end(); it != itEnd; it++ )
      {
      if ( (*it)->getType() == inType )
      {
      m_queues[m_activeQueue].erase(it);
      rc = true;

      if ( !allOfType )
      break;
      }
      }

      What happens if allOfType is true ? "it" is no longer valid, as it has been erased, and we are about to make "++it". Shouldn't rather be "it = m_queues[m_activeQueue].erase(it);" ?

      The post was edited 1 time, last by Ceacy ().

    • For the first situation (::delListener), Ceacy is absolutely right. The code seems to be acting as if the event is always based off of 'kpWildcardEventType'.

      For the second situation (::trigger), Ceacy is right again, but this one also brings to light an interesting question concerning the following line:

      Source Code

      1. EventListenerMap::const_iterator itWC = m_registry.find( 0 )

      This looks like it is doing a map lookup with 0 as the key, which means some event string identifier predictably hashes to zero or a zero key was specifically inserted into that m_registry map. I browsed the code and could find no reason for this. There are two for loops in that ::trigger function, but I don't know what the first for loop does, exactly. Does it need to be there?

      The third situation also seems to be an error as Ceacy pointed out, and I like his solution of using

      Source Code

      1. it = m_queues[m_activeQueue].erase(it);


      Three great catches, Ceacy!

    • I'm going to need to go back into the code and think a bit on these, but here's some off the cuff comments (original author of this bit here ...)

      I do not believe the first one pointed out is actually an error in that one list may have different types of event listeners inserted into it, and there is no requirment that a replaced lisenter have the same type. Building in this requirement may work for your specific uses, and is not inherently a bad thing I guess, but is certainly not something necessary to enforce and not as generic ... no code will be harmed with heterogenious listener types nor with mutating listener types.

      The explicit zero reference is actually documented in the eventsystem.h file ... and how it get's there ... it represents wildcard listeners as denoted by the wildcard event type string of "*" ( or the kpWildcardEventType constant from the header ) - the actual conversion is done as an early out in cESEventType::hash_name().

      The use of the bool is correct, changing to put an early return there will break the code when the list has more than one listener that is trying to get that event. Again, documented in the EventSytem.h header. The intent was not to stop processing at the first consumer, but to return whether the event HAD BEEN CONSUMED AT ALL BY ANYBODY. ;^)

      AllOfType abort-event bug: Nice spot. But you also need to change the list a bit more than just taking the return from erase() or you may end up skipping entires in the list. Remember that erase() will return the _next_ entry, and your loop end will iterate it again, thus potentially skipping interesting entries (or inrementing beyond the end of the list, which results in probably segfault). So the complete fix would look something like...

      Source Code

      1. for ( EventQueue::iterator it = m_queues[m_activeQueue].begin(), itEnd = m_queues[m_activeQueue].end(); it != itEnd; )
      2. {
      3. if ( (*it)->getType() == inType )
      4. {
      5. it = m_queues[m_activeQueue].erase(it);
      6. rc = true;
      8. if ( !allOfType )
      9. break;
      10. }
      11. else
      12. {
      13. ++it;
      14. }
      15. }
      Display All

    • These fixes will be published in GCC 3.0, actually. Sorry for the long wait.
      Mr.Mike
      Author, Programmer, Brewer, Patriot

    • Wait a second

      Maybe I'm missing something, but this version doesn't seem right either. Shouldn't we really be doing this?

      Source Code

      1. for ( EventQueue::iterator it = m_queues[m_activeQueue].begin(), itEnd = m_queues[m_activeQueue].end(); it != itEnd; it++)
      2. {
      3. if ( (*it)->getType() == inType )
      4. {
      5. // This invalidates itEnd
      6. it = m_queues[m_activeQueue].erase(it);
      8. // This doesn't feel right, but there aren't many other options
      9. itEnd = m_queues[m_activeQueue].end();
      11. rc = true;
      13. if ( !allOfType )
      14. break;
      15. }
      16. else
      17. {
      18. // erase() leaves it pointing to the element after the one that got erased.
      19. // Decrement it so it doesn't get skipped at the top of the loop
      20. --it;
      21. }
      22. }
      Display All


      Edit: Doh. The code in this thread doesn't have the increment at the top of the loop. And my version wouldn't work anyway, if you just deleted the only entry in the queue.

      And Queue is a list, not a vector. So that whole "invalidated" thing is wrong, too.

      In my defense, (unless it's been fixed since I downloaded it), the "official" source also has the extra it++ in the for loop.

      The post was edited 2 times, last by jimrthy ().