We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

Bug 1687 - SDL-Android project: Classification of touch events
Summary: SDL-Android project: Classification of touch events
Status: RESOLVED WONTFIX
Alias: None
Product: SDL
Classification: Unclassified
Component: *don't know* (show other bugs)
Version: HG 2.0
Hardware: All Android (All)
: P2 normal
Assignee: Gabriel Jacobo
QA Contact: Sam Lantinga
URL:
Keywords: target-2.0.0
Depends on:
Blocks:
 
Reported: 2013-01-12 08:17 UTC by wboe
Modified: 2013-07-27 09:15 UTC (History)
2 users (show)

See Also:


Attachments
Patch file for SDL-for-Android project (3.35 KB, patch)
2013-01-12 08:17 UTC, wboe
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description wboe 2013-01-12 08:17:15 UTC
Created attachment 1012 [details]
Patch file for SDL-for-Android project

Hi,
IMHO in SDLActivity.java there is a glitch in the classification of touch events. 
After line 624:
    if (action == MotionEvent.ACTION_MOVE && pointerCount > 1) {
in case pointerCount == 1, the statement
     pointerFingerId = event.getPointerId(i)
will be executed which is not correct.
I attached a patch file, that is also more modern and straightforward.

There is one more issue. In src/video/android/SDL_androidtouch.c there is a boolean leftFingerDown that probably is meant to repair some extern bug, I don't know what. Anyhow I see no use for that. Maybe it's a good idea to remove it plus all the code that would be executed if its value was true.

In the same file there is:
// The following two are deprecated but it seems they are still emitted (instead the corresponding ACTION_UP/DOWN) as of Android 3.2
#define ACTION_POINTER_1_DOWN 5
#define ACTION_POINTER_1_UP 6

Actually this is not deprecated at all, but the names should be ACTION_POINTER_DOWN and ACTION_POINTER_UP. They are used if one finger goes up or down while there are other fingers still down.

Kind regards,
Wouter Boeke
Comment 1 Philipp Wiesemann 2013-01-12 14:42:49 UTC
If getActionMasked() [1] or getActionIndex() [2] is used this part will not work on Android 2.1 or older. See Bug 1563.

If pointerCount == 1 then pointerCount > 1 will be false. The supplied line numbers seem not to match the file in repository.

The leftFingerDown variable is used for mouse emulation. This is needed because the "left" mouse button can only go down once but on touch screen there may be multiple fingers down. This would fire multiple mouse down events (which would be bad emulation :).

The two constants are really deprecated on Java side [3] because there are new ones with the same value. How they are called on C side does not really matter because there is no semantic link to the Java side.

[1] http://developer.android.com/reference/android/view/MotionEvent.html#getActionMasked%28%29
[2] http://developer.android.com/reference/android/view/MotionEvent.html#getActionIndex%28%29
[3] http://developer.android.com/reference/android/view/MotionEvent.html#ACTION_POINTER_1_DOWN
Comment 2 wboe 2013-01-13 00:56:23 UTC
(In reply to comment #1)

> 
> If pointerCount == 1 then pointerCount > 1 will be false. The supplied line
> numbers seem not to match the file in repository.
> 

To clarify: if the action == ACTION_MOVE and pointerCount == 1, then pointerFingerId will have the value event.getPointerId(actionPointerIndex), which is the initial value which is only valid for a DOWN or an UP event.
Sorry for the wrong line numbers.
 
> 
> The two constants are really deprecated on Java side [3] because there are new
> ones with the same value. How they are called on C side does not really matter
> because there is no semantic link to the Java side.
>

Indeed, this was more like a cosmetic issue. Still I think that also the comments inside source code should be not misleading.
Comment 3 Gabriel Jacobo 2013-02-12 09:28:31 UTC
If you can update the patch without getActionMasked and getActionIndex I'll take a look at it to incorporate it. Thanks!
Comment 4 wboe 2013-02-13 00:05:31 UTC
(In reply to comment #3)
> If you can update the patch without getActionMasked and getActionIndex I'll
> take a look at it to incorporate it. Thanks!

Dear Gabriel,
What's the point of sticking to old Android API's because someone might be still using them? I think you underestimate the intellect of the average dev!
Anyhow, please keep the getActionMasked and getActionIndex, they make the code cleaner and better understandable.
Comment 5 Gabriel Jacobo 2013-02-13 03:17:56 UTC
You are missing the point, it's not a matter of developer intellect. The lowest API level SDL requires, the more devices it can run in. To force an increase in the minimum API level required, there needs to be some substantial gain with no way around it, in this case there's none that I can see.
Comment 6 wboe 2013-02-13 10:06:10 UTC
(In reply to comment #5)

I'm feeling very stupid, but I can't recover the original, patched SDLActivity.java. Please could you do it manually?

old:
final int action = event.getActionMasked();
new:
final int action = event.getAction() & MotionEvent.ACTION_MASK;

old:
int actionPointerIndex = event.getActionIndex();
new:
int actionPointerIndex = (event.getAction() & MotionEvent.ACTION_POINTER_ID_MASK) >> MotionEvent.ACTION_POINTER_ID_SHIFT;

Regards,
Wouter
Comment 7 wboe 2013-02-14 00:32:13 UTC
(In reply to comment #6)

Maybe the whole issue about getActionMasked() and getActionIndex() is moot, because of a statement in the Android documentation at
http://developer.android.com/tools/sdk/ndk/index.html. There is stated:

If you use this NDK to create a native library that uses the API to access Android Bitmap pixel buffers or utilizes native activities, the application containing the library can be deployed only to devices running Android 2.2 (API level 8) or higher. To ensure compatibility, make sure that your application declares <uses-sdk android:minSdkVersion="8" /> attribute value in its manifest.

This means that all that is available from version 8 can be used, including getActionMasked and getActionIndex
Comment 8 Sam Lantinga 2013-03-04 01:03:25 UTC
FYI, we're currently requiring SDK revision 10 for SDL 2.0.  I don't remember what bumped us to that, but that's where we're currently sitting.

Gabriel, at some point it's worth doing a review of the Android code and marking which revision we need and why, and upgrading code workarounds for older SDK versions we no longer need.
Comment 9 Gabriel Jacobo 2013-03-04 09:36:52 UTC
I've added a few comments on the README.android file stating the minimum API level required, and a short paragraph explaining why (I doubled checked it by trying to build against API level 8, there's no egl.h available in the NDK).

http://hg.libsdl.org/SDL/rev/6ce8edb5577b
Comment 10 Philipp Wiesemann 2013-03-05 17:01:04 UTC
(In reply to comment #9)
> I've added a few comments on the README.android file stating the minimum API
> level required, and a short paragraph explaining why (I doubled checked it
> by trying to build against API level 8, there's no egl.h available in the
> NDK).
> 
> http://hg.libsdl.org/SDL/rev/6ce8edb5577b

According to the NDK version history [1] support for OpenGL ES was added in September 2009. API 9 was released in December 2010. Therefore API 9 could be no requirement for using it.
Also the documentation [1] states that OpenGL ES 1.1 may be used with API level 4. Which means parts of SDL may be used with older Android versions.

The reason for API 10 in SDLs AndroidManifest.xml seems to have nothing to do with the source code itself. [2]

Also there may be a misunderstanding about "minSdkVersion" and "targetSdkVersion". [3]
I think "targetSdkVersion" should be "10" or higher while "minSdkVersion" currently may be "8" or lower. Last time I checked "7" seems to be working (in emulator).
The function "getExternalFilesDir" is used on C side which requires API 8 but if it is not used it will not matter.

SDL's README.Platforms states a different Android version. Maybe this could be adapted to fit "minSdkVersion".

[1] http://developer.android.com/tools/sdk/ndk/index.html
[2] http://hg.libsdl.org/SDL/rev/47ab7ba21530
[3] http://developer.android.com/guide/topics/manifest/uses-sdk-element.html
Comment 11 Gabriel Jacobo 2013-03-05 17:20:39 UTC
Did you look at the actual NDK files? Try compiling SDL for Android using the "android-8" platform from the NDK and see what happens.
Comment 12 Philipp Wiesemann 2013-03-05 17:44:32 UTC
(In reply to comment #11)
> Did you look at the actual NDK files? Try compiling SDL for Android using
> the "android-8" platform from the NDK and see what happens.

I compile with "targetSdkVersion" set to "10" and "android-10". Therefore it works or me.

The compiled objects still can be used on older Android devices down to the "minSdkVersion". Things like newer headers only exist at compile time.

Just take a look at the documentation I linked in my last post or try it with an emulator.
Comment 13 Gabriel Jacobo 2013-03-05 18:11:01 UTC
I've read the documentation quite a few times, thanks for the suggestion though.

As it is now, SDL's code requires (among other things, as you mentioned yourself) the egl.h header, which is present on the android-9 NDK platform, which puts us on an API level 10 requirement to build the library. Could the binaries generated with the android-9 platform work on lower API level devices? Maybe. Did you try it on a good number of API level 8 devices to be comfortable doing that recommendation to everyone? I sure did not, and I don't think we should recommend it.

Android is already a minefield of vendors and device idiosyncrasies, I don't think hinting to users that they could run this on API level 8 or lower has much benefit, nor is the idea of recommending "you can run on API level X if you remove Y feature" from the library, specially because API level 10 now comprises 90% of the market.
Comment 14 Philipp Wiesemann 2013-03-06 16:36:19 UTC
(In reply to comment #13)
> I've read the documentation quite a few times, thanks for the suggestion
> though.

Your changeset added (in my option) wrong information about OpenGL ES to the README.android. That is what I wanted to point out some posts ago and that is why I referenced the documentation (because the dates are in there) in the first place.
Comment 15 Philipp Wiesemann 2013-03-10 07:42:26 UTC
(In reply to comment #10)
> (In reply to comment #9)
> > I've added a few comments on the README.android file stating the minimum API
> > level required, and a short paragraph explaining why (I doubled checked it
> > by trying to build against API level 8, there's no egl.h available in the
> > NDK).
> > 
> > http://hg.libsdl.org/SDL/rev/6ce8edb5577b
> 
> According to the NDK version history [1] support for OpenGL ES was added in
> September 2009. API 9 was released in December 2010. Therefore API 9 could
> be no requirement for using it.
> Also the documentation [1] states that OpenGL ES 1.1 may be used with API
> level 4. Which means parts of SDL may be used with older Android versions.
> 
> [...]
> 
> SDL's README.Platforms states a different Android version. Maybe this could
> be adapted to fit "minSdkVersion".
> 
> [...]

I created bug #1748 to propose a patch which makes the text (in my opinion) more correct. I also searched for errors in other README files to make the patch more worthwhile.
Comment 16 Ryan C. Gordon 2013-07-12 22:15:29 UTC
(Sorry if you get a lot of copies of this email, we're touching dozens of bug reports right now.)

Tagging a bunch of bugs as target-2.0.0, Priority 2.

This means we're in the final stretch for an official SDL 2.0.0 release! These are the bugs we really want to fix before shipping if humanly possible.

That being said, we don't promise to fix them because of this tag, we just want to make sure we don't forget to deal with them before we bless a final 2.0.0 release, and generally be organized about what we're aiming to ship.

Hopefully you'll hear more about this bug soon. If you have more information (including "this got fixed at some point, nevermind"), we would love to have you come add more information to the bug report when you have a moment.

Thanks!
--ryan.
Comment 17 Sam Lantinga 2013-07-27 06:02:14 UTC
Gabriel, is there anything left that needs to be done to close this bug out?  It looks like a bunch of things were discussed.
Comment 18 Gabriel Jacobo 2013-07-27 09:15:16 UTC
The patch as is doesn't apply, and reviewing the discussion I don't think it's valid (as Philipp stated, if pointerCount == 1 then pointerCount is not > 1). Marking as WONTFIX but feel free to update the patch and re open if there's something of value that I'm missing.