View Issue Details

Jump to NotesJump to History
IDProjectCategoryView StatusDate SubmittedLast Update
0002183NoesisGUIC++ SDKpublic2021-11-09 16:152021-11-12 12:34
Reportersteveh Assigned Tosfernandez  
PrioritynormalSeveritymajorReproducibilityalways
Status resolvedResolutionfixed 
Product Version3.1 
Target Version3.1.2Fixed in Version3.1.2 
Summary0002183: Assert in TimeManager::RemoveClock when removing a pending storyboard
DescriptionHi guys,

We're getting a reproducible crash caused by rapidly changing visual states when an element first loads. I've fixed the XAML which caused this (was a data trigger invoking a GoToStateAction fighitng with some code which calls VisualStateManager::GoToState).

The assert was coming from here:

void TimeManager::RemoveClock(Clock* clock, bool clearValue)
{
    NS_ASSERT(mMasterClocks.Find(clock) != mMasterClocks.End() && clock->HasControllableRoot());
    RemoveClocks(clock, clearValue);
}


The issue is that the clock had been registered and was in the mPendingMasterClocks map, and we hadn't had a tick to start the storyboard yet. I've also made a local patch to fix this, though I'm not 100% sure that the fix makes sense. This is my change, take note of my comments:

void TimeManager::RemoveClock(Clock* clock, bool clearValue)
{
    // Begin: SPH [2021-11-09] Ensure we detect pending master clocks before trying to remove a clock that's already started!
    NS_ASSERT(clock->HasControllableRoot());
    if (HasPendingAnimations())
    {
        // Remove the clock from the pending clocks if it hasn't started yet.
        Vector<Clock*>::Iterator iter = mPendingMasterClocks.Find(clock);
        if (iter != mPendingMasterClocks.End())
        {
            mPendingMasterClocks.Erase(iter);
            return;
        }
    }
    // If we get here it must be a master clock
    NS_ASSERT(mMasterClocks.Find(clock) != mMasterClocks.End());
    // End: SPH [2021-11-09]
    RemoveClocks(clock, clearValue);
}


My fix is to simply detect if the clock exists in the pending master clock map. If it does, remove it from there and early out so the RemoveClocks function is not invoked. This seemed to fix out issue without introducing any strange side effects from what I could see.
Steps To Reproduce1. Create a visual state group
2. Call GoToVisualState twice on the same frame.
3. When it changes state on the second attempt it will try to remove the first clock which is in the pending clock group.
TagsNo tags attached.
PlatformAny

Activities

steveh

steveh

2021-11-09 22:50

reporter   ~0007560

I've been running with this change a little while and have noticed some issues. There are times when the clock is in *both* the master list and the pending list. If I don't remove it from the master list the animations never stop being applied so it remains in the original visual state. I've fixed this with the following change:

void TimeManager::RemoveClock(Clock* clock, bool clearValue)
{
    // https://www.noesisengine.com/bugs/view.php?id=2183
    // Begin: SPH [2021-11-09] Ensure we detect pending master clocks before trying to remove a clock that's already started!
    NS_ASSERT(clock->HasControllableRoot());

    if (HasPendingAnimations())
    {
        // Remove the clock from the pending clocks if it hasn't started yet.
        Vector<Clock*>::Iterator iter = mPendingMasterClocks.Find(clock);
        if (iter != mPendingMasterClocks.End())
        {
            mPendingMasterClocks.Erase(iter);

            // This might not exist in the master clocks, so only run this if we had a valid master clock.
            if (mMasterClocks.Find(clock) != mMasterClocks.End())
            {
                RemoveClocks(clock, clearValue);
            }
            return;
        }
    }
    // If we get here it must be a master clock
    NS_ASSERT(mMasterClocks.Find(clock) != mMasterClocks.End());
    // End: SPH [2021-11-09]
    RemoveClocks(clock, clearValue);
}

Again, not sure if this is the correct thing to do, but seems to work a little better.
sfernandez

sfernandez

2021-11-10 12:47

manager   ~0007561

Hi, I wasn't able to reproduce the assert by calling GoToVisualState twice in a frame.
But looking at the code, everytime a master clock is registered, it gets added to both mMasterClocks and mPendingMasterClocks. I don't see how it can be in mPendingMasterClocks and not in mMasterClocks when trying to remove it, so I think it would be enough to just check if it exists in mMasterClocks before removing it (which will remove it from both lists), something like this:
void TimeManager::RemoveClock(Clock* clock, bool clearValue)
{
    NS_ASSERT(clock->HasControllableRoot());
    if (mMasterClocks.Find(clock) != mMasterClocks.End())
    {
        RemoveClocks(clock, clearValue);
    }
}
sfernandez

sfernandez

2021-11-12 10:48

manager   ~0007568

Could you please confirm that my code is also working in your game?
steveh

steveh

2021-11-12 11:12

reporter   ~0007569

Hey Sergio, I've just tested your change and it works great. Much appreciated.
sfernandez

sfernandez

2021-11-12 12:34

manager   ~0007570

Thanks for the confirmation, I've included the fix for the next release.

Issue History

Date Modified Username Field Change
2021-11-09 16:15 steveh New Issue
2021-11-09 22:50 steveh Note Added: 0007560
2021-11-10 10:49 sfernandez Assigned To => sfernandez
2021-11-10 10:49 sfernandez Status new => assigned
2021-11-10 10:49 sfernandez Target Version => 3.1.2
2021-11-10 12:47 sfernandez Status assigned => feedback
2021-11-10 12:47 sfernandez Note Added: 0007561
2021-11-12 10:48 sfernandez Note Added: 0007568
2021-11-12 11:12 steveh Note Added: 0007569
2021-11-12 11:12 steveh Status feedback => assigned
2021-11-12 12:34 sfernandez Status assigned => resolved
2021-11-12 12:34 sfernandez Resolution open => fixed
2021-11-12 12:34 sfernandez Fixed in Version => 3.1.2
2021-11-12 12:34 sfernandez Note Added: 0007570