isl-org / spear

SPEAR: A Simulator for Photorealistic Embodied AI Research
MIT License
224 stars 15 forks source link

Upgrade UrdfRobot, CoreUtils, SimulationController plugins #258

Closed RachithP closed 9 months ago

RachithP commented 10 months ago
mikeroberts3000 commented 9 months ago

Some clean-up and interface refinements to do but otherwise this PR is looking very good. Comment here when you think all items are addressed and it is ready to review again.

mikeroberts3000 commented 9 months ago

I've decided that the rules and conventions around CreateDefaultSubcomponent vs NewComponent, SetupAttachment vs no SetupAttachment, and RegisterComponent vs no RegisterComponent are too confusing. I propose to add the following functions to Unreal.h, and to use them throughout the code where appropriate.

template <typename TComponent>
TComponent* Unreal::createComponentInsideOwnerConstructor(const std::string& name, AActor* parent)
{
    // CreateDefaultSubobject is required when inside the constructor of the owner AActor (either directly or indirectly via
    // the constructor of a child component). SetRootComponent is required to attach the newly created root component to its
    // owner AActor.
    SP_ASSERT(parent);
    TComponent* component = CreateDefaultSubobject<TComponent>(Unreal::toFName(name));
    SP_ASSERT(component);
    parent->SetRootComponent(component);
}

template <typename TComponent>
TComponent* Unreal::createComponentInsideOwnerConstructor(const std::string& name, UActorComponent* parent)
{
    // CreateDefaultSubobject is required when inside the constructor of the owner AActor (either directly or indirectly via
    // the constructor of a child component). SetupAttachment is required to connect the newly created component to its
    // parent component.
    SP_ASSERT(parent);
    TComponent* component = CreateDefaultSubobject<TComponent>(Unreal::toFName(name));
    SP_ASSERT(component);
    component->SetupAttachment(parent);
}

template <typename TComponent>
TComponent* Unreal::createComponentOutsideOwnerConstructor(const std::string& name, AActor* parent)
{
    // NewObject and RegisterComponent are required when outside the constructor of the owner AActor (either directly or
    // indirectly via the constructor of a child component). SetRootComponent is required to attach the newly created root
    // component to its owner AActor. The Unreal codebase usually calls SetRootComponent before calling RegisterComponent
    // so we follow the same convention convention here.
    TComponent* component = NewObject<TComponent>(parent, Unreal::toFName(name));
    SP_ASSERT(component);
    parent->SetRootComponent(component);
    component->RegisterComponent();
}

template <typename TComponent>
TComponent* Unreal::createComponentOutsideOwnerConstructor(const std::string& name, UActorComponent* parent)
{
    // NewObject and RegisterComponent are required when outside the constructor of the owner AActor (either directly or
    // indirectly via the constructor of a child component). SetupAttachment is required to attach the newly created
    // component to its parent UActorComponent. The Unreal codebase usually calls SetupAttachment before calling
    // RegisterComponent so we follow the same convention convention here.
    SP_ASSERT(parent);
    TComponent* component = NewObject<TComponent>(parent, Unreal::toFName(name));
    SP_ASSERT(component);
    component->SetupAttachment(parent);
    component->RegisterComponent();
}

Does the behavior of each function look correct to you?

RachithP commented 9 months ago

I've decided that the rules and conventions around CreateDefaultSubcomponent vs NewComponent, SetupAttachment vs no SetupAttachment, and RegisterComponent vs no RegisterComponent are too confusing. I propose to add the following functions to Unreal.h, and to use them throughout the code where appropriate.

template <typename TComponent>
TComponent* Unreal::createComponentInsideOwnerConstructor(const std::string& name, AActor* parent)
{
    // CreateDefaultSubobject is required when inside the constructor of the owner AActor (either directly or indirectly via
    // the constructor of a child component). SetRootComponent is required to attach the newly created root component to its
    // owner AActor.
    SP_ASSERT(parent);
    TComponent* component = CreateDefaultSubobject<TComponent>(Unreal::toFName(name));
    SP_ASSERT(component);
    parent->SetRootComponent(component);
}

