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

Porting code to new GUI system #719

Closed
evouga opened this issue Mar 10, 2018 · 15 comments
Closed

Porting code to new GUI system #719

evouga opened this issue Mar 10, 2018 · 15 comments

Comments

@evouga
Copy link
Contributor

evouga commented Mar 10, 2018

Guys,

I am trying to get old code to work with the new viewer API.

It looks like there is still a LIBIGL_WITH_NANOGUI but it doesn't look like it's hooked up to anything? I have old code like

    viewer.ngui->addGroup("Scene");
    viewer.ngui->addVariable("Filename", sceneFile_);
    viewer.ngui->addGroup("Simulation Options");
    viewer.ngui->addVariable("Timestep", params_.timeStep);
    viewer.ngui->addVariable("Newton Tolerance", params_.NewtonTolerance);
    viewer.ngui->addVariable("Newton Max Iters", params_.NewtonMaxIters);
    viewer.ngui->addGroup("Forces");
    viewer.ngui->addVariable("Gravity Enabled", params_.gravityEnabled);
    viewer.ngui->addVariable("  Gravity g", params_.gravityG);  

and I figured out the new Viewer namespace, but I cannot figure out what "ngui" was renamed to. Or is it gone completely? I looked at Example 106 and it seems like it does a lot of complicated low-level windows management that wasn't necessary before? :-/

@evouga
Copy link
Contributor Author

evouga commented Mar 10, 2018

By the way, the tutorial at http://libigl.github.io/libigl/tutorial/tutorial.html still contains old code snippets that no longer compile.

@jdumas
Copy link
Collaborator

jdumas commented Mar 10, 2018

Yes sorry we should have updated the documentation when we moved to ImGui. That's on my todo list.

The LIBIGL_WITH_NANOGUI doesn't do anything anymore, and should be removed, as well as the files opengl/glfw/TextRenderer* which are not necessary anymore.

