[+] PDX ExclusiveTouch #110
Conversation
Summary of ChangesHello @clansty, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the touch input system, transitioning from a monolithic static class to an extensible, object-oriented design. The primary goal is to enable robust support for multiple distinct touch devices, particularly for "PDX ExclusiveTouch" panels, by abstracting common touch handling logic and providing a concrete implementation for specific hardware. This change enhances configurability, allowing users to specify devices for individual players and improving the accuracy of touch detection through refined sensor mapping. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and well-executed refactoring by creating a reusable ExclusiveTouchBase for handling touch input from various devices, demonstrated by the new PdxTouch implementation. The changes are a solid improvement for supporting multi-player setups. I have a few suggestions to enhance the code's clarity and robustness, mainly concerning the device initialization logic and ensuring resources are properly cleaned up on application exit.
| Application.quitting += () => | ||
| { | ||
| IUsbDevice wholeDevice = device as IUsbDevice; | ||
| var tmpDevice = device; | ||
| device = null; | ||
| if (wholeDevice != null) | ||
| { | ||
| wholeDevice.SetConfiguration(configuration1p); | ||
| wholeDevice.ClaimInterface(interfaceNumber1p); | ||
| } | ||
| touchSensorMappers[0] = new TouchSensorMapper(minX1p, minY1p, maxX1p, maxY1p, radius1p, flip1p); | ||
| Application.quitting += () => | ||
| { | ||
| devices[0] = null; | ||
| if (wholeDevice != null) | ||
| { | ||
| wholeDevice.ReleaseInterface(interfaceNumber1p); | ||
| } | ||
| device.Close(); | ||
| }; | ||
|
|
||
| allFingerPoints[0] = new TouchPoint[256]; | ||
| for (int i = 0; i < 256; i++) | ||
| { | ||
| allFingerPoints[0][i] = new TouchPoint(); | ||
| wholeDevice.ReleaseInterface(interfaceNumber); | ||
| } | ||
| tmpDevice.Close(); | ||
| }; |
There was a problem hiding this comment.
In the Application.quitting event handler, if wholeDevice.ReleaseInterface(interfaceNumber) throws an exception, tmpDevice.Close() will not be called, which could lead to an unclosed resource. It's safer to wrap the resource release logic in a try...finally block to guarantee that Close() is always called.
Application.quitting += () =>
{
var tmpDevice = device;
device = null;
try
{
if (wholeDevice != null)
{
wholeDevice.ReleaseInterface(interfaceNumber);
}
}
finally
{
tmpDevice.Close();
}
};| public static void OnBeforePatch() | ||
| { | ||
| if (string.IsNullOrWhiteSpace(serial1p) && string.IsNullOrWhiteSpace(serial2p)) | ||
| { | ||
| devices[0] = new PdxTouchDevice(0, null); | ||
| devices[0].Start(); | ||
| } | ||
| if (!string.IsNullOrWhiteSpace(serial1p)) | ||
| { | ||
| devices[0] = new PdxTouchDevice(0, serial1p); | ||
| devices[0].Start(); | ||
| } | ||
| if (!string.IsNullOrWhiteSpace(serial2p)) | ||
| { | ||
| devices[1] = new PdxTouchDevice(1, serial2p); | ||
| devices[1].Start(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for initializing devices in OnBeforePatch is a bit complex to follow due to multiple, non-exclusive if statements. This can be simplified into a more readable and maintainable structure that achieves the same result but is clearer about its intent.
public static void OnBeforePatch()
{
// Initialize 1P device if serial1p is provided, or if no serials are provided at all (auto-detect mode).
// This logic does not initialize 1P if only 2P serial is provided, matching original behavior.
if (!string.IsNullOrWhiteSpace(serial1p) || string.IsNullOrWhiteSpace(serial2p))
{
devices[0] = new PdxTouchDevice(0, serial1p);
devices[0].Start();
}
// Initialize 2P device if serial2p is provided.
if (!string.IsNullOrWhiteSpace(serial2p))
{
devices[1] = new PdxTouchDevice(1, serial2p);
devices[1].Start();
}
}| } | ||
| } | ||
|
|
||
| private class PdxTouchDevice(int playerNo, string serialNumber) : ExclusiveTouchBase(playerNo, vid: 0x3356, pid: 0x3003, serialNumber, configuration: 1, interfaceNumber: 1, ReadEndpointID.Ep02, packetSize: 64, minX: 18432, minY: 0, maxX: 0, maxY: 32767, flip: true, radius) |
There was a problem hiding this comment.
The constructor call for ExclusiveTouchBase uses several magic numbers for device-specific parameters (VID, PID, coordinates, etc.). To improve readability and maintainability, consider defining these as named constants within the PdxTouchDevice class and using them in the constructor call.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| var finder = new UsbDeviceFinder(vid1p, pid1p, serialNumber); | ||
| var device = UsbDevice.OpenUsbDevice(finder); | ||
| if (device == null) | ||
| MelonLogger.Msg("[ExclusiveTouch] Cannot connect 1P"); |
There was a problem hiding this comment.
Error message always shows "1P" regardless of player
Low Severity
The error message "[ExclusiveTouch] Cannot connect 1P" is hardcoded and will display "1P" even when attempting to connect a 2P device. The class has access to playerNo parameter which could be used to display the correct player number, as done elsewhere in the code (line 85 uses {playerNo + 1}P).
Note
Adds PDX exclusive touch support and refactors touch handling
PdxTouchwith config forradius, optionalserial1p/serial2p, and per-player device startup using VID/PID and PDX report parsingExclusiveTouchBase: per-device USB setup, interface claim/release, read thread, finger state tracking, and registration viaTouchStatusProviderTouchSensorMapper.ParseTouchPointto handleradiusmore robustly (circle-edge/interior checks when radius > 0, point-in-polygon when 0)Removes previous static config-driven
ExclusiveTouchimplementation in favor of instance-based design.Written by Cursor Bugbot for commit 0e69ffc. This will update automatically on new commits. Configure here.