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

[Android] Stylus support #3111

Open
wants to merge 40 commits into
base: main
Choose a base branch
from
Open

[Android] Stylus support #3111

wants to merge 40 commits into from

Conversation

m-bert
Copy link
Contributor

@m-bert m-bert commented Sep 17, 2024

Description

This PR adds stylus support on Android.

Note

You can read more about this feature in #3107

Test plan

Tested on StylusData example

@m-bert m-bert marked this pull request as ready for review September 18, 2024 08:50
@m-bert m-bert changed the base branch from main to @mbert/stylus-support-web September 18, 2024 08:50
@m-bert m-bert marked this pull request as draft September 19, 2024 08:12
@m-bert m-bert marked this pull request as ready for review September 19, 2024 09:18
@m-bert m-bert requested review from latekvo and j-piasecki and removed request for latekvo September 19, 2024 09:18
Base automatically changed from @mbert/stylus-support-web to main September 19, 2024 09:50
// To get azimuth angle, we need to use orientation property. Orientation value range is [-PI, PI] (https://developer.android.com/develop/ui/compose/touch-input/stylus-input/advanced-stylus-features#orientation)
// To shift range into [0, 2PI], we add 2PI if orientation is less than 0.
if (orientation < 0) {
orientation += 2 * PI
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't that shift it to [PI, 3PI]?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's possible that I missed something, but that is why there's an if expression. I'm only shifting angles from [-PI, 0] - that should result in new range being [PI, 2PI]. Combined with second half we get range [0, 2PI] - a full circle.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, it doesn't matter since now we use formula with modulo (mentioned in the comment below)

Comment on lines 74 to 78
val azimuthAngle = if (orientation >= 3 * PI / 2) {
orientation - 3 * PI / 2
} else {
orientation + PI / 2
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't it simply be orientation - 3PI/2 (mod 2PI)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Almost, as discussed it should be

$$(orientation + \frac{\pi}{2})\mod{2\pi}$$

Changed in 560c28a.

Comment on lines 82 to 86
stylusData.tiltX = tilts.first
stylusData.tiltY = tilts.second
stylusData.altitudeAngle = altitudeAngle
stylusData.azimuthAngle = azimuthAngle
stylusData.pressure = pressure
Copy link
Member

Choose a reason for hiding this comment

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

I think it would be better to assign a new object here. Since it's passed referentially to the EventDataBuilder, changes like that may modify an already scheduled but not yet sent event. (This would also warrant a name change)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought that it will be too expensive, but we can get back to it.

Done in 29d2d67

Comment on lines 144 to 154
fun createStylusDataObject(stylusData: StylusData): WritableMap {
val stylusDataObject = Arguments.createMap()

stylusDataObject.putDouble("tiltX", stylusData.tiltX)
stylusDataObject.putDouble("tiltY", stylusData.tiltY)
stylusDataObject.putDouble("altitudeAngle", stylusData.altitudeAngle)
stylusDataObject.putDouble("azimuthAngle", stylusData.azimuthAngle)
stylusDataObject.putDouble("pressure", stylusData.pressure)

return stylusDataObject
}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fun createStylusDataObject(stylusData: StylusData): WritableMap {
val stylusDataObject = Arguments.createMap()
stylusDataObject.putDouble("tiltX", stylusData.tiltX)
stylusDataObject.putDouble("tiltY", stylusData.tiltY)
stylusDataObject.putDouble("altitudeAngle", stylusData.altitudeAngle)
stylusDataObject.putDouble("azimuthAngle", stylusData.azimuthAngle)
stylusDataObject.putDouble("pressure", stylusData.pressure)
return stylusDataObject
}
fun createStylusDataObject(stylusData: StylusData) = Arguments.createMap().apply {
putDouble("tiltX", stylusData.tiltX)
putDouble("tiltY", stylusData.tiltY)
putDouble("altitudeAngle", stylusData.altitudeAngle)
putDouble("azimuthAngle", stylusData.azimuthAngle)
putDouble("pressure", stylusData.pressure)
}

This can also be moved to a method on StylusData as toReadableMap. I think WritableMap can be assigned to ReadableMap and it makes it clear that it shouldn't be modified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in 1ab4c37

@@ -11,6 +11,7 @@ import com.swmansion.gesturehandler.react.RNViewConfigurationHelper
class HoverGestureHandler : GestureHandler<HoverGestureHandler>() {
private var handler: Handler? = null
private var finishRunnable = Runnable { finish() }
var stylusData: StylusData = StylusData(0.0, 0.0, 0.0, 0.0, -1.0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var stylusData: StylusData = StylusData(0.0, 0.0, 0.0, 0.0, -1.0)
var stylusData: StylusData = StylusData(0.0, 0.0, 0.0, 0.0, -1.0)
private set

And please move the magic numbers to be the default values in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b8203f2 and 4034348

Comment on lines 33 to 35
if (stylusData.pressure != -1.0) {
putMap("stylusData", GestureUtils.createStylusDataObject(stylusData))
}
Copy link
Member

Choose a reason for hiding this comment

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

What's the pressure when hovering?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is 0

Comment on lines 216 to 218
if (state == STATE_UNDETERMINED) {
stylusData.pressure = -1.0
}
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be inside onReset? And please make a new object.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b896f2e

Comment on lines 4 to 8
var tiltX: Double,
var tiltY: Double,
var altitudeAngle: Double,
var azimuthAngle: Double,
var pressure: Double
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var tiltX: Double,
var tiltY: Double,
var altitudeAngle: Double,
var azimuthAngle: Double,
var pressure: Double
val tiltX: Double,
val tiltY: Double,
val altitudeAngle: Double,
val azimuthAngle: Double,
val pressure: Double

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b896f2e

@@ -45,6 +45,7 @@ class PanGestureHandler(context: Context?) : GestureHandler<PanGestureHandler>()
private var activateAfterLongPress = DEFAULT_ACTIVATE_AFTER_LONG_PRESS
private val activateDelayed = Runnable { activate() }
private var handler: Handler? = null
var stylusData: StylusData = StylusData(0.0, 0.0, 0.0, 0.0, -1.0)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
var stylusData: StylusData = StylusData(0.0, 0.0, 0.0, 0.0, -1.0)
var stylusData: StylusData = StylusData(0.0, 0.0, 0.0, 0.0, -1.0)
private set

And the same deal with numbers

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in b8203f2 and 4034348

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants