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

Diagonal arrow fix + bump GLFW, ImGui #252

Closed
wants to merge 4 commits into from

Conversation

litelawliet
Copy link
Collaborator

@litelawliet litelawliet commented Jul 6, 2023

Fix #121

Bump GLFW (dll-release) to 3fa2360720eeba1964df3c0ecf4b5df8648a8e52.
Bump ImGui (docking) to 8566fec661801a026e56f06cd53f5dac25c2595b.

Fix .gitignore tracking .vcproj (no x), now ignored.

Fix diagonal arrows not showing up by bumping GLFW and ImGui.

Changes were made due to ImGui internal changes:

  • ForceHover was removed from ImGui::PlotLines(...) and ImGui::PlotHistogram(...).
  • Flags were removed from buttons (to disable here), replaced by ImGui::BeginDisabled() and ImGui::EndDisabled.
  • SetNextTreeNodeOpen() was replaced by SetNextItemOpen().
  • ImGuiCol_ModalWindowDarkening was replaced by ImGuiCol_ModalWindowDimBg.

New bug introduced:
The first right click (for looking in the editor view) will keep updating the camera even after releasing the mouse button. Doing it a second time fix it.

Bump GLFW (dll-release) to 3fa2360720eeba1964df3c0ecf4b5df8648a8e52.
Bump ImGui (docking) to 8566fec661801a026e56f06cd53f5dac25c2595b.

Fix diagonal arrows not showing up by bumping GLFW and ImGui.

Changes were made due to ImGui internal changes:
- ForceHover was removed from `ImGui::PlotLines(...)` and `ImGui::PlotHistogram(...)`.
- Flags were removed from buttons (to disable here), replaced by `ImGui::BeginDisabled()` and `ImGui::EndDisabled`.
- `SetNextTreeNodeOpen()` was replaced by `SetNextItemOpen()`.
- `ImGuiCol_ModalWindowDarkening` was replaced by `ImGuiCol_ModalWindowDimBg`.

New bug introduced. The first right click (for looking in the editor view) will keep updating the camera even after releasing the mouse button. Doing it a second time fix it.
@litelawliet litelawliet self-assigned this Jul 6, 2023
@litelawliet litelawliet added Bug Something isn't working Editor Something relative with the editor Breaking Change Something that will break retro-compatibility or can affect other parts of the code base labels Jul 6, 2023
@litelawliet litelawliet added this to the 1.4 milestone Jul 6, 2023
Copy link
Owner

@adriengivry adriengivry left a comment

Choose a reason for hiding this comment

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

After some testing it looks like I can't right click anymore, the mouse now disappears while right clicking in any panel (Hierarchy, Inspector...), just like when right clicking in the scene view.

Dependencies/glfw/lib/glfw3dll.exp Outdated Show resolved Hide resolved
Copy link
Collaborator

@maxbrundev maxbrundev left a comment

Choose a reason for hiding this comment

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

I have been able to fix the new issue where the camera updating even after releasing the mouse button by calling the method UpdateMouseState(); outside and before the m_view.IsHovered() statement. However this is not an acceptable fix we certainly miss something with the ImGui change from the update.

@litelawliet
Copy link
Collaborator Author

I think the right mouse cursor is disapearing because of :

if (m_inputManager.IsMouseButtonPressed(OvWindowing::Inputs::EMouseButton::MOUSE_BUTTON_RIGHT))
{
m_rightMousePressed = true;
m_window.SetCursorMode(OvWindowing::Cursor::ECursorMode::DISABLED);
}
if (m_inputManager.IsMouseButtonReleased(OvWindowing::Inputs::EMouseButton::MOUSE_BUTTON_RIGHT))
{
m_rightMousePressed = false;
m_firstMouse = true;
m_window.SetCursorMode(OvWindowing::Cursor::ECursorMode::NORMAL);
}

I think we might have to change a few things in our current logic ?
If not, then the issue might come from :

bool DisableMouseUpdate; // false // Prevent the mouse from updating (Overload modification)

which might create problems with the new ImGui version due to :
ImGuiIO& io = ImGui::GetIO();
if (!io.DisableMouseUpdate && button >= 0 && button < ImGuiMouseButton_COUNT) // Overload modification
io.AddMouseButtonEvent(button, action == GLFW_PRESS);
}

and
https://github.com/adriengivry/Overload/blob/7aca41d80a1a26c35e3694d39a25d066b6f7eadc/Sources/Overload/OvUI/src/OvUI/ImGui/imgui.cpp#L4835-L4842

@adriengivry
Copy link
Owner

Just noticed that this PR also breaks the mouse wheel zoom

@adriengivry
Copy link
Owner

This PR seems to be really problematic and introducing several bugs (broken right click, hovering issue, broken zoom, and potentially more).
To improve the stability of our release, I would suggest closing this PR for now and starting over with:

  1. PR for GLFW update
  2. PR for ImGUI update
    @litelawliet Would that be possible?
    I was also wondering which update was necessary to get these diagonal arrow cursors, is updating GLFW alone sufficient? Or is updating ImGUI alone sufficient?

@litelawliet
Copy link
Collaborator Author

I don't think closing the PR will do any good, at best it shows the PR needs rework to make everything working - meaning the current code of Overload regarding input is too simple to handle more complex and contextualized behaviors alongside the internal changes of ImGui.
GLFW doesn't do anything wrong but provide more recent features and stability, so all good on that.
I tested GLFW before pulling a more recent version of ImGui to confirm ImGui was the issue.

I've been doing some changes in local, I got way more promising results which works almost as good as before (really tiny bugs due to how the logic works currently, but overall almost as good as before) but I don't have a lot of spare time to work on that PR to finish it quickly.

So I think it will be ready, but give it time and tries because it will need a recent version of ImGui to fix the original issue and thus changes in the logic :)
Plus, I think it's a good thing to keep ImGui up-to-date from time to time (not so often of course) to avoid bigger breaking changes when we'll want to update ImGui for new UI stuff (new features, both from Overload and ImGui ?) or to integrate fixes, like currently.

You need to know ImGui has done a complete rework of the internal input logic and behavior.
Imo, this PR shows ImGui do things differently now contrary to before and also shows the editor needs to be more robusts regarding input context which I think is not really a known concept currently (the logic seems to overlap a bit over all potential panel).

@adriengivry
Copy link
Owner

I think there's a simpler approach that can benefit both you and the reviewers. It's important to prioritize stability and prevent regressions, like the one introduced here. A good guideline would be to break down major changes, which will make bug hunting easier in a smaller context.

However, I'm also fine if you prefer to take on that responsibility. I'll be happy to thoroughly review your work once you've addressed these regressions.

@adriengivry adriengivry mentioned this pull request Oct 26, 2023
@maxbrundev
Copy link
Collaborator

Closing this pull request, the GLFW and imGUI Bumps are going to be split and the GLFW pull request is existing #261

@maxbrundev maxbrundev closed this Nov 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking Change Something that will break retro-compatibility or can affect other parts of the code base Bug Something isn't working Editor Something relative with the editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No diagonal window resize cursor
3 participants