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.

Sign up for GitHub

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

Jump to bottom

fix(controllers): restore legacy passing of HID data in plain array #11470

Merged
merged 1 commit into from
May 8, 2023
Merged

fix(controllers): restore legacy passing of HID data in plain array #11470

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 20 additions & 2 deletions res/controllers/common-hid-packet-parser.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,24 @@
this.HIDDebug = function(message) {
console.log(`HID ${message}`);
};
/**
* creates a `DataView` from any ArrayBuffer, TypedArray
* or plain Array (clamped to 8-bit width).
*
* @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;
}
})());
};
Comment on lines +16 to +29
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
* @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);
}
};

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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.


/**
* Callback function to call when, the packet represents an HID InputReport, and new data for this
Expand Down Expand Up @@ -413,7 +431,7 @@ class HIDPacket {
return;
}

const dataView = new DataView(data.buffer);
const dataView = createDataView(data);
switch (field.pack) {
case "b":
dataView.setInt8(field.offset, value);
Expand Down Expand Up @@ -453,7 +471,7 @@ class HIDPacket {
* @returns {number} Value for the field in data, represented according the fields packing type
*/
unpack(data, field) {
const dataView = new DataView(data.buffer);
const dataView = createDataView(data);
switch (field.pack) {
case "b":
return dataView.getInt8(field.offset);
Expand Down