-
Notifications
You must be signed in to change notification settings - Fork 13
Update controller asset #11
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
salarkhan-google
left a comment
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.
Update resource to latest
|
Hello. |
salarkhan-google
left a comment
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.
Everything is almost good. Just need to update the shader and baseColor texture, and ensure all textures have sRGB set to false. Then the controller looks more like the sysui controller.
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.
You can get rid of this since now we have a single texture storing metallic and roughness
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.
Removed.
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 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.
| float3 arm = tex2D (_AoRoughnessMetallic, IN.uv_BaseColor); | ||
| o.Metallic = arm.b; | ||
| o.Smoothness = 1 - arm.g; | ||
| o.Occlusion = 1; |
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.
o.Occlusion = arm.r;
according to the material in sysui
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.
Updated
|
From your scripts and shader file, please remove all trailing white space. Use space instead of tabs. |
salarkhan-google
left a comment
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.
Need to ensure you format your code correctly according to C# standards.
SA1600: Public classes and methods must have a document header
US1400: Indentations must use 4 spaces
US1300: Lines must contain 100 characters or fewer. If contains more than 100 characters, split over multiple lines in clang format.
SA1215: All non-static readonly private fields must be placed before all non-static non-readonly private fields.
Please check the scripts in the other samples to understand the formatting.
| /// </summary> | ||
| public class XRControllerDisplay : MonoBehaviour | ||
| { | ||
| #pragma warning disable CS0649 // Serialized fields don't need assignment |
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.
Either bring back the warning disabler or ensure to assign private fields with some default value.
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.
Add the warning disabler
| [SerializeField] private Transform _targetObject; | ||
| [SerializeField] private Vector3 _releasedPosition; | ||
| [SerializeField] private Vector3 _pressedPosition; | ||
| [SerializeField] private Vector3 _releasedRotation; | ||
| [SerializeField] private Vector3 _pressedRotation; |
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.
Even here consider adding the #pragma warning disable CS0649 // Serialized fields don't need assignment.
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.
Add the warning disabler
| private static void EnableActions(params InputAction[] actions) | ||
| { | ||
| foreach (var a in actions) a?.Enable(); | ||
| } | ||
|
|
||
| private static void DisableActions(params InputAction[] actions) | ||
| { | ||
| foreach (var a in actions) a?.Disable(); | ||
| } |
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.
Doesn't make sense to make these methods static since they are private anyways.
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.
Updated
| private readonly List<(InputAction action, | ||
| Action<InputAction.CallbackContext> onStarted, | ||
| Action<InputAction.CallbackContext> onPerformed, | ||
| Action<InputAction.CallbackContext> onCanceled)> _bindings = new(); |
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.
readonly members should go above all private members.
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.
Updated
| private readonly List<(InputAction action, | ||
| Action<InputAction.CallbackContext> onStarted, | ||
| Action<InputAction.CallbackContext> onPerformed, | ||
| Action<InputAction.CallbackContext> onCanceled)> _bindings = new(); |
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.
seperate () and new to make it "new ()"
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.
Updated
| } | ||
|
|
||
| [Serializable] | ||
| public class XrControllerButtonInfo |
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.
Need public class description header
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.
Add the description header.
| public Vector3 GetPositionLerp(float t) | ||
| { | ||
| return Vector3.Lerp(_releasedPosition, _pressedPosition, t); | ||
| } | ||
|
|
||
| public Vector3 GetRotationLerp(float t) | ||
| { | ||
| return Vector3.Lerp(_releasedRotation, _pressedRotation, t); | ||
| } | ||
|
|
||
| public Quaternion GetRotationQuaternionLerp(float t) | ||
| { | ||
| return Quaternion.Euler(Vector3.Lerp(_releasedRotation, _pressedRotation, t)); | ||
| } | ||
|
|
||
| public void SetStatus(float t) | ||
| { | ||
| if (_targetObject == null) return; | ||
| t = Mathf.Clamp01(t); | ||
|
|
||
| _targetObject.localPosition = Vector3.Lerp(_releasedPosition, _pressedPosition, t); | ||
| _targetObject.localRotation = Quaternion.Lerp( | ||
| Quaternion.Euler(_releasedRotation), | ||
| Quaternion.Euler(_pressedRotation), | ||
| t); | ||
| } |
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.
Need public method description headers.
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.
Add the description header.
| /// <summary> | ||
| /// Used to display the controller asset. | ||
| /// </summary> |
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.
Do not remove class description headers.
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.
Add the description header.
|
Thanks for the comment. I’ve updated it. |
| Action<InputAction.CallbackContext> onStarted, | ||
| Action<InputAction.CallbackContext> onPerformed, | ||
| Action<InputAction.CallbackContext> onCanceled)> _bindings = new (); | ||
|
|
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.
Please remove trailing white space
| ENDCG | ||
| } | ||
| FallBack "Diffuse" | ||
| } No newline at end of file |
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.
All source files must have a new line at end
| ENDCG | ||
| } | ||
| } | ||
| } No newline at end of file |
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.
All source files must have new line at end
| } | ||
| } | ||
| } | ||
| } |
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.
All source files must have new line at end
| if (_thumbstickTransform != null) | ||
| _thumbstickTransform.localRotation = _thumbstickInitialRotation; |
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.
Please add curly brackets to all if statements. if statements without curly brackets are not allowed. Ensure each bracket is on new line. Ensure there is a one line space between ending curly bracket and statements below it.
| _triggerAxisAction, | ||
| _gripAxisAction, | ||
| _thumbstickAxisAction | ||
| ); |
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.
Closing round bracket must be on same line as final statement.
|
@hj-cpdm-son please bring my recent changes on the main branch in your fork main branch. My new changes contains a github action which should then help you identify formatting errors or build related issues. |
* Update controller asset * Update resources to latest * Update resources(scripts, png) * Update code format



Before

After
