View Issue Details

IDProjectCategoryView StatusLast Update
0002282NoesisGUIUnrealpublic2023-02-17 16:41
Reporterlowprofile Assigned Tohcpizzi  
PrioritynormalSeverityminor 
Status resolvedResolutionfixed 
Product Version3.1.1 
Target Version3.1.7 
Summary0002282: [Unreal Editor] NoesisObjectWrapper cleanup interfering with view models data context cleanup
Description

We developed a way to create blueprint view models from XAML via custom "data context properties" elements.
When we do so, we pass to the property the path to the blueprint class and the property instantiates an instance of that class dynamically :

<UserControl x:Class="view.start_screen"
common:DataContextProperty.DataContextBP="/Game/UI/Noesis/viewmodels/StartScreen/StartScreenViewModel.StartScreenViewModel_C">
</UserControl>

void FDataContextProperty::OnLoadedBP(Noesis::BaseComponent const InObj, Noesis::RoutedEventArgs const& /InArgs/)
{
Noesis::FrameworkElement
const Element = Noesis::DynamicCast<Noesis::FrameworkElement*>(InObj);
checkf(Element != nullptr, TEXT("DataContextBP property can only be attached to only FrameworkElement"));
Element->Loaded() -= OnLoadedBP;
utils::FViewModelUtils::SetDataContext(Element, GetDataContextBP(Element).Str());
}

void FDataContextProperty::OnDataContextBPChanged(Noesis::DependencyObject const InObj, Noesis::DependencyPropertyChangedEventArgs const& /InArgs/)
{
Noesis::FrameworkElement
const Element = Noesis::DynamicCast<Noesis::FrameworkElement*>(InObj);
checkf(Element != nullptr, TEXT("DataContextBP property can only be attached to only FrameworkElement"));
if (Element->GetView())
{
utils::FViewModelUtils::SetDataContext(Element, GetDataContextBP(InObj).Str());
}
else
{
Element->Loaded() += OnLoadedBP;
}
Element->Initialized() += OnInitializedBP;
}

Now for those dynamically created blueprint view models to stay loaded, we have to keep a reference to them somewhere. To achieve that, we added a map in our parent NoesisInstance view that keeps references to all currently active blueprint view model instance. Each time we create a VM that way, we register it to that map so that it doesn't get garbage collected while that VM is in use :

Noesis::Ptr<Noesis::BaseComponent> FUIViewModelLocator::CreateViewModel(FString const& InBlueprintPath)
{
checkf(!InBlueprintPath.IsEmpty(), TEXT("All BP view models should have a path"));
UClass BlueprintClass = LoadClass<UViewModelBase>(nullptr, InBlueprintPath);
if (BlueprintClass != nullptr)
{
UViewModelBase* BlueprintObj = NewObject<UViewModelBase>(GetTransientPackage(), BlueprintClass);
Noesis::Ptr<Noesis::BaseComponent> DataContext = Noesis::Ptr<Noesis::BaseComponent>(NoesisCreateComponentForUObject(BlueprintObj));
H13NoesisInstance->AddViewModelInstance(BlueprintObj);

    return DataContext;
}
UE_LOG(LogSysNoesis, Error, TEXT("Failed to load blueprint view model class at %s"), *InBlueprintPath);
return nullptr;

}

But obviously we then have to clean this up. ie. if we assign a different BP data context to something, or if we just remove elements that have those BP data context assigned, we have to remove their respective entries from the map (and allow garbage collection to now operate on those objects that are now no longer in use).
To achieve this, we listen to the OnDestroy and OnPropertyChanged event on the element that the data context property is attached to. When those are called, we grab the element data context object as it was, remove it from the map, and reinstantiate a new one, in the case of the OnPropertychanged, for the new data context to assign.

void FDataContextProperty::OnDestroyBP(Noesis::DependencyObject const InDependencyObject)
{
Noesis::FrameworkElement
const Element = Noesis::DynamicCast<Noesis::FrameworkElement*>(InDependencyObject);
checkf(Element != nullptr, TEXT("DataContextBP or DataContextBPShared property can only be attached to only FrameworkElement"));
utils::FViewModelUtils::ClearDataContext(Element);
Element->Destroyed() -= OnDestroyBP;
}

void FViewModelUtils::ClearDataContext(Noesis::FrameworkElement const InFrameworkElement)
{
checkf(InFrameworkElement != nullptr, TEXT("We need a valid Framework element to clear the context from"));
Noesis::BaseComponent
const DataContext = InFrameworkElement->GetDataContext();
// We might not have set any context on an element voluntarily if that element was for example created just for the UE editor (see other comments above)
if (DataContext != nullptr)
{
UViewModelBase* const BlueprintObj = Cast<UViewModelBase>(NoesisCreateUObjectForComponent(DataContext));
// the Noesis instance is itself a view model that doesn't derive from UViewModelBase and can still call this method
if (BlueprintObj != nullptr)
{
BlueprintObj->GetH13NoesisInstance()->GetViewModelLocator()->DestroyViewModel(BlueprintObj);
}
InFrameworkElement->SetDataContext(nullptr);
}
}