ImGui is indeed a bit more low-level in terms of windows management than NanoGUI. Because it uses immediate-mode it cannot do a nice automatic layout yet (but I think it's being worked on). However, most of the time you have only 1 or 2 windows that are set up once so this is not a part of the code you would touch often.

Anyway, to use ImGui you need to enable the module in the CMake:

option(LIBIGL_WITH_OPENGL_GLFW_IMGUI "Use IMGUI" OFF)
find_package(LIBIGL REQUIRED QUIET)

...

target_link_libraries(${PROJECT_NAME}_bin igl::core igl::opengl_glfw_imgui)

Then register the plugin in the viewer:

#include <igl/opengl/glfw/imgui/ImGuiMenu.h>

...

// Attach a custom menu
igl::opengl::glfw::imgui::ImGuiMenu menu;
viewer.plugins.push_back(&menu);

Then to customize the default menu, you can do the following:

#include <igl/opengl/glfw/imgui/ImGuiHelpers.h>
#include <imgui/imgui.h>

...

// Customize default menu
menu.callback_draw_viewer_menu = [&]()
{
  menu.draw_viewer_menu(); // draw default menu, comment to overwrite
  if (ImGui::CollapsingHeader("Scene", ImGuiTreeNodeFlags_DefaultOpen))
  {
    static std::string sceneFile = "foo.json";
    ImGui::InputText("Filename", sceneFile);
  }
  if (ImGui::CollapsingHeader("Simulation Options", ImGuiTreeNodeFlags_DefaultOpen))
  {
    static float timeStep = 0.01;
    static float newtonTolerance = 1e-6;
    static int newtonMaxIters = 100;
    ImGui::InputFloat("Timestep", &timeStep, 0, 0, 3);
    ImGui::DragFloat("Newton Tolerance", &newtonTolerance, 0.01, 1e-16, 1e-1, "%.3e", 10);
    ImGui::InputInt("Newton Max Iters", &newtonMaxIters);
  }
  if (ImGui::CollapsingHeader("Forces", ImGuiTreeNodeFlags_DefaultOpen))
  {
    static bool gravityEnabled = true;
    static Eigen::Vector3f gravityG(0, 0, -1);
    ImGui::Checkbox("Gravity Enabled", &gravityEnabled);
    ImGui::InputFloat3("Gravity g", gravityG.data());
  }
};

2018-03-10-091510_1280x800_scrot

In this example I set the displayed precision of the timestep to 3 digits, and make the tolerance vary by powers of 10 if you drag the value with the mouse.

To get an idea of what is possible to do with ImGui, the recommended approach is to look at the demo windows (see a live example in the browser), and then copy from imgui_demo.cpp the code to reproduce the feature you have in mind.

Note that ImGui also made a few design choices, such as using char * instead of std::string, supports only float inputs (no double), and also no std::vector. It's pretty easy to wrap around those limitations, e.g. see <igl/opengl/glfw/imgui/ImGuiHelpers.h> where I've already added support for InputText(..., std::string &); as well as combo lists using lambdas/vectors.

In the example above, I can also throw in a piece of code to disable the "Gravity g" item on the fly depending on the status of the checkbox. Here is the complete example based on the libigl-example-project. The tutorial code features more advanced usage in terms of window management, which is why it needs to define a derived class, but I'm thinking maybe I could update the plugin a little bit to make it easier to use.

ImGui can also do a lot of cool stuff that are difficult to do with NanoGUI. E.g. you can update the layout and change the number of buttons on the fly at every frame, inject UI debugging code from deeply-nested functions withing your code, etc.

PS: I've submitted a PR #720 that does a little bit of cleanup and rename the lambdas overrides to callback_draw_* for consistency with Viewer.h. Let's see how it goes.
PS2: Ok PR merged, so you need to use callback_draw_* to override the menu callback now.

@evouga
Copy link
Contributor Author

evouga commented Mar 11, 2018

Thanks for the tips, I've set things up along the lines of your example and Example 106, but I'm getting a segfault at ImGuiMenu.cpp line 218 due to "viewer" being a bad pointer. Am I setting up the GUI wrong? Here is my main():

bool drawGUI(igl::opengl::glfw::imgui::ImGuiMenu &menu)
{
    menu.draw_viewer_menu();
    if (ImGui::CollapsingHeader("Simulation Control", ImGuiTreeNodeFlags_DefaultOpen))
    {
        if (ImGui::Button("Run/Pause Sim", ImVec2(-1, 0)))
        {
            toggleSimulation();
        }
        if (ImGui::Button("Reset Sim", ImVec2(-1, 0)))
        {
            resetSimulation();
        }
    }
    hook->drawGUI(menu);
    return false;
}

int main(int argc, char *argv[])
{
    igl::opengl::glfw::Viewer viewer;

    hook = new SevenHook();
    hook->reset();

    viewer.data().set_face_based(false);
    viewer.core.is_animating = true;

    viewer.callback_key_pressed = keyCallback;
    viewer.callback_pre_draw = drawCallback;
    viewer.callback_mouse_down = mouseCallback;
    viewer.callback_mouse_scroll = mouseScroll;

    igl::opengl::glfw::imgui::ImGuiMenu menu;
    viewer.plugins.push_back(&menu);

    menu.draw_viewer_menu_func = std::bind(drawGUI, menu);


    viewer.launch();
}

@evouga
Copy link
Contributor Author

evouga commented Mar 11, 2018

Never mind, found the issue: std::bind() was not passing the menu by reference.

@evouga
Copy link
Contributor Author

evouga commented Mar 11, 2018

It seems the float widget behaves strangely: it will not let me type in numbers in scientific notation (e.g. "1e-6") and does some kind of truncation? Is there a way around this?

@jdumas
Copy link
Collaborator

jdumas commented Mar 11, 2018

Looks like you are not the first person to ask, see this ticket. It should be possible to implement a InputDouble() using the InputText() function and doing the conversion yourself (see the instructions in the ticket above). Let me know if you need help with that, that's something we can add to ImGuiHelpers.h.

For the truncation you can change the number of displayed decimals (the 3 in ImGui::InputFloat("Timestep", &timeStep, 0, 0, 3);), or the displayed format %.3e with DragFloat()). If the truncation happens because you are using float instead of double, it may be fixed by implementing our own InputDouble() function.

