Review Pen API

[icculus]

There are several (probably minor!) things that don't match our code or design style in the new pen API, so we should probably give this a more thorough looking-over before 3.2.0 locks down, in case we want to tweak things.

For example, SDL_GetPenCapabilities fills in a struct with pen info, but also returns capability bitflags, instead of putting everything in the struct and returning 0 or -1. Little things like that.

I'm assigning this to myself, since I'm probably going to write the missing documentation for SDL_pen.h, but I'll report back before making any API changes.

[icculus]

Okay, I spent a lot of today with this, and there's a lot I want to change.

At the public API level:

• Capabilities and input state share bits and symbols, and this is confusing. It has separate mutually-exclusive bits for drawing vs erasing. Splitting it into input states and capabilities is more clear, easier to document, and looks like other bitfield typedefs.

/**
 * Pen input flags, as reported by SDL_GetPenStatus and various events.
 *
 * The `SDL_PEN_INPUT_BUTTON_*` defines happen to match SDL_MouseButtonFlags.
 *
 * \since This datatype is available since SDL 3.0.0.
 */
typedef Uint32 SDL_PenInputFlags;
#define SDL_PEN_INPUT_BUTTON_1     1   /**< Bit index for pen button 1 is pressed */
#define SDL_PEN_INPUT_BUTTON_2     2   /**< Bit index for pen button 2 is pressed */
#define SDL_PEN_INPUT_BUTTON_3     3   /**< Bit index for pen button 3 is pressed */
#define SDL_PEN_INPUT_BUTTON_4     4   /**< Bit index for pen button 4 is pressed */
#define SDL_PEN_INPUT_BUTTON_5     5   /**< Bit index for pen button 5 is pressed */
#define SDL_PEN_INPUT_ERASER_TIP  30   /**< Bit index for pen is using eraser tip, not regular drawing tip */
#define SDL_PEN_INPUT_DOWN        29   /**< Bit index for pen is touching the surface */

#define SDL_PEN_INPUT(X)              (1u << ((X)-1))
#define SDL_PEN_INPUT_BUTTON_1_MASK   SDL_PEN_INPUT(SDL_PEN_INPUT_BUTTON_1)   /**< & to see if button 1 is pressed */
#define SDL_PEN_INPUT_BUTTON_2_MASK   SDL_PEN_INPUT(SDL_PEN_INPUT_BUTTON_2)   /**< & to see if button 2 is pressed */
#define SDL_PEN_INPUT_BUTTON_3_MASK   SDL_PEN_INPUT(SDL_PEN_INPUT_BUTTON_3)   /**< & to see if button 3 is pressed */
#define SDL_PEN_INPUT_BUTTON_4_MASK   SDL_PEN_INPUT(SDL_PEN_INPUT_BUTTON_4)   /**< & to see if button 4 is pressed */
#define SDL_PEN_INPUT_BUTTON_5_MASK   SDL_PEN_INPUT(SDL_PEN_INPUT_BUTTON_5)   /**< & to see if button 5 is pressed */
#define SDL_PEN_INPUT_ERASER_TIP_MASK SDL_PEN_INPUT(SDL_PEN_INPUT_ERASER_TIP) /**< & to see if eraser tip is used */
#define SDL_PEN_INPUT_DOWN_MASK       SDL_PEN_INPUT(SDL_PEN_INPUT_DOWN)       /**< & to see if pen is down */


/**
 * SDL_PenInfo capability flags.
 *
 * \since This datatype is available since SDL 3.0.0.
 */
typedef Uint32 SDL_PenCapabilityFlags;

#define SDL_PEN_CAPABILITY_ERASER    0x00000001u  /**< Pen also has an eraser tip */
#define SDL_PEN_CAPABILITY_PRESSURE  0x00000002u  /**< Pen provides pressure information in axis SDL_PEN_AXIS_PRESSURE */
#define SDL_PEN_CAPABILITY_XTILT     0x00000004u  /**< Pen provides horizontal tilt information in axis SDL_PEN_AXIS_XTILT */
#define SDL_PEN_CAPABILITY_YTILT     0x00000008u  /**< Pen provides vertical tilt information in axis SDL_PEN_AXIS_YTILT */
#define SDL_PEN_CAPABILITY_DISTANCE  0x00000008u  /**< Pen provides distance to drawing tablet in axis SDL_PEN_AXIS_DISTANCE */
#define SDL_PEN_CAPABILITY_ROTATION  0x00000008u  /**< Pen provides barrel rotation info in axis SDL_PEN_AXIS_ROTATION */
#define SDL_PEN_CAPABILITY_SLIDER    0x00000008u  /**< Pen provides slider/finger wheel/etc in axis SDL_PEN_AXIS_SLIDER */

