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.

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

[HDRP Template] Upgrade Input System to 1.8.1 in 3D Sample #8056

Merged
merged 8 commits into from
Apr 15, 2024

Conversation

jfreire-unity
Copy link
Contributor


Purpose of this PR

  • Upgrading to latest Input System package 1.8.1
  • Changing Project Settings > Active Input Handling to "Both". This should overcome issues with UGUI reported here.

Testing status

❌ No local testing done yet


Comments to reviewers

Notes for the reviewers you have assigned.

Copy link

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to add EditorBuildAsset file that references InputSystem_Actions.asset

Currently this file has no reference to the newly created project wide input actions asset in the Assets folder

@Pauliusd01
Copy link

  • I see that the player is set to use both input backends but there's no PWA asset by default, shouldn't that be changed?
  • There's supposed to be 2 types of HDRP projects an empty one and sample one and it seems like only the sample one was changed. Shouldn't both be changed?
  • Is this template project supposed to be spamming all these errors?
    image

Copy link

@lyndon-unity lyndon-unity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Config objects info in EditorBuildSettings looks good - but looks like one extra unnecessary line changes which is probably not needed and may have a side effect. Suggest manually putting that one line back

@@ -6,6 +6,8 @@ EditorBuildSettings:
serializedVersion: 2
m_Scenes:
- enabled: 1
path: Assets/SampleSceneAssets/Scenes/SampleScene.unity
path: Assets/Scenes/SampleScene.unity

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line change is probably unnecessary AFAICT - can you revert that one line?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, can do - the (updated) path is correct though, the old one doesn't exist, is that fine?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok if that's a correction then great lets land it as is. Thank you.

Copy link

@Pauliusd01 Pauliusd01 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, the player is set to use both backends, EventSystem no longer throws the errors and a default ProjectWideActions asset is present in the main Assets directory (The only thing that might be a bit odd that It's just a loose file instead of being nested in some Settings folder)

Also, double checked basic uGUI and UIToolkit functionality by creating and checking functionality of buttons, toggles, input fields, etc.

@@ -6,6 +6,8 @@ EditorBuildSettings:
serializedVersion: 2
m_Scenes:
- enabled: 1
path: Assets/SampleSceneAssets/Scenes/SampleScene.unity
path: Assets/Scenes/SampleScene.unity

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok if that's a correction then great lets land it as is. Thank you.

@graham-huws graham-huws removed the request for review from stefanunity April 5, 2024 13:18
Copy link
Collaborator

@LagueKristin LagueKristin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed outstanding issues, bumped version and changelog changes!

@LagueKristin LagueKristin merged commit 6ea1d0b into master Apr 15, 2024
1 check passed
@LagueKristin LagueKristin deleted the update-project-active-input-handling branch April 15, 2024 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants