isl-org / spear

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

Expose `UnrealClassRegistrar.h` functions to python API #284

Closed RachithP closed 4 months ago

mikeroberts3000 commented 4 months ago

This PR is looking great. None of the changes I suggested are super complicated, but they might touch a lot of the new code, so I'd like to have one more look before approving.

mikeroberts3000 commented 4 months ago

Here is a design sketch for how I think we should synchronize Unreal enums across Python and C++.

Suppose there is some underlying Unreal enum that is not tagged as a UENUM that we would like to be able to easily pass to and from Python.

enum class UnrealEnum
{
    Zero = 0,
    One  = 1,
    Two  = 2
};

Our first step is to declare a new UENUM with matching values. No matter what approach we use, we will need to declare an extra enum somewhere. I think it's better to do this in C++ rather than Python, because we can avoid explicitly referring to the raw underlying integer values.

UENUM()
enum class ESpEnum
{
    Zero = static_cast<std::underlying_type_t<UnrealEnum>>(UnrealEnum::Zero),
    One  = static_cast<std::underlying_type_t<UnrealEnum>>(UnrealEnum::One),
    Two  = static_cast<std::underlying_type_t<UnrealEnum>>(UnrealEnum::Two)
};

Our next step is to declare a USTRUCT that contains our UENUM. We don't need to do anything similar in the MSGPACK approach, so the fact that we need this extra boilerplate is a legitimate downside of my proposed approach. But it will provide a lot of extra convenience in the following step.

USTRUCT()
struct FSpEnum
{
    GENERATED_BODY()

    UPROPERTY()
    ESpEnum Enum;
};

Now suppose we want to call an Unreal function from a service entry point, where the Unreal function takes as input an FVector, an FRotator, and an UnrealEnum. We can do this very conveniently using the UnrealObj class that we're already using in other parts of our code.

UnrealObj<FVector> location("Location");
UnrealObj<FRotator> rotation("Rotation");
UnrealObj<FSpEnum> sp_enum("Enum");
UnrealObjUtils::setObjectPropertiesFromStrings({&location, &rotation, &sp_enum}, unreal_obj_strings);

// this cast operation is hideous but we can wrap it in a helper function if we find that we are doing it often
UnrealEnum unreal_enum = static_cast<UnrealEnum>(static_cast<std::underlying_type_t<ESpEnum>>(sp_enum.get().Enum));

GEngine->UnderlyingUnrealFunction(location.get(), rotation.get(), unreal_enum);

In this example, the type of unreal_obj_strings is std::map<std::string, std::string>, and its key-value pairs should be set as follows.

{
   "X": 1.23
   "Y": 4.56
   "Z": 7.89
}
{
   "Roll": 1.23
   "Pitch": 4.56
   "Yaw": 7.89
}
{
   "Enum": "Two"
}

I think this approach is preferable to the MSGPACK approach for the following reasons.

If you are persuaded by this reasoning, you might think that we should use this approach for all structs and enums, and we should never use the MSGPACK approach for anything. But I still like the MSGPACK approach for structs and enums that we expect to use in tight loops, e.g., everything in CppFuncService. GameWorldServicePropertyDesc is a less obvious case (I could see reasonable arguments for each approach), but I think it is reasonable to leave it how it is. If we do that, then we have a reasonable rule of thumb that we can use to decide which approach to use for a given struct or enum. If we are trying to expose an existing Unreal struct or enum, prefer the USTRUCT/UENUM approach. But if we are trying to expose one of our own structs or enums, prefer the MSGPACK approach.

mikeroberts3000 commented 4 months ago

Hey Rachith, you marked this as being ready to review, but there are a still a bunch of unaddressed comments. Was that intended? Did you want me to review again?

RachithP commented 4 months ago

Hey Rachith, you marked this as being ready to review, but there are a still a bunch of unaddressed comments. Was that intended? Did you want me to review again?

Hey Mike, It wasn't intended for another round of review, I changed it as the draft state didn't make sense at this stage of this PR.