#define SDL_PEN_CAPABILITY_BIDIRECTIONAL (SDL_PEN_CAPABILITY_XTILT | SDL_PEN_CAPABILITY_YTILT)  /**< for convenience */

• There are APIs to query a pen's GUID, its subtype, and its capabilities. I'd move it to a single SDL_GetPenInfo() function:

/**
 * Pen hardware information, as reported by SDL_GetPenInfo()
 *
 * \since This struct is available since SDL 3.0.0.
 *
 * \sa SDL_GetPenInfo
 */
typedef struct SDL_PenInfo
{
    SDL_GUID guid;      /**< a (best-attempt) unique identifier for this hardware. */
    SDL_PenCapabilityFlags capabilities;  /**< bitflags of device capabilities */
    float max_tilt;    /**< Physical maximum tilt angle, for XTILT and YTILT, or -1.0f if unknown.  Pens cannot typically tilt all the way to 90 degrees, so this value is usually less than 90.0. */
    Uint32 wacom_id;   /**< For Wacom devices: wacom tool type ID, otherwise 0 (useful e.g. with libwacom) */
    int num_buttons; /**< Number of pen buttons (not counting the pen tip), or -1 if unknown. */
    SDL_PenSubtype subtype;  /**< type of pen device */
} SDL_PenInfo;

/**
 * Retrieves hardware details for a given SDL_PenID.
 *
 * \param instance_id The pen to query.
 * \param info Filled in with information about pen details. Can be NULL.
 * \returns zero on success, -1 on error.
 *
 * \since This function is available since SDL 3.0.0.
 */
extern SDL_DECLSPEC int SDLCALL SDL_GetPenInfo(SDL_PenID instance_id, SDL_PenInfo *info);

• I wouldn't move all this stuff to properties, since most of it is stuff you're going to need to know to effectively work with a pen, but I can be persuaded. Also, there's currently no SDL_GetPenProperties(), we might want to add that.

Internally:

• The platform backend interface could benefit greatly from heavy simplification.
• There's a whole thing about garbage collecting devices and other life-cycle concerns in the upper-level code that doesn't match other SDL subsystems, and adds a lot of unnecessary complications.
• There are only X11 and Wayland implementations of this at the moment.

I spent some time scrubbing through SDL_pen.c, fixing and cleaning as I went, but I'm finding I'm refactoring more than touching things up. I might stash this and just make the changes I need to support the API changes I want for now so I don't get sucked into a long effort.

But I'm worried there's a lot of work in our future if we want this to be robust for a 3.2.0 release.

[AntTheAlchemist]

I'd love to tinker with the Pen API (it'd also make a great addition to my gamepad testing app).

Can anyone please suggest a suitable low-priced pen device I can buy? I've looked on eBay but it's a confusing mix of mostly passive devices that don't do anything more than a finger would.

[icculus]

I bought a Wacom Intuos tablet yesterday for 40 bucks, and Amazon had it on my doorstep 3 hours later. 😯

https://www.amazon.com/dp/B079HL9YSF

I haven't taken it out of the box yet, so I can't say if it's a good tablet, but I think it will suffice for testing.

[AntTheAlchemist]

@icculus Thanks! Amazon logistics is crazy impressive, even here in the UK.

What are you currently using to test the API?

[icculus]

Nothing yet, I'm just getting started. :)

But later today: I'll be using that tablet on a Linux box, until we have more than X11 and Wayland support implemented.

[AntTheAlchemist]

I just got the UGEE S640 for 22 quid (28 bucks) on Android. That way, we both have a different device to test the API with. It's purple, but I don't care 😆

[ericoporto]

If there’s some testing software that can be built for testing (like that one test for Gamepad) I have an old Wacom Bamboo connected to my Mac mini and an Android Galaxy Note 9 phone too.

[icculus]

Update: I have a PR mostly ready to clean up the public API, just trying to get testautomation_pen.c back in working order.

[icculus]

Okay, the new API is in. We do not have an Android backend yet, @AntTheAlchemist, but most of the work is already done there (it's just the same MotionEvent objects we already process for touch events, but they have extra information in them when a stylus generates them). Once I get a working Android system where I can test this, I'll fill it in.

SDL/test/testpen.c is the fastest way to see if this works on your system (on X11, Wayland, or macOS right now).

I'm closing this issue, as fixing the interface before ABI lock was the task here. We are tracking work on the missing backends in #10516.