Refactor the ModelViewer to use PiGui #4849
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Now LuaPiGui handles it, decoupling the PiGui implementation from lua - Made all externally-facing symbols in LuaPiGui part of the PiGUI namespace This will be renamed to PiGui and the class of the same name will become part of the namespace.
At the moment, the lifecycle is responsible for calling HandleEvents and rendering PiGui. This can change once legacy code no longer depends on HandleEvents running after View::Draw3D(). Added virtual HandleEvent() call to dispatch to legacy UI code. This is suboptimal; we need a better way that doesn't involve virtual function calls. Use std::function or a better, lower-overhead delegate library
Make PiGui a namespace for easier method scoping, allows us to be more flexible PiGui::Instance only has the bare minimum of methods needed to run ImGui Move PiGui RegisterClass call to pigui/PiGuiLua - we'll further unify LuaPiGui and PiGuiLua later.
Explicitly inject the renderer dependency, instead of relying on Pi::renderer existing Made it Pi.cpp's responsibility to hide the mouse cursor.
Added IsKeyPressed / Released methods to track per-frame key state Added GetMouseWheel so users don't have to track mouse wheel events themselves Input keeps a pointer to a config instance, completely decoupling it from Pi
"Snap to Direction" keybinds don't work, due to Input not dispatching their event handlers. Will be resolved in a later sprint on the Input system.
|
Did we find out what's happened to travis? |
|
Nope, still not sure what's going on there. The config file works fine, travis is building correctly, but it's not showing up on the repo. I think you'll have to dig around in the Travis settings to figure out what's going on. |
fluffyfreak
approved these changes
Apr 4, 2020
|
Merging this in, as I think we've caught all of the related bugs! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I've re-implemented the ModelViewer in PiGui, using the GuiApplication class and the Input system to simplify a fair amount of the handling.
To accomplish this, I've done a fair amount of refactoring:
PiGuiis nowPiGui::Instance, and all of the static drawing methods have been moved out of the class, opening up the possibility to add more C++-side widgets for both Lua and C++ to use.PiGui::Instanceis now only responsible for handling ImGui - all Lua-side initialization is handled byPiGUI::Lua::Init()instead.PiGui::Instancehas its renderer dependency injected by its owner rather than relying onPi::renderer. This actually reduced complexity a bit, because we no longer had to manually specify the SDL window every time we started an ImGui frame.Inputhad its last ties toPiremoved, with the Config dependency injected by the owner, similarly to PiGui.KeyBindings::AxisBindingandActionBindingstill depend on thePi::inputsingleton variable, so there's an ugly hack in the ModelViewer to work around that. I'll address that in a later PR that refactors bindings (once again!).GuiApplicationnow owns an Input instance and a PiGui instance, decoupling management of those two classes away fromPi.cpp. It's not as automatic as I'd like, due to oldUI's issues with label drawing, but it's 90% of the way there.Inputnow has methodsIsKeyPressed()andIsKeyReleased()to check whether a specific key was pressed or released during the current frame. This does come with some more runtime complexity, but the overall effect should be mostly negligible.Overall, this is a 1:1 replacement of the previous ModelViewer, with only one regression: the "snap view to direction" keybindings are not currently working because they're not being fed SDL_Events to trigger the
onPresscallbacks. This will also be resolved in a future Input PR, but is outside of the scope of this work.