Skip to content

move ImGuizmo tutorial into plugin; simplify and isolate#1618

Merged
alecjacobson merged 1 commit intomasterfrom
ImGuizmo-simplify
Sep 27, 2020
Merged

move ImGuizmo tutorial into plugin; simplify and isolate#1618
alecjacobson merged 1 commit intomasterfrom
ImGuizmo-simplify

Conversation

@alecjacobson
Copy link
Copy Markdown
Contributor

Much of the ImGuizmo functionality was in the tutorial code. This moves it into a minimal reusable Viewer plugin. Along the way it fixes some important bugs with how the imguizmo displayed on models that don't fit in the unit cube.

The tutorial is updated to be much more minimal and in the style of the other tutorials.

cow-imguizmo

Check all that apply (change to [x])

  • All changes meet libigl style-guidelines.
  • Adds new .cpp file.
  • Adds corresponding unit test.
  • This is a minor change.

@jdumas jdumas added the viewer label Sep 26, 2020
@alecjacobson
Copy link
Copy Markdown
Contributor Author

This achieves the desired behavior, but extending ImGuiMenu creates a bit of a mess. I previously had the impression that Viewer::plugins list could contain multiple ImGui menus/plugins. That appears to be impossible because of how the plugin calls pre_draw/post_draw: you can only have one ImGuiMenu/ImGui-based plugin. I'm wondering if anyone is actually using more than one plugin...

I noticed this when making an app that tried to use two of these ImGuizmoPlugins. ImGui crashes when on the pre_draw of the second plugin:

-> 6733 IM_ASSERT((g.FrameCount == 0 || g.FrameCountEnded == g.FrameCount) && "Forgot to call Render() or EndFrame() at the end of the previous frame?");

Having the caller fill out a class (like the old demo) shifts this problem to the user who could solve it by putting 2+ guizmos in the post_draw. But this is really cumbersome.

I like what this PR achieves. The caller simply pushes the plugin to Viewer::plugins and attaches a callback.

ImGuizmo doesn't appear support multiple guizmos so the issue above is moot (still think the plugin predraw/postdraw could be an issue, but I'll just leave it crossed out here for reference in case I forget this and find it later).

As far as ImGuizmos, if the caller wants to use the guizmo for different things then they need to handle the logic for that.

Copy link
Copy Markdown
Collaborator

@jdumas jdumas left a comment

Choose a reason for hiding this comment

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

Lgtm, thanks Alec!

@alecjacobson alecjacobson merged commit 9921615 into master Sep 27, 2020
@jdumas jdumas deleted the ImGuizmo-simplify branch November 18, 2020 18:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants