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

Fix scrolling in scroll containers #14702

Merged
merged 2 commits into from
Jun 3, 2024
Merged

Conversation

appgurueu
Copy link
Contributor

@appgurueu appgurueu commented May 25, 2024

Should fix #14517; since this is a highly platform and compiler-specific issue, I am hesitant to claim that it always works, so make sure to test this thoroughly.

It seems ButtonStates was not initialized for scroll events, but accessed through isLeftPressed. With optimizations enabled this probably yielded nonzero values, which resulted in a mess: The scroll container sometimes falsely believed that the button was held down.

How to test

Compile a release build with GCC on Linux. Verify that scrolling is choppy. Verify that with this PR applied, it isn't anymore. The following diff might help for testing:

diff --git a/irr/src/CIrrDeviceSDL.cpp b/irr/src/CIrrDeviceSDL.cpp
index 1a7fe0c5a..6437e8107 100644
--- a/irr/src/CIrrDeviceSDL.cpp
+++ b/irr/src/CIrrDeviceSDL.cpp
@@ -19,6 +19,7 @@
 #include <cstdlib>
 #include "SIrrCreationParameters.h"
 #include <SDL_video.h>
+#include <iostream>
 
 #ifdef _IRR_EMSCRIPTEN_PLATFORM_
 #include <emscripten.h>
@@ -666,6 +667,8 @@ bool CIrrDeviceSDL::run()
                case SDL_MOUSEWHEEL: {
                        SDL_Keymod keymod = SDL_GetModState();
 
+                       std::cout << "sdl says we scrollin" << std::endl;
+
                        irrevent.EventType = irr::EET_MOUSE_INPUT_EVENT;
                        irrevent.MouseInput.Event = irr::EMIE_MOUSE_WHEEL;
 #if SDL_VERSION_ATLEAST(2, 0, 18)
diff --git a/src/gui/guiScrollContainer.cpp b/src/gui/guiScrollContainer.cpp
index 2d71f3453..7015ed9c3 100644
--- a/src/gui/guiScrollContainer.cpp
+++ b/src/gui/guiScrollContainer.cpp
@@ -18,6 +18,7 @@ with this program; if not, write to the Free Software Foundation, Inc.,
 */

 #include "guiScrollContainer.h"
+#include <iostream>
 
 GUIScrollContainer::GUIScrollContainer(gui::IGUIEnvironment *env,
                gui::IGUIElement *parent, s32 id, const core::rect<s32> &rectangle,
@@ -41,6 +42,8 @@ bool GUIScrollContainer::OnEvent(const SEvent &event)
                Environment->setFocus(m_scrollbar);
                bool retval = m_scrollbar->OnEvent(event);
 
+               std::cout << "scrollin" << std::endl;
+
                // a hacky fix for updating the hovering and co.
                IGUIElement *hovered_elem = getElementFromPoint(core::position2d<s32>(
                                event.MouseInput.X, event.MouseInput.Y));

Without the fix, not all SDL events "come through", even though you're not pressing any mouse buttons: You see "SDL says we scrollin" without "scrollin". After the fix, each "SDL says we scrollin" triggered inside the scroll container, without the mouse button being pressed, should be followed by a "scrollin".

@appgurueu appgurueu changed the title Properly initialize ButtonStates for EMIE_MOUSE_WHEEL Fix scrolling in scroll containers May 25, 2024
irr/include/IEventReceiver.h Outdated Show resolved Hide resolved
@grorp
Copy link
Member

grorp commented May 25, 2024

You ninja'd me very hard, I just wanted to create a PR with the same patch (I was working on the same code to implement highdpi support, then noticed the uninitialized field). So yes, I can confirm that this works for me.

Copy link
Member

@Desour Desour left a comment

Choose a reason for hiding this comment

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

Tested, works. 👍 Thanks! <3

(But fix the comment, see sfan5's comment.)

@appgurueu
Copy link
Contributor Author

Bump, is it good to go now?

(Asking since I ended up "fixing the comment" by always initializing the variable as appropriate, and I want to make sure I didn't miss something there.)

@sfan5 sfan5 merged commit ec9c000 into luanti-org:master Jun 3, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Scrolling with mouse wheel in main menu settings is broken
5 participants