void FUIViewModelLocator::DestroyViewModel(UViewModelBase* const BlueprintObj)
{
checkf(IsValid(BlueprintObj) && !BlueprintObj->IsUnreachable(), TEXT("Trying to destroy a null view model object"));
BlueprintObj->OnDestruction();
H13NoesisInstance->RemoveViewModelInstance(BlueprintObj);
}

Now the problem comes when we shutdown the game (ie. press stop in the editor or just close down the editor while the game is being played).
At that moment, we unload all our elements and all our UObjects at the same time. And when OnDestroy gets called on elements that are attached to a BP data context, that data context UObject has already been unloaded or is on the verge of being unloaded (ie. IsUnreachable returns true on it).
So the FViewModelUtils::ClearDataContext method above, that would normally call NoesisCreateUObjectForComponent to convert from the BaseComponent attached to our custom property to the actual UObject it contains, fails, because NoesisCreateUObjectForComponent actually returns null at that point. Even worse, it can sometimes fail to DynamicCast the component into a NoesisObjectWrapper, since the NoesisObjectWrapper::GetClassType() method only returns a valid type if its wrapped UObject is actually still valid. This leads to NoesisCreateUObjectForComponent actually trying to reconstruct a new NoesisObjectWrapper causing an exception.

To fix this, I added a new method to the NoesisTypeClass.h/cpp named NoesisHasUObjectForComponent.
This method essentially does what NoesisCreateUObjectForComponent checks at the beginning but leaves out the rest like this :

NOESISRUNTIME_API bool NoesisHasUObjectForComponent(Noesis::BaseComponent* Component)
{
if (Component == nullptr)
return false;

NoesisObjectWrapper* Wrapper = Noesis::DynamicCast<NoesisObjectWrapper*>(Component);
if (Wrapper != nullptr)
{
    return (ObjectMap.FindKey(Wrapper) != nullptr);
}

return false;

}

So if the wrapped object is already unloaded and the ObjectMap was already cleaned up, or if the DynamicCast fails and the returned wrapper variable is null, we return false.
That way, I was able to change my initial FViewModelUtils::ClearDataContext method this way :

void FViewModelUtils::ClearDataContext(Noesis::FrameworkElement const InFrameworkElement)
{
checkf(InFrameworkElement != nullptr, TEXT("We need a valid Framework element to clear the context from"));
Noesis::BaseComponent
const DataContext = InFrameworkElement->GetDataContext();

// We might not have set any context on an element voluntarily if that element was for example created just for the UE editor (see other comments above)
if (DataContext != nullptr)
{
    // we can be clearing data context as the UObject is also being destroyed, in which case Noesis might have already cleared its NoesisObjectWrapper part
    // in that case, NoesisHasUObjectForComponent should return false
    if (NoesisHasUObjectForComponent(DataContext))
    {
        UViewModelBase* const BlueprintObj = Cast<UViewModelBase>(NoesisCreateUObjectForComponent(DataContext));
        // the Noesis instance is itself a view model that doesn't derive from UViewModelBase and can still call this method
        if (BlueprintObj != nullptr)
        {
            BlueprintObj->GetH13NoesisInstance()->GetViewModelLocator()->DestroyViewModel(BlueprintObj);
        }
    }

    InFrameworkElement->SetDataContext(nullptr);
}

}

This made it now work safely on shutdown.

PlatformAny

Activities

hcpizzi

hcpizzi

2023-02-17 16:41

developer   ~0008294

I've added a function NoesisFindUObjectForComponent that you could use similarly to your proposed NoesisHasUObjectForComponent.

Issue History

Date Modified Username Field Change
2022-02-14 19:08 Ferdinand Fayollet New Issue
2022-02-14 19:13 Ferdinand Fayollet Description Updated
2022-02-14 19:57 sfernandez Assigned To => hcpizzi
2022-02-14 19:57 sfernandez Status new => assigned
2022-02-14 19:57 sfernandez Target Version => 3.1.4
2022-03-17 21:31 sfernandez Target Version 3.1.4 => 3.1.5
2022-03-22 11:48 Ferdinand Fayollet Reporter Ferdinand Fayollet => lowprofile
2022-06-24 17:21 sfernandez Target Version 3.1.5 => 3.1.6
2022-11-07 17:14 sfernandez Target Version 3.1.6 => 3.1.7
2023-02-17 16:41 hcpizzi Status assigned => resolved
2023-02-17 16:41 hcpizzi Resolution open => fixed
2023-02-17 16:41 hcpizzi Note Added: 0008294