@evouga
Copy link
Contributor Author

evouga commented Mar 12, 2018

Thanks again for the help! My code is now in a state "good enough for undergrads" so I didn't have time to try to incorporate this fix, though I definitely would find better floating-point widgets useful in the future.

As a final note, while I'm sure ImGUI has some nice features, and that there were good reasons to make the switch while dropping all support for NanoGUI, I urge y'all to very carefully consider the extent of downstream disruption before making this kind of sweeping API change again in the future. My code was a simple class project, but I know some of my students will soon have a nasty surprise when they try to recompile their research code...

At minimum, a prominent warning on the main libigl page that the API is about to change/has changed, a tagged release marking the transition from "libigl 2.0" to "libigl 3.0", with both versions being maintained for a while with bugfixes, etc. would have been nice...

Tagging also @alecjacobson and @danielepanozzo to hear my kvetching ;)

@evouga evouga closed this as completed Mar 12, 2018
@jdumas
Copy link
Collaborator

jdumas commented Mar 12, 2018

And that is why it is important to use submodules :p. But joke aside I completely agree with you. A new version tag was supposed to happen at some point, but I'm not sure what's the hold there.

Ideally we should have refactored the nanogui integration as a proper viewer plugin during the refactoring of the viewer (similar to the current imgui plugin right now). If you think this is important, I can try to look into it and maybe post a code snippet somewhere.

@panchagil
Copy link
Contributor

Following up on the scientific notation issue. I modified the code @jdumas pointed to, so it uses the imgui API. I copied the DataTypeFormatString function from imgui.cpp, there is probably a better solution for that part.

input_scalar_scientific.h

#include <imgui/imgui.h>
#include <imgui/imgui_internal.h>

bool InputFloatScientific( const char* label, float* v, const char *display_format = "%.3g", ImGuiInputTextFlags extra_flags = 0);
bool InputScalarScientific( const char* label, ImGuiDataType data_type, void* data_ptr, const char* scalar_format, ImGuiInputTextFlags extra_flags );

input_scalar_scientific.cpp

static inline void DataTypeFormatString(ImGuiDataType data_type, void* data_ptr, const char* display_format, char* buf, int buf_size)
{
    if (data_type == ImGuiDataType_Int)
        ImFormatString(buf, buf_size, display_format, *(int*)data_ptr);
    else if (data_type == ImGuiDataType_Float)
        ImFormatString(buf, buf_size, display_format, *(float*)data_ptr);
}

static bool DataTypeApplyOpFromTextScientific( const char* buf, const char* initial_value_buf, ImGuiDataType data_type, void* data_ptr, const char* scalar_format )
{
    scalar_format = "%f";
    float* v = (float*)data_ptr;
    const float old_v = *v;
    *v = (float)atof( buf );
    return *v != old_v;
}

bool InputScalarScientific( const char* label, ImGuiDataType data_type, void* data_ptr, const char* scalar_format, ImGuiInputTextFlags extra_flags )
{
    ImGuiWindow* window = ImGui::GetCurrentWindow();
    if ( window->SkipItems )
        return false;

    const ImVec2 label_size =  ImGui::CalcTextSize( label, NULL, true );

    ImGui::BeginGroup();
    ImGui::PushID( label );

    char buf[64];
    DataTypeFormatString( data_type, data_ptr, scalar_format, buf, IM_ARRAYSIZE( buf ) );

    bool value_changed = false;
    extra_flags |= ImGuiInputTextFlags_AutoSelectAll;
    if ( ImGui::InputText( "", buf, IM_ARRAYSIZE( buf ), extra_flags ) )
        value_changed = DataTypeApplyOpFromTextScientific( buf, GImGui->InputTextState.InitialText.begin(), data_type, data_ptr, scalar_format );

    ImGui::PopID();

    if ( label_size.x > 0 )
    {
        ImGui::SameLine( 0, ImGui::GetStyle().ItemInnerSpacing.x );
        ImGui::RenderText( ImVec2( window->DC.CursorPos.x, window->DC.CursorPos.y + ImGui::GetStyle().FramePadding.y ), label );
        ImGui::ItemSize( label_size, ImGui::GetStyle().FramePadding.y );
    }
    ImGui::EndGroup();

    return value_changed;
}

