[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-agent]

PR Reviewer Guide 🔍

(Review updated until commit 4c24f95)

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪ The PR introduces a small sample game; the logic is mostly self-contained and straightforward, though it requires attention to Unity-specific lifecycle and timing details.

🏅 Score: 80 The code is readable and structured well for a sample, but includes a critical bug in the loop handling logic and modifies configuration data at runtime, which affects maintainability.

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Logic Bug In Update, the loop detection logic triggers HandleLoopReset, which immediately resets the _songLoopResetTriggered flag to false. Since the condition timeRemainingInLoop <= loopResetOffsetSeconds remains true for multiple frames (until the audio actually loops), HandleLoopReset will be executed every frame during this duration, causing the scroller to glitch or reset repeatedly. if (timeRemainingInLoop <= loopResetOffsetSeconds && !_songLoopResetTriggered) { _songLoopResetTriggered = true; HandleLoopReset(); } Bad Practice The Start method modifies the public beatTempo field directly. This permanently alters the configuration value (likely BPM) to a speed value, which causes issues if the component is disabled/re-enabled or if the value is tweaked in the Inspector during runtime. Store the calculated speed in a separate private variable. beatTempo = beatTempo / 60f; Code Quality Usage of string-based Invoke and CancelInvoke ("DeactivateNote") is error-prone and unsafe for refactoring. Use nameof(DeactivateNote) or a Coroutine for better type safety and maintainability. CancelInvoke("DeactivateNote"); Invoke("DeactivateNote", displayDelaySeconds);

Update review

---

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

[u-pr-agent]

Persistent review updated to latest commit 4c24f95

[u-pr-agent]

PR Code Suggestions ✨

Explore these optional code suggestions:

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
More suggestions

---

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

[codecov-github-com]

Codecov Report

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

Files with missing lines	Patch %	Lines
Assets/Samples/InputRecorder/Game/GameManager.cs	0.00%	193 Missing ⚠️
...ets/Samples/InputRecorder/Game/ButtonController.cs	0.00%	137 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.55%   -0.40%     
===========================================
  Files          477      481       +4     
  Lines        97419    97935     +516     
===========================================
+ Hits         75943    75955      +12     
- Misses       21476    21980     +504     

Flag	Coverage Δ
inputsystem_MacOS_2022.3	5.54% <ø> (-0.01%)	⬇️
inputsystem_MacOS_2022.3_project	75.04% <0.00%> (-0.46%)	⬇️
inputsystem_MacOS_6000.0	5.32% <ø> (-0.01%)	⬇️
inputsystem_MacOS_6000.0_project	76.93% <0.00%> (-0.49%)	⬇️
inputsystem_MacOS_6000.2	5.32% <ø> (-0.01%)	⬇️
inputsystem_MacOS_6000.2_project	76.93% <0.00%> (-0.49%)	⬇️
inputsystem_MacOS_6000.3	5.32% <ø> (-0.01%)	⬇️
inputsystem_MacOS_6000.3_project	76.93% <0.00%> (-0.49%)	⬇️
inputsystem_MacOS_6000.4	5.32% <ø> (-0.01%)	⬇️
inputsystem_MacOS_6000.4_project	76.93% <0.00%> (-0.44%)	⬇️
inputsystem_MacOS_6000.5	5.32% <ø> (-0.01%)	⬇️
inputsystem_MacOS_6000.5_project	76.93% <0.00%> (-0.49%)	⬇️
inputsystem_Ubuntu_2022.3	5.54% <ø> (-0.01%)	⬇️
inputsystem_Ubuntu_2022.3_project	74.84% <0.00%> (-0.46%)	⬇️
inputsystem_Ubuntu_6000.0	5.32% <ø> (-0.01%)	⬇️
inputsystem_Ubuntu_6000.0_project	76.73% <0.00%> (-0.49%)	⬇️
inputsystem_Ubuntu_6000.2	5.32% <ø> (-0.01%)	⬇️
inputsystem_Ubuntu_6000.2_project	76.73% <0.00%> (-0.49%)	⬇️
inputsystem_Ubuntu_6000.3	5.32% <ø> (-0.01%)	⬇️
inputsystem_Ubuntu_6000.3_project	76.73% <0.00%> (-0.49%)	⬇️
inputsystem_Ubuntu_6000.4	5.33% <ø> (-0.01%)	⬇️
inputsystem_Ubuntu_6000.4_project	76.74% <0.00%> (-0.49%)	⬇️
inputsystem_Ubuntu_6000.5	5.33% <ø> (-0.01%)	⬇️
inputsystem_Ubuntu_6000.5_project	76.74% <0.00%> (-0.49%)	⬇️
inputsystem_Windows_2022.3	5.54% <ø> (-0.01%)	⬇️
inputsystem_Windows_2022.3_project	75.17% <0.00%> (-0.46%)	⬇️
inputsystem_Windows_6000.0	5.32% <ø> (-0.01%)	⬇️
inputsystem_Windows_6000.0_project	77.06% <0.00%> (-0.48%)	⬇️
inputsystem_Windows_6000.2	5.32% <ø> (-0.01%)	⬇️
inputsystem_Windows_6000.2_project	77.05% <0.00%> (-0.49%)	⬇️
inputsystem_Windows_6000.3	5.32% <ø> (-0.01%)	⬇️
inputsystem_Windows_6000.3_project	77.06% <0.00%> (-0.49%)	⬇️
inputsystem_Windows_6000.4	5.32% <ø> (-0.01%)	⬇️
inputsystem_Windows_6000.4_project	77.06% <0.00%> (-0.48%)	⬇️
inputsystem_Windows_6000.5	5.32% <ø> (-0.01%)	⬇️
inputsystem_Windows_6000.5_project	77.06% <0.00%> (-0.49%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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 7 files with indirect coverage changes

🚀 New features to boost your workflow:

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