[WIP] Recorder sample rework

[Pauliusd01]

Description

Please fill this section with a description what the pull request is trying to address and what changes were made.

Testing status & QA

Please describe the testing already done by you and what testing you request/recommend QA to execute. If you used or created any testing project please link them here too for QA.

Overall Product Risks

Please rate the potential complexity and halo effect from low to high for the reviewers. Note down potential risks to specific Editor branches if any.

• Complexity:
• Halo Effect:

Comments to reviewers

Please describe any additional information such as what to focus on, or historical info for the reviewers.

Checklist

Before review:

• Changelog entry added.
  • Explains the change in Changed, Fixed, Added sections.
  • For API change contains an example snippet and/or migration example.
  • JIRA ticket linked, example (case %%). If it is a private issue, just add the case ID without a link.
  • Jira port for the next release set as "Resolved".
• Tests added/changed, if applicable.
  • Functional tests Area_CanDoX, Area_CanDoX_EvenIfYIsTheCase, Area_WhenIDoX_AndYHappens_ThisIsTheResult.
  • Performance tests.
  • Integration tests.
• Docs for new/changed API's.
  • Xmldoc cross references are set correctly.
  • Added explanation how the API works.
  • Usage code examples added.
  • The manual is updated, if needed.

During merge:

• Commit message for squash-merge is prefixed with one of the list:
  • NEW: ___.
  • FIX: ___.
  • DOCS: ___.
  • CHANGE: ___.
  • RELEASE: 1.1.0-preview.3.

[u-pr]

PR Reviewer Guide 🔍

(Review updated until commit 99d690a)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪ The PR introduces several new classes implementing a rhythm game logic with input handling, which requires careful checking of state management and input system integration.

🏅 Score: 85 The code is generally clean and readable, but contains bad practices regarding Input System usage (checking specific devices inside a generic controller) and Singleton initialization timing.

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Improper Input Checks The code explicitly checks Mouse.current and Pointer.current global states inside OnTriggerExit. This will cause all instances of ButtonController (different lanes) to register a press simultaneously if the mouse is clicked, leading to gameplay bugs. Input checking should rely solely on the assigned pressAction. float mouseValue = Mouse.current != null ? Mouse.current.leftButton.ReadValue() : 0f; float pointerValue = Pointer.current != null ? Pointer.current.press.ReadValue() : 0f; bool actionPressed = pressAction != null && pressAction.action.IsPressed(); bool isButtonCurrentlyPressed = mouseValue > 0.5f || pointerValue > 0.5f || actionPressed || rawInputValue > 0.5f; Race Condition The Singleton instance is initialized in Start(). This creates a race condition where other scripts' OnEnable or input callbacks might try to access GameManager.instance before it is assigned. It should be initialized in Awake(). instance = this; Configuration Mutation The public beatTempo field is modified in Start(). This changes the value visible in the Inspector at runtime, making debugging and tweaking difficult. It is better to use a separate private variable for the calculated speed. beatTempo = beatTempo / 60f;

Update review

---

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

[u-pr]

Persistent review updated to latest commit 4c24f95

[u-pr]

PR Code Suggestions ✨

Latest suggestions up to 99d690a

