-
Notifications
You must be signed in to change notification settings - Fork 59
Input System Touch Controls #1104
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
Conversation
Moving on to create a joystick
/** @returns {number} a number between -1 and 1 for this input. */ | ||
abstract getValue(useGamepad: boolean): number | ||
// Returns the current value of the input. Range depends on input type | ||
abstract getValue(useGamepad: boolean, useTouchControls: boolean): number |
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.
What if instead of getValue()
taking in useTouchControls
and useGamepad
, the input scheme stored an enum InputType, replacing the boolean it currently has called useGamepad
? That would avoid different input type conditions originating from different places and potentially overriding each other.
export enum InputType {
KEYBOARD,
GAMEPAD,
TOUCH
}
export type InputScheme = {
schemeName: string
descriptiveName: string
customized: boolean
inputType: InputType
inputs: Input[]
}
@@ -18,8 +20,8 @@ abstract class Input { | |||
this.inputName = inputName | |||
} | |||
|
|||
/** @returns {number} a number between -1 and 1 for this input. */ | |||
abstract getValue(useGamepad: boolean): number | |||
// Returns the current value of the input. Range depends on input 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.
Every input should be between -1 and 1 for consistency
Changes that need to be made: Only one of the usegamepad and usetouchcontrols checkboxes can be selected at a time When creating a new scheme you may click an add joint button to add another joint configuration.
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.
This is looking good, but I have a couple requests and recommendations:
-
If you aren't on a device that supports touch controls, the touch control scheme should not be selectable because there's not way to control it. If we keep a similar button, consider making it more opaque and blurring the background.
-
Instead of a button to open touch controls, they should automatically appear if there's a robot spawned that uses them.
-
The place assembly button text is really hard to read with some backgrounds. It also looks a little out of place. I think this is because the colors and style are so different than the others buttons. Maybe it could appear on the panel that that loading bar is on once it finishes loading?
-
When I connect to the network version, I get the error
Uncaught TypeError: can't access property "getDirectory", navigator.storage is undefined MirabufLoader.ts:29
. Is this just me?
Redundant after merge
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.
There are a lot of scaling issues on a phone, but I am assuming those either don't happen on a tablet/iPad or will be fixed later
The scaling issues with be fixed with the UI refactor |
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 get an error when I try to spawn a robot after spawning a field. The error message is: Place assembly before spawning another (on desktop).
Must be a merging error; I'll look into that |
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.
Note: with the addition of the usesTouchControls, prettier will autoformat the axisInputs over multiple lines rather than condensing them.
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.
Now why does prettier want to do this? Who knows 🤷
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.
The code looks good.
As far as functionality goes, it was great. If I were to suggest anything, I'd say I think touch controls should be enabled by default on small screens, but not needed.
Also, I had trouble actually testing the functionality of the joysticks because seemingly the resolution of my Pixel 7 is too low and the config GUI was getting cut off (I couldn't close it). Ended up using a sim and it seemed fine.
To support more mobile devices, we may consider adding a fullscreen button, and UI scaling options.
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.
Think at this point these changes are good to go in. There is still lots of problems with getting everything working on mobile (particularly when it comes to smaller screens) however at this point the necessary changes are out of scope.
Few things to note for mobile updates that are needed:
- Gamepad/Tough Control Checkbox Conflict AARD-1945.
- Prevent Joint Auto-Configuration on New Touch Control Setup AARD-1946.
- Implement Tough Control Top Auction UI AARD-1947.
- Implement Tough Control Joint Slider UI AARD-1948.
- General Mobile Device UI Updates (for small screen scaling issues +extra) AARD-1950.
That being said, these issues already existed and this PR is one more step to getting everything working on mobile. Glad to get the last major PR from last year done. 📵
Description
Creating touch controls joysticks for mobile devices. To enable the touch controls, you must be on a device with touch capabilities. When this happens, the Enable touch controls button will appear in the MainHUD.
After enabling the touch controls the controls themselves will appear on screen.
To use them, you must configure a inputscheme or use the preset "Brandon"
Notes
Testing
To test this on a mobile device:
bun run host
ornpm run host
to expose it to the networkJIRA Issue