-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
fix(controllers): restore legacy passing of HID data in plain array #11470
Conversation
I don't have a HID controller to test, so it would be nice if someone with the affected hardware could fix it.
|
I fully agree, that this is the right place to fix this! Thank you for providing this fix! |
Thank you. I don't think so. from what I can tell, these code parts are not particularly hot. Still, I'd really prefer if someone could test this before merging. |
Fixes mixxxdj#11461, alternative to mixxxdj#11463
confirm commit works against last pull on 2.4 for a NITrakKontrolS2MK3 tyvm! |
Thank you for testing. I'm still waiting on a review by @JoergAtGithub or someone else. Maybe @ywwg? Thanks. |
I will test it tonight! |
friendly ping @JoergAtGithub. If you don't have the time or resources to test this, you could probably also just merge this since it was already confirmed to be working. |
I will check this out this weekend - promised! |
* @param {number[] | ArrayBuffer | TypedArray} bufferLike Object that can be represented as a sequence of bytes | ||
* @returns {DataView} dataview over the bufferLike object | ||
*/ | ||
const createDataView = function(bufferLike) { | ||
return new DataView((() => { | ||
if (Array.isArray(bufferLike)) { | ||
return new Uint8ClampedArray(bufferLike).buffer; | ||
} else if (ArrayBuffer.isView(bufferLike)) { | ||
return bufferLike.buffer; | ||
} else { | ||
return bufferLike; | ||
} | ||
})()); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* @param {number[] | ArrayBuffer | TypedArray} bufferLike Object that can be represented as a sequence of bytes | |
* @returns {DataView} dataview over the bufferLike object | |
*/ | |
const createDataView = function(bufferLike) { | |
return new DataView((() => { | |
if (Array.isArray(bufferLike)) { | |
return new Uint8ClampedArray(bufferLike).buffer; | |
} else if (ArrayBuffer.isView(bufferLike)) { | |
return bufferLike.buffer; | |
} else { | |
return bufferLike; | |
} | |
})()); | |
}; | |
* @param {Uint8Array | number[] | ArrayBuffer} bufferLike Object that can be represented as a sequence of bytes | |
* @returns {DataView} dataview over the bufferLike object | |
*/ | |
const createDataView = function(bufferLike) { | |
if (bufferLike.constructor === Uint8Array) { | |
return new DataView(bufferLike.buffer); | |
} else if (Array.isArray(bufferLike)) { | |
return new DataView(new Uint8ClampedArray(bufferLike).buffer); | |
} else { | |
return new DataView(bufferLike); | |
} | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed a performance impact by your implementation. I played a bit with the code and this is faster. I guess because the bufferLike is in nearly all cases a Uint8Array.
I don't think there is a need to support any other TypedArray type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think there is a need to support any other TypedArray type.
I disagree, and I don't think it would be difficult to support it.
The improvement from performance is likely just the change in ordering of the if cases, can you experiement whether there is a significant difference when you change bufferLike.constructor === Uint8Array
back to ArrayBuffer.isView(bufferLike)
in your suggestion? Also I doubt the IIFE is make any difference either because it should just be inlined by the JIT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thee IIFE is removed for better code readabilty and I also doubt it affects the performance. I will benchmark your suggestions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thee IIFE is removed for better code readabilty
I guess this is certainly something to argue about, but I think the IIFE improves readability because it DRY's up the code. If I see the same code repeated in all branches, it usually creates suspicion for me that this could be the source of a bug from mindlessly copy-pasting code. But that is certainly controversial.
I tested this under various conditions on Windows 7 and Windows 11 and it always worked. There is a slight performance degrations but this is not so clear to measure. Therefore this is Ok for me, further performance improvements can be done in Follow-Up PRs if needed. |
Thank you for the review. 👍 |
Fixes #11461, alternative to #11463