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

[Scene2d.ui] Adding support for horizontal scrolling (touch pad and touch screen) #6154

Merged
merged 2 commits into from Oct 13, 2020

Conversation

jpauty
Copy link
Contributor

@jpauty jpauty commented Aug 31, 2020

Hi,

This is a new PR, with code updated w.r.t. master branch. Closed PR

I prepared a new PR to fix horizontal scrolling support and fractional scrolling.

Currently horizontal scrolling with a touch pad is not supported. More over, devices which report fractional scrolling values, such as touch pads and mouses with continuous wheels are not supported. The scrolling is super fast, because small scroll increments (0.1) are converted to 1 or -1. See my previous PR for more details.

Cheers,

Julien

Also, adding support for devices which report fractional scrolling
amounts. E.g. a mouse with a continuous mouse wheel, or a Mac touch
pad. This is currently only supported in lwjgl3. It can probably be
added to android and maybe to gwt.

InputProcessor scrolled method now receives scroll amount for X and
Y. Changed type to float to support devices which report fractional
scroll amounts. Updated InputEvent in scene2d accordingly: added
scrollAmountX, scrollAmountY attributes and corresponding setters and
getters.

Input event queue can receive float scrolling values. Since the queue
stores the events with integers. Scrolling values are stored in fixed
precision format, with 8 bits for the fraction.

Updated backends accordingly: lwjgl, lwjgl3, android and gwt.

@Darkyenus
Copy link
Contributor

Instead of /256 & *256, you could use floatToIntBits and inverse. (Use wrappers from NumberUtils). It won't lose precision and the performance will be probably similar.

@NathanSweet
Copy link
Member

Agreed, floatToIntBits is a bit better since it preserves the entire float. Otherwise it looks good to me.

@MrStahlfelge
Copy link
Member

Please correct the tests, they don't compile any more.

@tommyettinger
Copy link
Member

I have a problem with this. Changing InputListener and InputAdapter by removing the old scrolled(float) and adding the new scrolled(float, float) breaks almost all existing libGDX code that implemented the old scrolled(float) method, including all code that used InputListener directly. There's no compatibility layer here, which explains why the tests don't run. InputAdapter at least has an easy workaround that could be employed; scrolled(float) would stay in InputAdapter (not necessarily InputListener), and the default implementation of scrolled(float, float) could call scrolled(float), ignoring horizontal scroll unless the game code can handle horizontal scroll (in which case scrolled(float, float) could be overridden). This allows existing code that uses InputAdapter to at least compile when users upgrade, instead of all users getting the same compile errors the day 1.10 is released. If we had Java 8 default interfaces, maybe we could make the workaround apply to InputListener too.

@MrStahlfelge
Copy link
Member

@jpauty let's add a new InputAdapter method and fix the compile errors to get this forward.

@jpauty
Copy link
Contributor Author

jpauty commented Sep 1, 2020

I've added the scrolled method for backward compatibility. I've also replaced my retro fixed point conversion to calls to NumberUtils.

The travis checks passes. Sorry for the compile errors. I'm only using mvn to check compilation, which apparently skips the tests. I tried briefly with gradle, but it requires android, which I don't have time to setup right now.

@payne911
Copy link
Contributor

payne911 commented Sep 1, 2020

using mvn to check compilation, which apparently skips the tests

Which command are you using?

@jpauty
Copy link
Contributor Author

jpauty commented Sep 1, 2020

using mvn to check compilation, which apparently skips the tests

Which command are you using?

mvn install

@payne911
Copy link
Contributor

payne911 commented Sep 1, 2020

mvn install

That should not skip tests: weird. mvn install -DskipTests would skip tests.

Maybe it's because the libGDX project structure isn't the same as the default Maven one, and thus the tests aren't hooked up properly to the maven command?

Also, it's recommended to clean too: mvn clean install.

@SimonIT
Copy link
Member

SimonIT commented Sep 1, 2020

The tests which failed were not automated tests. It's the test project which is not included inside the maven environment because they don't have to get deployed

@MobiDevelop
Copy link
Member

Every implementor of InputProcessor is going to be 'broken' no matter what - whether we keep the old scrolled method or not, right? After all, they will need to implement the new method. I'm not sure about the change in InputAdapter calling into the old scrolled method by default - this feels like asking for trouble.

If we're going to break it, we might as well just break it once and leave it in the state we want. Leaving a method that we don't really want and which only gets called via a compatibility layer (InputAdapter) isn't really doing anyone any favors. When we eventually do remove it, we'd just break everyone again.

@jpauty
Copy link
Contributor Author

jpauty commented Sep 2, 2020

My initial idea of not adding backward compatibility was that it was easy to fix your code: add the missing parameter, change the types and you're done.

I can remove the call to old method in InputAdapter.

@jpauty
Copy link
Contributor Author

jpauty commented Sep 3, 2020

Following the "thumbs up votes" I removed the compatibility call.

@MrStahlfelge
Copy link
Member

Thank you. Scrolling over your code, I only think it is sad not to support the new feature on GWT. Apart from that, it looked good to me, but I did not test yet. As it is a breaking change, we still have to decide when to pull it in.

Copy link
Member

@MrStahlfelge MrStahlfelge left a comment

Choose a reason for hiding this comment

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

Tested on Lwjgl3 and ScrollbarScrollbarsTest. Works good. Scrollbar scrolls more fluently than before, and horizontal scrolling works great. I'm in high favor to merge as soon as we can merge in breaking changes. @jpauty just explain while you roll back your first code change from the other PR.

@crykn
Copy link
Member

crykn commented Sep 8, 2020

I just tested your changes on Mac OS with LWJGL3 and the horizontal scrolling stuff is working like a charm. I didn't notice any reduction in scrolling speed due to your fractional scrolling fixes, but I also wasn't sure how I could reliably test that.

Also, adding support for devices which report fractional scrolling
amounts. E.g. a mouse with a continuous mouse wheel, or a Mac touch
pad. This is currently only supported in lwjgl3. It can probably be
added to android and maybe to gwt.

InputProcessor scrolled method now receives scroll amount for X and
Y. Changed type to float to support devices which report fractional
scroll amounts. Keeping the original scroll method of InputProcessor
for backward compatibility.  Updated InputEvent in scene2d
accordingly: added scrollAmountX, scrollAmountY attributes and
corresponding setters and getters.

Input event queue can receive float scrolling values.

Updated backends accordingly: lwjgl, lwjgl3, android and gwt.
@jpauty
Copy link
Contributor Author

jpauty commented Sep 12, 2020

I've fixed formating.

@crykn crykn added this to the Next Major Release milestone Oct 8, 2020
@MrStahlfelge MrStahlfelge merged commit 432df1b into libgdx:master Oct 13, 2020
@obigu
Copy link
Contributor

obigu commented Oct 28, 2020

Upgrade Notes: For apps that upgrade to libGDX 1.10.0 and were using previous scrolled (float amount), on non-Lwjgl3 backends the amount is now reported on amountY argument.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Label for breaking changes core affecting all platforms enhancement input Concerning Gdx.input scene2d
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet