vsg-dev / vsgImGui

Integration of VulkanSceneGraph with ImGui
MIT License
43 stars 28 forks source link

Let the user create the ImGUI context to control the settings #61

Closed MauFranco closed 9 months ago

MauFranco commented 9 months ago

Let the user create the ImGUI context to control the settings before the renderer is initialized.

robertosfield commented 9 months ago

This change will break existing applications and require them to change their application code to keep things working, worse they will likely still compile fine but they will only see problems once they run their applications.

The modifications to vsgExamples only calls ImGui::CreateContext() but doesn't call ImPlot::CreateContext() so may also fail once users enable the ImPlot code paths.

--

The change was done for a reason so we'll need to come up with a scheme that addresses the usage case that you want to follow without breaking existing applications.

Could you explain what settings you are wanting to set and why this preferable to do before creation of the RenderImGui object?

I don't have a solution in mind, but perhaps if ImGui and ImPlot have methods for checking whether CreateContext() has already been called so that the vsgImGui code could check this and only call CreateContext() if it hasn't already been called.

MauFranco commented 9 months ago

That's correct. It will break existing applications. I agree ImPlot::CreateContext() would need to be added to the example.

The setting I want to set is:

auto &io = ImGui::GetIO();
io.IniFilename = nullptr;

so that the ini file is not created in a system read-only location.

robertosfield commented 9 months ago

My preferred behaviour will be for existing applications to continue to work without changes, but for us to give the option for usage case you want to follow, such a scheme wouldn't require adding the ImGui::CreateContext() and ImPlot:::CreateContext() calls to the example. Ideally if we could just call CreateContext() when required so that there needn't be any public API required to toggle this behaviour.

I am away visiting family this week so on occasionally getting an opportunity to catch up with support. If I don't get a chance this week to look into doing the CreateContext on demand I'll do it next Tuesday when I'm back on the office.

MauFranco commented 9 months ago

Agree. Creating the context as needed in RenderImGui seems to be the best way forward. Added new commit.

auto imGuiContext = ImGui::GetCurrentContext();
if (imGuiContext == nullptr)
{
    ImGui::CreateContext();
}

auto imPlotContext = ImPlot::GetCurrentContext();
if (imPlotContext == nullptr)
{
    ImPlot::CreateContext();
}
MauFranco commented 9 months ago

thanks!

robertosfield commented 9 months ago

After merging I tweaked the code yo be a bit more streamlined and more straightforward to read the intent: 5ff77032342e854d00358d3b2a3314e947177a55

MauFranco commented 9 months ago

Sounds good for this case.

We usually like to put statements in different lines to make putting breakpoints, stepping into and variable inspection easier when debugging.

Thanks!

From: Robert Osfield @.> Sent: Sunday, December 31, 2023 3:41 AM To: vsg-dev/vsgImGui @.> Cc: Franco, Mauricio @.>; Author @.> Subject: Re: [vsg-dev/vsgImGui] Let the user create the ImGUI context to control the settings (PR #61)

CAUTION: This email originated from outside of the organization.

After merging I tweaked the code yo be a bit more streamlined and more straightforward to read the intent: 5ff7703https://github.com/vsg-dev/vsgImGui/commit/5ff77032342e854d00358d3b2a3314e947177a55

— Reply to this email directly, view it on GitHubhttps://github.com/vsg-dev/vsgImGui/pull/61#issuecomment-1872928029, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABTWKN65AP6AFRKCGLCRNDLYMFFNHAVCNFSM6AAAAABBGDEUDOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQNZSHEZDQMBSHE. You are receiving this because you authored the thread.Message ID: @.**@.>>