Category	Suggestion	Impact
General	Reset button state unconditionally on release The current logic returns early for short presses (taps), failing to reset _isButtonHeld and the button texture. This causes the button to visually stick in the pressed state. Move the state reset logic before the duration check. Assets/Samples/InputRecorder/Game/ButtonController.cs [136-150] private void ProcessButtonRelease() { + _isButtonHeld = false; + + if (defaultTexture != null) + meshR.material.mainTexture = defaultTexture; + float holdDuration = Time.time - _buttonPressTime; if (holdDuration < MIN_HOLD_DURATION) { return; } - _isButtonHeld = false; - - if (defaultTexture != null) - meshR.material.mainTexture = defaultTexture; - for (int i = notesInLane.Count - 1; i >= 0; i--) Suggestion importance[1-10]: 8 __ Why: The current implementation returns early for short presses (taps), which prevents _isButtonHeld from being reset to false and the texture from reverting to default. This causes a visual bug where the button appears stuck. Moving the state reset before the duration check ensures correct behavior.	Medium
	Use Input Action state instead of hardware polling Directly polling hardware devices (Mouse.current, Pointer.current) bypasses the Input System abstraction and can introduce logic errors or unexpected behavior (e.g., mouse clicks registering as valid input even if not bound). Rely exclusively on the pressAction state. Assets/Samples/InputRecorder/Game/ButtonController.cs [173-186] private void OnTriggerExit(Collider other) { NoteObject note = other.GetComponent<NoteObject>(); if (note != null) { - // Check actual input device state using multiple methods - float rawInputValue = pressAction != null ? pressAction.action.ReadValue<float>() : 0f; - // Read raw values from input devices (works for both keyboard and UI/mouse input) - float mouseValue = Mouse.current != null ? Mouse.current.leftButton.ReadValue() : 0f; - float pointerValue = Pointer.current != null ? Pointer.current.press.ReadValue() : 0f; - bool actionPressed = pressAction != null && pressAction.action.IsPressed(); - bool isButtonCurrentlyPressed = mouseValue > 0.5f || pointerValue > 0.5f || actionPressed || rawInputValue > 0.5f; + bool isButtonCurrentlyPressed = pressAction != null && pressAction.action.IsPressed(); notesInLane.Remove(note); Suggestion importance[1-10]: 7 __ Why: Directly polling Mouse.current or Pointer.current bypasses the Input System abstraction and may allow inputs (like mouse clicks) to trigger actions even if they aren't bound to the pressAction. Relying solely on pressAction ensures consistency with the defined input bindings.	Medium
More suggestions

---

🤖 Helpful? Please react with 👍/👎 | Questions❓Please reach out in Slack #ask-u-pr

---

Previous suggestions

Suggestions up to commit 4c24f95

Category	Suggestion	Impact
Possible issue	Fix infinite loop reset triggering per frame The current logic resets _songLoopResetTriggered immediately inside HandleLoopReset, causing the reset logic to trigger every frame during the loop offset window. Remove the flag reset from HandleLoopReset and instead reset it in Update when timeRemainingInLoop is outside the offset window (indicating the loop has occurred). Assets/Samples/InputRecorder/Game/GameManager.cs [175-196] private void HandleLoopReset() { if (noteScroller != null) noteScroller.LoopResetScroller(); - _songLoopResetTriggered = false; Debug.Log("Song loop detected! Resetting NoteScroller for next loop."); } void Update() { if (startPlaying && musicSource != null && musicSource.clip != null) { float timeRemainingInLoop = musicSource.clip.length - musicSource.time; - if (timeRemainingInLoop <= loopResetOffsetSeconds && !_songLoopResetTriggered) + if (timeRemainingInLoop <= loopResetOffsetSeconds) { - _songLoopResetTriggered = true; - HandleLoopReset(); + if (!_songLoopResetTriggered) + { + _songLoopResetTriggered = true; + HandleLoopReset(); + } + } + else + { + _songLoopResetTriggered = false; } } } Suggestion importance[1-10]: 9 __ Why: The current implementation resets the _songLoopResetTriggered flag immediately inside HandleLoopReset, while the condition in Update remains true for the duration of loopResetOffsetSeconds. This causes HandleLoopReset to be called every frame during that window, effectively locking the NoteScroller in a reset state. The suggestion correctly moves the flag reset logic to Update to occur only when the loop condition is no longer met.	High
	Fix Input System event subscription leak Using a lambda expression for event subscription creates a new delegate instance each time, and OnDisable fails to unsubscribe from the event, leading to duplicate calls if the object is re-enabled or the scene is reloaded (referencing an Input Action Asset). Use a named method for the event handler and properly unsubscribe in OnDisable. Assets/Samples/InputRecorder/Game/ResetBeatTrack.cs [9-24] private void OnEnable() { if (resetAction != null) { - resetAction.action.performed += ctx => gameManager.ResetSong(); + resetAction.action.performed += OnResetPerformed; resetAction.action.Enable(); } } private void OnDisable() { if (resetAction != null) { + resetAction.action.performed -= OnResetPerformed; resetAction.action.Disable(); } } +private void OnResetPerformed(InputAction.CallbackContext ctx) +{ + gameManager.ResetSong(); +} + Suggestion importance[1-10]: 8 __ Why: The code subscribes to an InputAction event using a lambda expression but fails to unsubscribe correctly in OnDisable (since lambdas create new delegate instances). This leads to duplicate event invocations and memory leaks if the InputAction asset persists across object lifecycles or scene reloads. The suggestion correctly replaces the lambda with a named method and proper unsubscription.	Medium
General	Fix incorrect prioritization of Hold notes The break statement causes the logic to select the first hold note found (which is the newest/furthest due to reverse iteration) instead of the closest one. Remove the break and compare distance against smallestDistance to ensure the most accurate note is selected, consistent with Normal note logic. Assets/Samples/InputRecorder/Game/ButtonController.cs [107-115] else if (note.noteType == NoteObject.NoteType.Hold && !note.IsHoldStarted()) { float distance = Mathf.Abs(note.transform.position.x - WorldXPosition); - if (distance <= 0.25f) + if (distance <= 0.25f && distance < smallestDistance) { + smallestDistance = distance; bestNoteToHit = note; - break; } } Suggestion importance[1-10]: 7 __ Why: The loop iterates backwards through the notes list. For Hold notes, the existing code breaks immediately upon finding any valid note within range. This prioritizes the note added last (potentially further away) over the closest note. The suggestion aligns the logic with the Normal note handling by checking for smallestDistance and removing the break statement, ensuring the most accurate note is targeted.	Medium

[codecov-github-com]

Codecov Report

Attention: Patch coverage is 0% with 478 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
Assets/Samples/InputRecorder/Game/GameManager.cs	0.00%	192 Missing ⚠️
...ets/Samples/InputRecorder/Game/ButtonController.cs	0.00%	124 Missing ⚠️
Assets/Samples/InputRecorder/Game/NoteObject.cs	0.00%	106 Missing ⚠️
Assets/Samples/InputRecorder/Game/NoteScroller.cs	0.00%	43 Missing ⚠️
...ssets/Samples/InputRecorder/Game/ResetBeatTrack.cs	0.00%	13 Missing ⚠️

@@             Coverage Diff             @@
##           develop    #2306      +/-   ##
===========================================
- Coverage    77.95%   77.57%   -0.39%     
===========================================
  Files          476      481       +5     
  Lines        97453    97903     +450     
===========================================
- Hits         75971    75948      -23     
- Misses       21482    21955     +473     

Files with missing lines	Coverage Δ
...ssets/Samples/InputRecorder/Game/ResetBeatTrack.cs	0.00% <0.00%> (ø)
Assets/Samples/InputRecorder/Game/NoteScroller.cs	0.00% <0.00%> (ø)
Assets/Samples/InputRecorder/Game/NoteObject.cs	0.00% <0.00%> (ø)
...ets/Samples/InputRecorder/Game/ButtonController.cs	0.00% <0.00%> (ø)
Assets/Samples/InputRecorder/Game/GameManager.cs	0.00% <0.00%> (ø)

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[u-pr]

Persistent review updated to latest commit 99d690a