template <typename TComponent>
TComponent* Unreal::createComponentInsideOwnerConstructor(const std::string& name, UActorComponent* parent)
{
    // CreateDefaultSubobject is required when inside the constructor of the owner AActor (either directly or indirectly via
    // the constructor of a child component). SetupAttachment is required to connect the newly created component to its
    // parent component.
    SP_ASSERT(parent);
    TComponent* component = CreateDefaultSubobject<TComponent>(Unreal::toFName(name));
    SP_ASSERT(component);
    component->SetupAttachment(parent);
}

template <typename TComponent>
TComponent* Unreal::createComponentOutsideOwnerConstructor(const std::string& name, AActor* parent)
{
    // NewObject and RegisterComponent are required when outside the constructor of the owner AActor (either directly or
    // indirectly via the constructor of a child component). SetRootComponent is required to attach the newly created root
    // component to its owner AActor. The Unreal codebase usually calls SetRootComponent before calling RegisterComponent
    // so we follow the same convention convention here.
    TComponent* component = NewObject<TComponent>(parent, Unreal::toFName(name));
    SP_ASSERT(component);
    parent->SetRootComponent(component);
    component->RegisterComponent();
}

template <typename TComponent>
TComponent* Unreal::createComponentOutsideOwnerConstructor(const std::string& name, UActorComponent* parent)
{
    // NewObject and RegisterComponent are required when outside the constructor of the owner AActor (either directly or
    // indirectly via the constructor of a child component). SetupAttachment is required to attach the newly created
    // component to its parent UActorComponent. The Unreal codebase usually calls SetupAttachment before calling
    // RegisterComponent so we follow the same convention convention here.
    SP_ASSERT(parent);
    TComponent* component = NewObject<TComponent>(parent, Unreal::toFName(name));
    SP_ASSERT(component);
    component->SetupAttachment(parent);
    component->RegisterComponent();
}

Does the behavior of each function look correct to you?

Yes, I think these behaviors are correct. I'll update the code with these changes.

mikeroberts3000 commented 9 months ago

As discussed in our meeting today, CreateDefaultSubject is a public method on UObject, not a free function like NewObject, so the code snippet above needs to be modified slightly.

template <typename TComponent>
TComponent* Unreal::createComponentInsideOwnerConstructor(AActor* parent, const std::string& name)
{
    return createComponentInsideOwnerConstructor(parent, parent, name); // assume parent is owner
}

template <typename TComponent>
TComponent* Unreal::createComponentInsideOwnerConstructor(UObject* owner, AActor* parent, const std::string& name)
{
    // CreateDefaultSubobject is required when inside the constructor of the owner AActor (either directly or indirectly via
    // the constructor of a child component). SetRootComponent is required to attach the newly created root component to its
    // owner AActor.
    SP_ASSERT(owner);
    SP_ASSERT(parent);
    TComponent* component = owner->CreateDefaultSubobject<TComponent>(Unreal::toFName(name));
    SP_ASSERT(component);
    parent->SetRootComponent(component);
    return component;
}
RachithP commented 9 months ago

As discussed in our meeting today, CreateDefaultSubject is a public method on UObject, not a free function like NewObject, so the code snippet above needs to be modified slightly.

template <typename TComponent>
TComponent* Unreal::createComponentInsideOwnerConstructor(AActor* parent, const std::string& name)
{
    return createComponentInsideOwnerConstructor(parent, parent, name); // assume parent is owner
}

template <typename TComponent>
TComponent* Unreal::createComponentInsideOwnerConstructor(UObject* owner, AActor* parent, const std::string& name)
{
    // CreateDefaultSubobject is required when inside the constructor of the owner AActor (either directly or indirectly via
    // the constructor of a child component). SetRootComponent is required to attach the newly created root component to its
    // owner AActor.
    SP_ASSERT(owner);
    SP_ASSERT(parent);
    TComponent* component = owner->CreateDefaultSubobject<TComponent>(Unreal::toFName(name));
    SP_ASSERT(component);
    parent->SetRootComponent(component);
    return component;
}

Thanks! For some reason I kept thinking that CreateDefaultSubobject function was a static function! 🙈