UI: Dear ImGui by sp0lsh · Pull Request #158 · PolyEngineTeam/PolyEngine

sp0lsh · 2018-09-27T18:16:51Z

Added Dear ImGui by https://github.com/ocornut/imgui

Engine and Game systems can draw Imgui windows in Update Phase.

Check build status of prototype
Move Imgui to ThirdParties
Cleanup System/Component/Resource approach
Debug keyboard input in Imgui text Widgets

# Conflicts: # PolyEngine/Engine/Src/Engine.cpp

…ce and Game Project. Spams a lot

…included in shared libs

MuniuDev

I only addressed architectonical issues for now, as it's yet unfinished (at least according to the description)

MuniuDev · 2018-10-31T06:45:56Z

PolyEngine/Engine/Src/EnginePCH.hpp

 #include <assimp/scene.h>
 #include <assimp/Importer.hpp>
+
+// SILENCE_GCC_WARNING(-Wimplicit-fallthrough=, "Surpressing clang warnings in imstb_truetype")


We either need that or not, if not needed then remove commented code.

MuniuDev · 2018-10-31T06:50:15Z

PolyEngine/Engine/Src/Imgui/ImguiSystem.hpp

+
+	namespace ImguiSystem
+	{
+		void ImguiUpdatePhase(Scene* world);


Use class systems, function systems are deprecated.

MuniuDev · 2018-10-31T06:53:50Z

PolyEngine/RenderingDevice/OpenGL/Src/Proxy/imgui_impl_opengl3.h

@@ -0,0 +1,37 @@
+// ImGui Renderer for: OpenGL3 / OpenGL ES2 / OpenGL ES3 (modern OpenGL with shaders / programmatic pipeline)


This header and it's corresponding cpp look like part of imgui, we should make this another library and link it, not copy the files. This can be called ImGuiGLBinding or sth similar. Later, this would need to be linked by our rendering backend, depending on the rendering framework used.

I would keep it as example of custom rendering pass introduced by engine ImguiSystem

But this is not custom pass. This is third party code injected into our code tree. I think it would be much better to separate this. I know it's easier that way, but I would still try. You can think of it as something similar to epoxy, it provides gl bindings to our engine.

Need to compare OpenGL implementation with Vulkan.

MuniuDev · 2018-10-31T07:00:15Z

PolyEngine/Engine/Src/Imgui/ImguiResource.cpp

+
+using namespace Poly;
+
+ImguiResource::ImguiResource()


Why make it resource? I think this can safely be system field (use class systems) as it does not hold any state, at least relevant to game.

MuniuDev · 2018-10-31T07:04:09Z

PolyEngine/Engine/Src/Imgui/ImguiWorldComponent.hpp

+{
+	class ImguiResource;
+
+	class ENGINE_DLLEXPORT ImguiWorldComponent : public ComponentBase


There really is no need for the component. This can be safely put into system, as it does not hold state relevant to the game.

…stem constructor, removed Resource and WorldComponent

… consumed by any system

MuniuDev · 2018-11-04T12:10:19Z

PolyEngine/Engine/CMakeLists.txt

-find_package(Imgui REQUIRED)
 find_package(RapidJSON REQUIRED)
 find_package(STB REQUIRED)
+add_subdirectory("${ENGINE_ROOT_DIR}/ThirdParty/Imgui" "${CMAKE_CURRENT_BINARY_DIR}/Imgui")


Move this line to CMakeListsCommon.txt

MuniuDev · 2018-11-04T12:11:30Z

PolyEngine/Engine/CMakeLists.txt

 add_library(${ENGINE_TARGET} SHARED ${POLYENGINE_SRCS})
 target_compile_definitions(${ENGINE_TARGET} PRIVATE _ENGINE)
-target_include_directories(${ENGINE_TARGET} PUBLIC ${POLYENGINE_INCLUDE} ${BULLET_INCLUDE_DIRS} ${STB_INCLUDE_DIRS} ${IMGUI_INCLUDE_DIRS})
+target_include_directories(${ENGINE_TARGET} PUBLIC ${POLYENGINE_INCLUDE} ${BULLET_INCLUDE_DIRS} ${STB_INCLUDE_DIRS} "${ENGINE_ROOT_DIR}/ThirdParty/Imgui")


Let's make the location of the imgui headers a variable, set in CMakeListsCommon.txt, just near the line with add_subdirectory.

MuniuDev · 2018-11-04T12:12:55Z

PolyEngine/Engine/Src/Engine.hpp

+		/// <summary>Pushes input event to an input queue with specified UTF8 character.
+		/// One of eight functions handling incoming input events.</summary>
+		/// <param name="key">Unicode UTF8 character</param>
+		void AddCharacterUTF8(char* charUTF8) { InputEventsQueue.PushBack({eInputEventType::TEXTCHAR, charUTF8}); }


const char* ?

MuniuDev · 2018-11-04T12:13:56Z

PolyEngine/Engine/Src/Engine.hpp


+		/// <summary>Returns refference to output queue needed by Main.</summary>
+		/// <returns>Reference to OutputQueue instance.</returns>
+		OutputQueue& GetOutputQueue() { return OutputEventsQueue; }


Just out of curiosity, what do we output in that queue?

Commands to Window handling library (SDL2) in main.cpp such as switching between cursors on panel resizing.

MuniuDev · 2018-11-04T12:18:14Z

PolyEngine/Engine/Src/Imgui/ImguiSystem.cpp

+	if (inputCmp->GetCharUTF8() != nullptr)
+	{
+		const char* charUTF8 = inputCmp->GetCharUTF8();
+		size_t length = StrLen(charUTF8);


This is dangerous. The length should be handled by utf-8 aware method. We really need a UTF-8 String... But do we really need to check that length? I would just check if it's nullptr.

nullptr check was added later and should be sufficient, done

MuniuDev · 2018-11-04T12:24:59Z

PolyEngine/Engine/Src/Imgui/ImguiSystem.cpp

-	static bool show_demo_window = true;
-	static ImVec4 clear_color = ImVec4(0.45f, 0.55f, 0.60f, 1.00f);
+	if (io.WantCaptureMouse || io.WantCaptureKeyboard)
+		inputCmp->SetConsumed();


Why do we need to consume the input?

Bad thing:

Checkout fet/imgui branch

Uncomment lines 174. and 175.
3.1 See that ex. camera (FreeFloatMovement) is rotated when dragging mouse inside imgui panels.

2 See that ex. camera (FreeFloatMovement) is moved when typing inside imgui text widgets.

Good thing:
Consuming input in ImguiSystem handles also Imgui panels created by other systems including game systems.

MuniuDev · 2018-11-04T12:29:12Z

PolyEngine/Engine/Src/Input/InputSystem.cpp

 			break;
 		case eInputEventType::MOUSEPOS:
-			com->MouseDelta = ev.Pos - com->MousePos;
+			// com->MouseDelta = ev.Pos - com->MousePos;


Suddenly MOUSEPOS event i overwriting com->MouseDelta with (0,0) after MOUSEMOVE event and camera (FreeFloatMovement) does not rotate. Issue is also on dev. Tested in Standalone. Happens also on dev branch

Please checkout branches and confirm the issue.

Diffed with dev branch, seems that there are no changes in this branch for regression.

Indeed there is a problem, this is a result the way event are being sent. MOUSEMOVE and MOUSEPOS are always received in a pair (with that order). Let's add a comment describing that behaviour, and remove the commended line of code.

MuniuDev · 2018-11-04T12:29:45Z

PolyEngine/Engine/Src/Input/InputWorldComponent.hpp

 		Dynarray<Optional<size_t>> PlayerIDToJoystickID;
 		std::unordered_map<size_t, size_t> JoystickIDToPlayerID;
+		bool IsConsumed;
+		char* CharUTF8;


default values for both of those fields are needed.

Done, please check it

MuniuDev · 2018-11-04T12:31:02Z

PolyEngine/Engine/Src/Movement/MovementSystem.cpp

 	float deltaTime = (float)(TimeSystem::GetTimerDeltaTime(world, Poly::eEngineTimer::GAMEPLAY));
 	InputWorldComponent* inputCmp = world->GetWorldComponent<InputWorldComponent>();
+
+	if (inputCmp->GetIsConsumed())


This is very hacky

I know. Please see reported issues with camera rotation and mouse dragging on Imgui panels.

MuniuDev · 2018-11-04T12:32:29Z

PolyEngine/Standalone/Src/Main.cpp

 		Engine->Update();
+
+		Poly::OutputQueue& OutputEventsQueue = Engine->GetOutputQueue();
+		while (!OutputEventsQueue.IsEmpty())


We should add that logic for input handling to editor also.

Need to know where it is handled in Editor.

Ask @PiotrMoscicki

Engine::Update is called here - Editor/Src/Managers/Project/EngineManager.cpp
You can get SDL from - Editor/Src/Widgets/Inspectors/ViewportInspectorWidget.cpp
You can write getter for ViewportInsepctor InspectorManager at- Editor/Src/Managers/Project/InspectorManager.cpp
and then get ViewportInsepctor using gApp (gApp->InspectorMgr)

Added cursor handling from OutputEvent queue from engine. Couldn't open project in editor so it is only untested conversation starter.

MuniuDev · 2018-11-11T10:05:21Z

PolyEngine/Engine/Src/Input/InputWorldComponent.hpp


-		InputWorldComponent() = default;
+		InputWorldComponent()
+			: IsConsumed(false), CharUTF8(nullptr)


This should have been done in class header, not in the constructor. Please revert this.

* imgui reacting to input and drawing on screen from render device * proper init, render deinit and windows created by game * comments * comments * commented #pragma failing travis ci * removed #pragma * removed pragma from resource * commented unused components * commented out wrong variable doh... * merged with dev * cmake attempt * moved imgui to third parties, cmake updated for Engine, RenderingDevice and Game Project. Spams a lot * removed sanity spam * resource cleanup * imguisystem cleanup * imgui world component cleanup * added non windows API attributes to imconfig.h * moved imgui to pch * silencing third party gcc -Wimplicit-fallthrough= * enabled -fPIC (Position Independent Code) for imgui static lib to be included in shared libs * fixing gcc includes * fixing cgg includes * made includes same as in GLTextureDeviceProxy * removed commented out code * ImguiUpdate is now in System, Imgui context creation moved to ImguiSystem constructor, removed Resource and WorldComponent * moved Imgui to Engine subdirectory * Added clipboard support, tweaked mouse handling and input that can be consumed by any system * added UTF8 char input from keyboard keys * fixing input consumption (works on all widowns: engine and game) * added output queue and events handled by SDL * CI fix * CI fix and cleanup * CI fix * CI fix * fix * move add_subdirectory of imgui to CMakeListsCommon * added const to charUTF8 * removed length check when passing charUTF to Imgui * variable reuse * InputWorldComponent member initialization * dummy change * added comment about MouseDelta overwrite to (0,0) in MOUSEPOS * property init moved to header * handling mouse cursor feedback from engine imgui

sp0lsh added 5 commits September 25, 2018 22:19

imgui reacting to input and drawing on screen from render device

924fc01

proper init, render deinit and windows created by game

cceace4

comments

8d4f0e0

comments

f16f8d5

Merge branch 'dev' into fet/imgui

7f23fd5

# Conflicts: # PolyEngine/Engine/Src/Engine.cpp

sp0lsh mentioned this pull request Sep 27, 2018

UI: Dear ImGui KNTGPolygon/PolyEngineExamples#14

Open

sp0lsh added 21 commits September 28, 2018 22:32

commented #pragma failing travis ci

ef0e789

removed #pragma

8befcc8

removed pragma from resource

c5f2ab8

commented unused components

4ac9b58

commented out wrong variable doh...

c5cab92

Merge branch 'dev' into fet/imgui

3ee3414

merged with dev

b2eddea

cmake attempt

1210c94

moved imgui to third parties, cmake updated for Engine, RenderingDevi…

b22bd5a

…ce and Game Project. Spams a lot

removed sanity spam

003f1e7

resource cleanup

3d02fa9

imguisystem cleanup

c4ee972

imgui world component cleanup

f3aacaf

added non windows API attributes to imconfig.h

e1711fa

moved imgui to pch

d5001ab

silencing third party gcc -Wimplicit-fallthrough=

0f58768

enabled -fPIC (Position Independent Code) for imgui static lib to be …

2b5a565

…included in shared libs

fixing gcc includes

3113e77

fixing cgg includes

bc8c094

made includes same as in GLTextureDeviceProxy

78d1a90

removed commented out code

ab80981

MuniuDev reviewed Oct 31, 2018

View reviewed changes

MuniuDev requested changes Oct 31, 2018

View reviewed changes

ImguiUpdate is now in System, Imgui context creation moved to ImguiSy…

b9fb4b7

…stem constructor, removed Resource and WorldComponent

sp0lsh added 10 commits November 2, 2018 21:41

moved Imgui to Engine subdirectory

7bd9937

Added clipboard support, tweaked mouse handling and input that can be…

32e5d29

… consumed by any system

added UTF8 char input from keyboard keys

19cca52

fixing input consumption (works on all widowns: engine and game)

58e2f9a

added output queue and events handled by SDL

6984d1d

CI fix

f0c1cc5

CI fix and cleanup

354c6c4

CI fix

362d1fb

CI fix

470215d

fix

1572e31

MuniuDev requested changes Nov 4, 2018

View reviewed changes

sp0lsh added 6 commits November 6, 2018 20:06

move add_subdirectory of imgui to CMakeListsCommon

a2479ae

added const to charUTF8

16714a0

removed length check when passing charUTF to Imgui

920b277

variable reuse

cc7a439

InputWorldComponent member initialization

559288b

dummy change

9e16b6f

MuniuDev requested changes Nov 11, 2018

View reviewed changes

sp0lsh mentioned this pull request Nov 15, 2018

Rendering: Shadows pt 2 #155

Merged

13 tasks

sp0lsh added 3 commits November 16, 2018 13:51

added comment about MouseDelta overwrite to (0,0) in MOUSEPOS

40a0376

property init moved to header

6ad39c6

handling mouse cursor feedback from engine imgui

95b5987

MuniuDev approved these changes Nov 18, 2018

View reviewed changes

MuniuDev merged commit 46430d5 into PolyEngineTeam:dev Nov 18, 2018

		@@ -0,0 +1,37 @@
		// ImGui Renderer for: OpenGL3 / OpenGL ES2 / OpenGL ES3 (modern OpenGL with shaders / programmatic pipeline)

Conversation

sp0lsh commented Sep 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MuniuDev left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sp0lsh Nov 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sp0lsh Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sp0lsh Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sp0lsh Nov 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

sp0lsh commented Sep 27, 2018 •

edited

Loading

sp0lsh Nov 2, 2018 •

edited

Loading

sp0lsh Nov 6, 2018 •

edited

Loading

sp0lsh Nov 6, 2018 •

edited

Loading

sp0lsh Nov 6, 2018 •

edited

Loading