Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Various Fixes #8

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Conversation

JordanSchlick
Copy link

@JordanSchlick JordanSchlick commented May 18, 2023

Added imgui std::string lib
Fixed resetting input on release
Fixed Linux compilation errors
Updated for unreal 5.2
Improved canvas viewport sizing

This code has been tested in OpenStorm

{
// ImGui::InputText() with std::string
// Because text input needs dynamic resizing, we need to setup a callback to grow the capacity
IMGUI_API bool InputText(const char* label, std::string* str, ImGuiInputTextFlags flags = 0, ImGuiInputTextCallback callback = NULL, void* user_data = NULL);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to be honest and say that I wouldn't want to merge this in our fork. Taking a std::string by pointer is prone to error, it should be a reference for safety's sake. Moreover, user data holding a pointer to the std::string is also pretty suspect, and it would make more sense for it to hold a reference as well.

Though in all honesty, I would prefer this use FString and forgo the std library entirely.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did get that code directly from the main imgui repo so I suspect it has had a decent amount of review. I am using it my application as an alternative to char* strings without issue.

Copy link

@SK83RJOSH SK83RJOSH Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did get that code directly from the main imgui repo so I suspect it has had a decent amount of review. I am using it my application as an alternative to char* strings without issue.

Certainly, but the issue isn't that the code doesn't work, it's just super easily abused. Plus I feel it makes a lot more sense to reuse unreal types where possible, since they'll be far more common in game code. To each their own though – and admittedly I understand not rewriting sample code for this purpose. :)

@WiggleWizard
Copy link
Owner

Will pull this into our repo and test these changes on Monday.

@SK83RJOSH
Copy link

SK83RJOSH commented Jun 1, 2023

If you do, I would suggest @JordanSchlick update the strncpy fix to use FCStringAnsi::Strncpy to ensure maximum portability (and that you try to use those utilities where possible ^^).

I also found the ResetInput stuff to be a bit unnecessary, and a more concise fix would be updating SImGuiWidget::UpdateInputState to call the following code when returning focus (which is more inline with what the rest of the code is doing there - I think the fact it didn't do this already was an oversight):

InputHandler->OnKeyboardInputDisabled();
InputHandler->OnGamepadInputDisabled();
InputHandler->OnMouseInputDisabled();

Otherwise the rest of this stuff is great, particularly the viewport fixes, though I didn't get a chance to test the other two settings for CanvasSizeType - but I think the changes that were reverted via this PR were the main culprit with issues there anyways. So I think it's more correct in general now. 🙂

Also I apologize for dropping in out of nowhere on this issue; I was trying to find a fork that's currently being maintained to upstream to and came across this one. So I wanted to drop in and leave a quick code review on this PR since this is the fork we'll end up using, and these changes include a number of nice fixes.

@WiggleWizard
Copy link
Owner

Thanks for the info. I've been meaning to update this with our studio's changes but we've been really busy...hopefully next week I will have enough down time on the programming side that I can actually update this repo with the much needed pushes.

@JordanSchlick
Copy link
Author

I implemented and tested the suggested changes involving InputHandler and Strncpy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants