IDI-Systems / UnrealImGui

Unreal plug-in that integrates Dear ImGui framework into Unreal Engine 4/5.
MIT License
121 stars 27 forks source link

Add `ToggleInput` keybinding support for all `UPlayerInput` based systems (including EnhancedInput & custom) #17

Closed Herhangi closed 4 months ago

Herhangi commented 5 months ago

With the engine's recent change to defaulting to EnhancedInput, UnrealImGui's ToggleInput keybinding doesn't properly register itself during initialization and requires itself to be reconfigured while in PIE; see: https://github.com/IDI-Systems/UnrealImGui/issues/13

This change aims to fix this in 2 steps;

jonpas commented 5 months ago

Neat!

Does UE not have any API to get all UPlayerInput objects so we could avoid iterating through all the objects in the world? Maybe using TObjectIterator with UPlayerInput and again on the base class directly as well?

Herhangi commented 5 months ago

Firstly, I think there's a bit of misconception about the TObjectIterator<UClass> here, it doesn't iterate through all objects in the world, only the class objects which is still not a small about, at the end of development cycle there would be about 5000 UClass objects, but I think in terms of performance comparison it's not a lot more expensive then TObjectIterator<UPlayerInput>. The problem here is we sadly need to know the classes that implements UPlayerInput. There's a faster function that does this called GetDerivedClasses but it doesn't include the original class in the results so code becomes a bit more messy;

TArray<UClass*> InputClasses;
GetDerivedClasses(UPlayerInput::StaticClass(), InputClasses, true);
InputClasses.Add(UPlayerInput::StaticClass());
for (const UClass* InputClass : InputClasses)
{
    UpdatePlayerInput(Cast<UPlayerInput>(InputClass->GetDefaultObject()), KeyBind);
}

Most performant implementation would be to actually create a new UPROPERTY in the settings so that user can provide which player input class they are using but I think that has a problem in cases where user has multiple player input classes that they are switching between and they also need to remember to change this field if they decide to move to a new player input class at some point in development.

All things considered, because this function is executed only once during the initialization and even in the development editor build taking less than 0.2 milliseconds I think it doesn't really need other optimizations and keeping it readable like this would be a good approach.

P.S: I wanted to compare execution times with the GetDerivedClasses function as well and that optimization actually makes the function work in about 0.01 milliseconds (20x faster) but at this point I think it's negligible.

jonpas commented 5 months ago

Thanks for the detailed breakdown!

20-fold improvement is pretty substantial, even if we are talking in milliseconds. I would argue using GetDerivedClass here is definitely preferred, even if it makes the function a tiny bit more messy. Your example of the "messy" isn't even that messy to be fair, it's just one more .Add().

Herhangi commented 5 months ago

Sure thing, I've submitted the updated logic.