bool InputFloatScientific( const char* label, float* v, const char *display_format, ImGuiInputTextFlags extra_flags )
{
    return InputScalarScientific( label, ImGuiDataType_Float, (void*)v, display_format, extra_flags );
}

@patrikhuber
Copy link
Contributor

patrikhuber commented Apr 1, 2018

This bit me too. I've updated the libigl submodule in my app, and then step-by-step found out that not only the igl-viewer has been updated, but that the complete GUI system has been changed. It was quite hard to find information about it: The online tutorial still contains old code, and I was nowhere able to find release notes of libigl that would mention that massive incompatibility.

In the end, I now spent more than a day to completely re-write the GUI of my viewer-app. And the resulting code is spaghetti-code throughout. I must say that nanogui was much more pleasant to work with, with its more modern style, and callbacks and everything. ImGui feels like coding spaghetti C code.
And what I got as an end result in terms of application is not really better in any way. In fact, nanogui looked much more pleasant. (Yes I know I can apply styles to ImGui but I am no designer, I want a GUI that looks beautiful by default - like nanogui).

Anyway, it's too late for me now, but having nanogui as a plugin would've been great and would've spared me a complete rewrite (with worse code resulting).
And going forward, it would be really great if libigl had Release Notes, for example using GitHub Releases.

Anyway, thank you to all for the awesome work on libigl, it's a great library! :-)

@jdumas
Copy link
Collaborator

jdumas commented Apr 1, 2018

Hi,

The tutorial file has been updated to mention ImGui, but somehow the gh-pages branch that hosts the website hasn't been updated and I don't know why. We're also working on a new website for the documentation (here is a temporary url), I hope we can merge it soon. I agree with you that a release notes/changelog would be nice.

Whether NanoGui looks better/feels better to work with is really up to debate and a matter of personal preference. Personally I think it takes way too much space, and I prefer the more compact style of ImGui. For the rest, NanoGui may be nice to code if you use the FormHelper written by Christian Schüller, but this offers very limited possibilities. I found the low-level API of NanoGui to be much more difficult to use. On the other hand, the immediate mode of ImGui allows you to keep the GUI code localized, and I find it much more pleasant to use. I don't know why you are complaining about spaghetti code. And there is no limits to what you can do with ImGui, just look at their screenshot gallery.

Anyhow, there were reasons for this change (mostly dropping nested dependencies). There are pros and cons to both libraries (code a bit more verbose but more powerful, immediate mode paradigm but more limited automatic layout capabilities). If you want to write a ViewerPlugin to support NanoGui and help others doing the transition, that would certainly be helpful, and we can post a link somewhere.

@ocornut
Copy link

ocornut commented Apr 27, 2018

@panchagil FYI I have pushed some changes to imgui recently and InputFloat() have prototypes allowing a custom format string, which allows both scientific notation input and display.

In other words, if imgui gets updated, you should be able to use InputFloat() in place of your InputScalarScientific() function. I'd be interested in your feedback to confirm that it works as expected for you.

@jdumas
Copy link
Collaborator

jdumas commented Apr 27, 2018

Nice! It's on the todo list to update the imgui submodule at some point. I also need to fix some issues with the current vertex text labels (we may have too many labels on larger meshes), so it may take some time, but it will happen eventually =)

@ocornut
Copy link

ocornut commented Apr 28, 2018

@jdumas You may use a #define ImDrawIdx unsigned int in imconfig.h to change meshes to use 32-bit indices, if that's helpful.

@jdumas
Copy link
Collaborator

jdumas commented Apr 28, 2018

Thanks for the tip. I still need to detect when ImGui runs out of indices for the labels (seems to happen even before the number of vertices goes over 2^16). I also need to check what happens when we have multiple meshes in the viewer. I'll probably open an issue with more detail if I cannot figure it out.

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

No branches or pull requests

5 participants