From 9ff621088fb2c382c938f3976d3de5a2337fea94 Mon Sep 17 00:00:00 2001 From: Kurtis Date: Fri, 25 Sep 2026 14:45:12 -0700 Subject: [PATCH 1/3] Remove CanvasSliderVisuals Update polling, refactor UpdateValue helper, and add tests . --- org.mixedrealitytoolkit.uxcore/CHANGELOG.md | 5 + .../Slider/Slider.cs | 93 +++++--- .../Slider/Visuals/CanvasSliderVisuals.cs | 106 ++++++--- .../Tests/Editor/CanvasSliderVisualsTests.cs | 209 ++++++++++++++++++ .../Editor/CanvasSliderVisualsTests.cs.meta | 11 + .../Editor/MRTK.UXCore.EditorTests.asmdef | 3 + .../CanvasSliderVisualsRuntimeTests.cs | 152 +++++++++++++ .../CanvasSliderVisualsRuntimeTests.cs.meta | 11 + 8 files changed, 519 insertions(+), 71 deletions(-) create mode 100644 org.mixedrealitytoolkit.uxcore/Tests/Editor/CanvasSliderVisualsTests.cs create mode 100644 org.mixedrealitytoolkit.uxcore/Tests/Editor/CanvasSliderVisualsTests.cs.meta create mode 100644 org.mixedrealitytoolkit.uxcore/Tests/Runtime/CanvasSliderVisualsRuntimeTests.cs create mode 100644 org.mixedrealitytoolkit.uxcore/Tests/Runtime/CanvasSliderVisualsRuntimeTests.cs.meta diff --git a/org.mixedrealitytoolkit.uxcore/CHANGELOG.md b/org.mixedrealitytoolkit.uxcore/CHANGELOG.md index b5ac52eb0..34b31f70a 100644 --- a/org.mixedrealitytoolkit.uxcore/CHANGELOG.md +++ b/org.mixedrealitytoolkit.uxcore/CHANGELOG.md @@ -8,11 +8,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). * Added `AlphaBlend` tint mode to `TintEffect`. [PR #1131](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1131) +### Changed + +* Removed redundant `Update()` from `CanvasSliderVisuals` in favor of event-driven updates and `OnValidate()`. [PR #1144](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1144) + ### Fixed * Fixed "leaked managed shell" issue in `UGUIInputAdapter`. [PR #1096](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1096) * Fixed "Attribute 'SerializeField' is not valid on this declaration type. It is only valid on 'field' declarations" error on `DialogButton` in Unity 6.3. [PR #1108](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1108) * Fixed `SeeItSayItLabelEnablerTests` failure when running on platforms without speech recognition support. [PR #1141](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1141) +* Fixed slider handle visuals not updating when slider value is modified from Inspector in play mode. [PR #1144](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1144) ## [3.3.0] - 2025-11-12 diff --git a/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs b/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs index c0bc6b755..593f57625 100644 --- a/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs +++ b/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs @@ -103,14 +103,8 @@ public Collider TrackCollider /// public float MinValue { - get - { - return minValue; - } - set - { - minValue = Mathf.Min(value, maxValue); - } + get => minValue; + set => minValue = Mathf.Min(value, maxValue); } [SerializeField] @@ -122,14 +116,8 @@ public float MinValue /// public float MaxValue { - get - { - return maxValue; - } - set - { - maxValue = Mathf.Max(minValue, value); - } + get => maxValue; + set => maxValue = Mathf.Max(minValue, value); } [VariableRange("minValue", "maxValue")] @@ -149,16 +137,7 @@ public float MaxValue public float Value { get => value; - set - { - if (this.value != value) - { - value = Mathf.Clamp(value, minValue, maxValue); - var oldSliderValue = this.value; - this.value = value; - OnValueUpdated.Invoke(new SliderEventData(oldSliderValue, value)); - } - } + set => UpdateValue(value); } /// @@ -176,7 +155,14 @@ public float Value public bool UseSliderStepDivisions { get => useSliderStepDivisions; - set => useSliderStepDivisions = value; + set + { + useSliderStepDivisions = value; + if (hasStarted && useSliderStepDivisions) + { + UpdateValue(this.value); + } + } } [SerializeField] @@ -190,7 +176,14 @@ public bool UseSliderStepDivisions public int SliderStepDivisions { get => sliderStepDivisions; - set => sliderStepDivisions = value; + set + { + sliderStepDivisions = value; + if (hasStarted && useSliderStepDivisions) + { + UpdateValue(this.value); + } + } } [Header("Layout")] @@ -240,10 +233,13 @@ public Transform SliderEnd /// A Unity event that is invoked when changes. /// public SliderEvent OnValueUpdated => onValueUpdated; + #endregion #region Private Fields + private bool hasStarted; + /// /// Private member used to adjust slider values /// @@ -285,9 +281,11 @@ protected override void Reset() /// /// A Unity event function that is called on the frame when a script is enabled just before any of the update methods are called the first time. - /// + /// protected virtual void Start() { + hasStarted = true; + // Turn on/off colliders at Start() to avoid bugs with // colliders not calling OnTriggerExit on interactors SnapToPosition = snapToPosition; @@ -324,14 +322,15 @@ private void OnValidate() // Ensure that the proper constraints are applied to the possible values of the slider MinValue = minValue; MaxValue = maxValue; - Value = value; + UpdateValue(value, forceUpdate: true); } #endregion #region Protected Methods + /// - /// Invoked on , , and to apply required + /// Invoked on , , and to apply required /// settings to this instance. /// /// @@ -344,22 +343,45 @@ protected virtual void ApplyRequiredSettings() // been acquired. selectMode = InteractableSelectMode.Single; } + #endregion Protected Methods #region Private Methods - /// + + /// /// Private method used to adjust initial slider value to stepwise values /// private void InitializeStepDivisions() { - Value = SnapSliderToStepPositions(Value); + UpdateValue(value); + } + + private float ClampAndSnapValue(float rawValue) + { + float clamped = Mathf.Clamp(rawValue, minValue, maxValue); + if (useSliderStepDivisions) + { + clamped = SnapSliderToStepPositions(clamped); + } + return Mathf.Clamp(clamped, minValue, maxValue); + } + + private void UpdateValue(float newValue, bool forceUpdate = false) + { + float clampedValue = ClampAndSnapValue(newValue); + + if (forceUpdate || this.value != clampedValue) + { + var oldSliderValue = this.value; + this.value = clampedValue; + OnValueUpdated.Invoke(new SliderEventData(oldSliderValue, clampedValue)); + } } private float SnapSliderToStepPositions(float value) { var stepCount = value / SliderStepVal; var snappedValue = SliderStepVal * Mathf.RoundToInt(stepCount); - Mathf.Clamp(snappedValue, 0f, 1.0f); return snappedValue; } @@ -373,8 +395,7 @@ private void UpdateSliderValue() var normalizedStartValue = (StartSliderValue - MinValue) / (MaxValue - MinValue); float normalizedValue = Mathf.Clamp(normalizedStartValue + handDelta / SliderTrackDirection.magnitude, 0f, 1.0f); - var unsnappedValue = normalizedValue * (MaxValue - MinValue) + MinValue; - Value = useSliderStepDivisions ? SnapSliderToStepPositions(unsnappedValue) : unsnappedValue; + UpdateValue(normalizedValue * (MaxValue - MinValue) + MinValue); } #endregion diff --git a/org.mixedrealitytoolkit.uxcore/Slider/Visuals/CanvasSliderVisuals.cs b/org.mixedrealitytoolkit.uxcore/Slider/Visuals/CanvasSliderVisuals.cs index 9eb29634f..ad95a3c8f 100644 --- a/org.mixedrealitytoolkit.uxcore/Slider/Visuals/CanvasSliderVisuals.cs +++ b/org.mixedrealitytoolkit.uxcore/Slider/Visuals/CanvasSliderVisuals.cs @@ -130,9 +130,13 @@ public Direction SliderDirection /// private void OnEnable() { - sliderStart = SliderState.SliderStart.EnsureComponent(); - sliderEnd = SliderState.SliderEnd.EnsureComponent(); - touchableFitter = TrackArea.GetComponent(); + EnsureSliderEndpoints(); + + if (TrackArea != null) + { + touchableFitter = TrackArea.GetComponent(); + } + uguiInputAdapter = GetComponent(); if (sliderStart == null || sliderEnd == null) @@ -140,10 +144,14 @@ private void OnEnable() Debug.LogError("Slider is missing start/end transforms."); } - SliderState.OnValueUpdated.AddListener(UpdateHandle); + if (SliderState != null) + { + SliderState.OnValueUpdated.AddListener(UpdateHandle); + + // Initial update. We may miss the first OnValueUpdated, depending on execution order. + UpdateHandle(SliderState.NormalizedValue); + } - // Initial update. We may miss the first OnValueUpdated, depending on execution order. - UpdateHandle(SliderState.NormalizedValue); SetLayout(sliderDirection); } @@ -156,17 +164,22 @@ private void OnDisable() } #if UNITY_EDITOR - // Keep track of the last direction, so we can re-layout if needed. private Direction prevDirection; /// - /// A Unity event function that is called every frame, if this object is enabled. + /// A Unity Editor only event function that is called when the script is loaded or a value changes in the Unity Inspector. /// - private void Update() + private void OnValidate() { - // Only do this in edit mode, for performance. - if (Application.isPlaying) + if (trackArea == null || handle == null || fillVisual == null) + { + return; + } + + EnsureSliderEndpoints(); + + if (sliderStart == null || sliderEnd == null) { return; } @@ -179,14 +192,17 @@ private void Update() trackArea.anchoredPosition = Vector3.zero; // Helper to apply layout if set from inspector. - if (SliderDirection != prevDirection) + if (sliderDirection != prevDirection) { - SetLayout(SliderDirection); - prevDirection = SliderDirection; + SetLayout(sliderDirection); + prevDirection = sliderDirection; } // Update handle in editor view. - UpdateHandle(SliderState.NormalizedValue); + if (SliderState != null) + { + UpdateHandle(SliderState.NormalizedValue); + } } #endif // UNITY_EDITOR @@ -199,6 +215,11 @@ void UpdateHandle(SliderEventData data) // Update the things that depend on the slider value. void UpdateHandle(float value) { + if (handle == null || fillVisual == null) + { + return; + } + switch (SliderDirection) { case Direction.LeftToRight: @@ -225,17 +246,47 @@ void UpdateHandle(float value) handle.anchoredPosition = Vector3.zero; } + private void EnsureSliderEndpoints() + { + if (sliderStart == null && SliderState != null && SliderState.SliderStart != null) + { + sliderStart = Application.isPlaying + ? SliderState.SliderStart.EnsureComponent() + : SliderState.SliderStart.GetComponent(); + } + + if (sliderEnd == null && SliderState != null && SliderState.SliderEnd != null) + { + sliderEnd = Application.isPlaying + ? SliderState.SliderEnd.EnsureComponent() + : SliderState.SliderEnd.GetComponent(); + } + } + void SetLayout(Direction direction) { + // Ensure sliderStart and sliderEnd are rooted at 0, we change the positions of these points via their anchors + sliderStart.anchoredPosition = Vector3.zero; + sliderEnd.anchoredPosition = Vector3.zero; + + if (uguiInputAdapter == null) + { + uguiInputAdapter = GetComponent(); + } + + // Make sure UGUI understands which axis we can + // slide on. (Affects dpad/gamepad/etc) + if (uguiInputAdapter != null) + { + uguiInputAdapter.MovableAxes = (direction == Direction.LeftToRight || direction == Direction.RightToLeft) + ? AxisFlags.XAxis + : AxisFlags.YAxis; + } + float trackWidth = Mathf.Max(trackArea.sizeDelta.x, trackArea.sizeDelta.y); switch (direction) { case Direction.LeftToRight: - - // Make sure UGUI understands which axis we can - // slide on. (Affects dpad/gamepad/etc) - uguiInputAdapter.MovableAxes = AxisFlags.XAxis; - sliderStart.anchorMin = new Vector2(0.0f, 0.5f); sliderStart.anchorMax = new Vector2(0.0f, 0.5f); sliderEnd.anchorMin = new Vector2(1.0f, 0.5f); @@ -251,11 +302,6 @@ void SetLayout(Direction direction) break; case Direction.RightToLeft: - - // Make sure UGUI understands which axis we can - // slide on. (Affects dpad/gamepad/etc) - uguiInputAdapter.MovableAxes = AxisFlags.XAxis; - sliderStart.anchorMin = new Vector2(1.0f, 0.5f); sliderStart.anchorMax = new Vector2(1.0f, 0.5f); sliderEnd.anchorMin = new Vector2(0.0f, 0.5f); @@ -271,11 +317,6 @@ void SetLayout(Direction direction) break; case Direction.BottomToTop: - - // Make sure UGUI understands which axis we can - // slide on. (Affects dpad/gamepad/etc) - uguiInputAdapter.MovableAxes = AxisFlags.YAxis; - sliderStart.anchorMin = new Vector2(0.5f, 0.0f); sliderStart.anchorMax = new Vector2(0.5f, 0.0f); sliderEnd.anchorMin = new Vector2(0.5f, 1.0f); @@ -291,11 +332,6 @@ void SetLayout(Direction direction) break; case Direction.TopToBottom: - - // Make sure UGUI understands which axis we can - // slide on. (Affects dpad/gamepad/etc) - uguiInputAdapter.MovableAxes = AxisFlags.YAxis; - sliderStart.anchorMin = new Vector2(0.5f, 1); sliderStart.anchorMax = new Vector2(0.5f, 1); sliderEnd.anchorMin = new Vector2(0.5f, 0); diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Editor/CanvasSliderVisualsTests.cs b/org.mixedrealitytoolkit.uxcore/Tests/Editor/CanvasSliderVisualsTests.cs new file mode 100644 index 000000000..baf884879 --- /dev/null +++ b/org.mixedrealitytoolkit.uxcore/Tests/Editor/CanvasSliderVisualsTests.cs @@ -0,0 +1,209 @@ +// Copyright (c) Mixed Reality Toolkit Contributors +// Licensed under the BSD 3-Clause + +// Disable "missing XML comment" warning for tests. While nice to have, this documentation is not required. +#pragma warning disable CS1591 + +using MixedReality.Toolkit.UX; +using NUnit.Framework; +using System.Reflection; +using UnityEditor; +using UnityEngine; + +namespace MixedReality.Toolkit.UX.Tests.EditMode +{ + public class CanvasSliderVisualsTests + { + private GameObject sliderObject; + private Slider slider; + private CanvasSliderVisuals visuals; + private RectTransform sliderStart; + private RectTransform sliderEnd; + private RectTransform trackArea; + private RectTransform handle; + private RectTransform fillVisual; + + [SetUp] + public void Setup() + { + sliderObject = new GameObject("TestSlider", typeof(RectTransform), typeof(Slider)); + sliderObject.SetActive(false); + slider = sliderObject.GetComponent(); + + GameObject startGo = new GameObject("SliderStart", typeof(RectTransform)); + startGo.transform.SetParent(sliderObject.transform, false); + sliderStart = startGo.GetComponent(); + + GameObject endGo = new GameObject("SliderEnd", typeof(RectTransform)); + endGo.transform.SetParent(sliderObject.transform, false); + sliderEnd = endGo.GetComponent(); + + GameObject trackGo = new GameObject("TrackArea", typeof(RectTransform)); + trackGo.transform.SetParent(sliderObject.transform, false); + trackArea = trackGo.GetComponent(); + trackArea.sizeDelta = new Vector2(100f, 10f); + + GameObject fillGo = new GameObject("FillVisual", typeof(RectTransform)); + fillGo.transform.SetParent(trackGo.transform, false); + fillVisual = fillGo.GetComponent(); + + GameObject handleGo = new GameObject("Handle", typeof(RectTransform)); + handleGo.transform.SetParent(trackGo.transform, false); + handle = handleGo.GetComponent(); + + slider.SliderStart = sliderStart; + slider.SliderEnd = sliderEnd; + slider.MinValue = 0f; + slider.MaxValue = 1f; + slider.Value = 0f; + + visuals = sliderObject.AddComponent(); + visuals.TrackArea = trackArea; + visuals.Handle = handle; + visuals.FillVisual = fillVisual; + sliderObject.SetActive(true); + } + + [TearDown] + public void TearDown() + { + if (sliderObject != null) + { + Object.DestroyImmediate(sliderObject); + } + } + + private static void InvokeOnValidate(Object target) + { + MethodInfo onValidate = target.GetType().GetMethod("OnValidate", BindingFlags.Instance | BindingFlags.NonPublic | BindingFlags.Public); + if (onValidate != null) + { + onValidate.Invoke(target, null); + } + } + + [Test] + public void Slider_OnValidate_ClampsAndSnapsValue_AndFiresOnValueUpdated() + { + slider.MinValue = 0f; + slider.MaxValue = 10f; + slider.UseSliderStepDivisions = true; + slider.SliderStepDivisions = 10; + + // Set raw serialized value to out-of-range un-snapped value + SerializedObject so = new SerializedObject(slider); + so.FindProperty("value").floatValue = 12.3f; + so.ApplyModifiedProperties(); + + bool eventFired = false; + float reportedValue = -1f; + slider.OnValueUpdated.AddListener(data => + { + eventFired = true; + reportedValue = data.NewValue; + }); + + InvokeOnValidate(slider); + + Assert.IsTrue(eventFired, "OnValueUpdated should have been fired by Slider.OnValidate()"); + Assert.AreEqual(10f, slider.Value, 0.0001f, "Slider.Value should be clamped to MaxValue"); + Assert.AreEqual(10f, reportedValue, 0.0001f, "SliderEventData should report the clamped value"); + } + + [Test] + public void CanvasSliderVisuals_EditMode_RespondsToSliderValueUpdate() + { + // Because CanvasSliderVisuals is [ExecuteAlways], in EditMode it listens to OnValueUpdated. + slider.Value = 0.75f; + + Assert.AreEqual(0.75f, handle.anchorMin.x, 0.001f, "Handle anchorMin.x should update to reflect 0.75"); + Assert.AreEqual(0.75f, handle.anchorMax.x, 0.001f, "Handle anchorMax.x should update to reflect 0.75"); + Assert.AreEqual(0.75f, fillVisual.anchorMax.x, 0.001f, "FillVisual anchorMax.x should update to reflect 0.75"); + } + + [Test] + public void CanvasSliderVisuals_EditMode_SliderOnValidateSyncsVisuals() + { + // Simulate changing slider value in inspector during Edit Mode + SerializedObject so = new SerializedObject(slider); + so.FindProperty("value").floatValue = 0.4f; + so.ApplyModifiedProperties(); + + InvokeOnValidate(slider); + + Assert.AreEqual(0.4f, handle.anchorMin.x, 0.001f, "Handle anchorMin.x should update when Slider.OnValidate runs"); + Assert.AreEqual(0.4f, fillVisual.anchorMax.x, 0.001f, "FillVisual anchorMax.x should update when Slider.OnValidate runs"); + } + + [Test] + public void CanvasSliderVisuals_SliderDirectionProperty_AppliesLayoutAndHandlePosition() + { + slider.Value = 0.5f; + + // Test RightToLeft + visuals.SliderDirection = CanvasSliderVisuals.Direction.RightToLeft; + Assert.AreEqual(new Vector2(1f, 0.5f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(0f, 0.5f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.x, 0.001f); + + // Test BottomToTop + visuals.SliderDirection = CanvasSliderVisuals.Direction.BottomToTop; + Assert.AreEqual(new Vector2(0.5f, 0f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(0.5f, 1f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.y, 0.001f); + + // Test TopToBottom + visuals.SliderDirection = CanvasSliderVisuals.Direction.TopToBottom; + Assert.AreEqual(new Vector2(0.5f, 1f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(0.5f, 0f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.y, 0.001f); + + // Test LeftToRight + visuals.SliderDirection = CanvasSliderVisuals.Direction.LeftToRight; + Assert.AreEqual(new Vector2(0f, 0.5f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(1f, 0.5f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.x, 0.001f); + } + + [Test] + public void CanvasSliderVisuals_OnValidate_AppliesLayoutAndHandlePosition() + { + slider.Value = 0.5f; + SerializedObject so = new SerializedObject(visuals); + SerializedProperty dirProp = so.FindProperty("sliderDirection"); + + // Test RightToLeft + dirProp.enumValueIndex = (int)CanvasSliderVisuals.Direction.RightToLeft; + so.ApplyModifiedProperties(); + InvokeOnValidate(visuals); + Assert.AreEqual(new Vector2(1f, 0.5f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(0f, 0.5f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.x, 0.001f); + + // Test BottomToTop + dirProp.enumValueIndex = (int)CanvasSliderVisuals.Direction.BottomToTop; + so.ApplyModifiedProperties(); + InvokeOnValidate(visuals); + Assert.AreEqual(new Vector2(0.5f, 0f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(0.5f, 1f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.y, 0.001f); + + // Test TopToBottom + dirProp.enumValueIndex = (int)CanvasSliderVisuals.Direction.TopToBottom; + so.ApplyModifiedProperties(); + InvokeOnValidate(visuals); + Assert.AreEqual(new Vector2(0.5f, 1f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(0.5f, 0f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.y, 0.001f); + + // Test LeftToRight + dirProp.enumValueIndex = (int)CanvasSliderVisuals.Direction.LeftToRight; + so.ApplyModifiedProperties(); + InvokeOnValidate(visuals); + Assert.AreEqual(new Vector2(0f, 0.5f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(1f, 0.5f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.x, 0.001f); + } + } +} +#pragma warning restore CS1591 diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Editor/CanvasSliderVisualsTests.cs.meta b/org.mixedrealitytoolkit.uxcore/Tests/Editor/CanvasSliderVisualsTests.cs.meta new file mode 100644 index 000000000..6c2f08ade --- /dev/null +++ b/org.mixedrealitytoolkit.uxcore/Tests/Editor/CanvasSliderVisualsTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 64e0c647ae5de7c43968c831c636d807 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Editor/MRTK.UXCore.EditorTests.asmdef b/org.mixedrealitytoolkit.uxcore/Tests/Editor/MRTK.UXCore.EditorTests.asmdef index 77be8997b..82c9d0677 100644 --- a/org.mixedrealitytoolkit.uxcore/Tests/Editor/MRTK.UXCore.EditorTests.asmdef +++ b/org.mixedrealitytoolkit.uxcore/Tests/Editor/MRTK.UXCore.EditorTests.asmdef @@ -2,8 +2,11 @@ "name": "MixedReality.Toolkit.UXCore.Editor.Tests", "rootNamespace": "", "references": [ + "MixedReality.Toolkit.Core", "MixedReality.Toolkit.Core.TestUtilities", "MixedReality.Toolkit.UXCore", + "Unity.XR.Interaction.Toolkit", + "Unity.XR.CoreUtils", "UnityEngine.TestRunner", "UnityEditor.TestRunner" ], diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Runtime/CanvasSliderVisualsRuntimeTests.cs b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/CanvasSliderVisualsRuntimeTests.cs new file mode 100644 index 000000000..012f00d87 --- /dev/null +++ b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/CanvasSliderVisualsRuntimeTests.cs @@ -0,0 +1,152 @@ +// Copyright (c) Mixed Reality Toolkit Contributors +// Licensed under the BSD 3-Clause + +// Disable "missing XML comment" warning for tests. While nice to have, this documentation is not required. +#pragma warning disable CS1591 + +using MixedReality.Toolkit.Input.Tests; +using NUnit.Framework; +using System.Collections; +using UnityEngine; +using UnityEngine.TestTools; +using UnityEngine.UI; + +namespace MixedReality.Toolkit.UX.Runtime.Tests +{ + /// + /// PlayMode runtime tests verifying CanvasSliderVisuals and Slider runtime behavior. + /// + public class CanvasSliderVisualsRuntimeTests : BaseRuntimeInputTests + { + private GameObject canvasObject; + private GameObject sliderObject; + private Slider slider; + private CanvasSliderVisuals visuals; + private RectTransform sliderStart; + private RectTransform sliderEnd; + private RectTransform trackArea; + private RectTransform handle; + private RectTransform fillVisual; + + public override IEnumerator Setup() + { + yield return base.Setup(); + + canvasObject = new GameObject("SliderParent", typeof(RectTransform), typeof(Canvas), typeof(CanvasScaler)); + canvasObject.transform.localScale = Vector3.one * 0.001f; + (canvasObject.transform as RectTransform).sizeDelta = Vector2.one * 200; + + sliderObject = new GameObject("Slider", typeof(RectTransform), typeof(Slider)); + sliderObject.SetActive(false); + sliderObject.transform.SetParent(canvasObject.transform, false); + sliderObject.transform.position = Vector3.zero; + (sliderObject.transform as RectTransform).sizeDelta = new Vector2(200f, 10f); + + slider = sliderObject.GetComponent(); + + GameObject startGo = new GameObject("SliderStart", typeof(RectTransform)); + startGo.transform.SetParent(sliderObject.transform, false); + sliderStart = startGo.GetComponent(); + + GameObject endGo = new GameObject("SliderEnd", typeof(RectTransform)); + endGo.transform.SetParent(sliderObject.transform, false); + sliderEnd = endGo.GetComponent(); + + GameObject trackGo = new GameObject("TrackArea", typeof(RectTransform)); + trackGo.transform.SetParent(sliderObject.transform, false); + trackArea = trackGo.GetComponent(); + trackArea.sizeDelta = new Vector2(200f, 10f); + + GameObject fillGo = new GameObject("FillVisual", typeof(RectTransform)); + fillGo.transform.SetParent(trackGo.transform, false); + fillVisual = fillGo.GetComponent(); + + GameObject handleGo = new GameObject("Handle", typeof(RectTransform)); + handleGo.transform.SetParent(trackGo.transform, false); + handle = handleGo.GetComponent(); + + BoxCollider trackCollider = trackGo.AddComponent(); + slider.TrackCollider = trackCollider; + typeof(Slider).GetField("handleTransform", System.Reflection.BindingFlags.Instance | System.Reflection.BindingFlags.NonPublic).SetValue(slider, handle.transform); + slider.SliderStart = sliderStart; + slider.SliderEnd = sliderEnd; + slider.MinValue = 0f; + slider.MaxValue = 1f; + slider.Value = 0f; + + visuals = sliderObject.AddComponent(); + visuals.TrackArea = trackArea; + visuals.Handle = handle; + visuals.FillVisual = fillVisual; + sliderObject.SetActive(true); + + yield return null; + } + + public override IEnumerator TearDown() + { + if (canvasObject != null) + { + Object.Destroy(canvasObject); + } + yield return base.TearDown(); + } + + [UnityTest] + public IEnumerator CanvasSliderVisuals_PlayMode_RespondsToSliderValueUpdate() + { + slider.Value = 0.75f; + yield return null; + + Assert.AreEqual(0.75f, handle.anchorMin.x, 0.001f, "Handle anchorMin.x should update to reflect 0.75 in PlayMode"); + Assert.AreEqual(0.75f, handle.anchorMax.x, 0.001f, "Handle anchorMax.x should update to reflect 0.75 in PlayMode"); + Assert.AreEqual(0.75f, fillVisual.anchorMax.x, 0.001f, "FillVisual anchorMax.x should update to reflect 0.75 in PlayMode"); + } + + [UnityTest] + public IEnumerator CanvasSliderVisuals_PlayMode_StepDivisionsSnapValueAndVisuals() + { + slider.MinValue = 0f; + slider.MaxValue = 10f; + slider.Value = 3.4f; + + slider.SliderStepDivisions = 10; + slider.UseSliderStepDivisions = true; + yield return null; + + Assert.AreEqual(3.0f, slider.Value, 0.0001f, "Slider value should snap to step division when enabled"); + Assert.AreEqual(0.3f, handle.anchorMin.x, 0.001f, "Handle should reflect snapped normalized value"); + } + + [UnityTest] + public IEnumerator CanvasSliderVisuals_PlayMode_SliderDirectionUpdatesLayout() + { + slider.Value = 0.5f; + + visuals.SliderDirection = CanvasSliderVisuals.Direction.RightToLeft; + yield return null; + Assert.AreEqual(new Vector2(1f, 0.5f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(0f, 0.5f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.x, 0.001f); + + visuals.SliderDirection = CanvasSliderVisuals.Direction.BottomToTop; + yield return null; + Assert.AreEqual(new Vector2(0.5f, 0f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(0.5f, 1f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.y, 0.001f); + + visuals.SliderDirection = CanvasSliderVisuals.Direction.TopToBottom; + yield return null; + Assert.AreEqual(new Vector2(0.5f, 1f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(0.5f, 0f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.y, 0.001f); + + visuals.SliderDirection = CanvasSliderVisuals.Direction.LeftToRight; + yield return null; + Assert.AreEqual(new Vector2(0f, 0.5f), sliderStart.anchorMin); + Assert.AreEqual(new Vector2(1f, 0.5f), sliderEnd.anchorMin); + Assert.AreEqual(0.5f, handle.anchorMin.x, 0.001f); + } + } +} +#pragma warning restore CS1591 diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Runtime/CanvasSliderVisualsRuntimeTests.cs.meta b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/CanvasSliderVisualsRuntimeTests.cs.meta new file mode 100644 index 000000000..04cbb43c6 --- /dev/null +++ b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/CanvasSliderVisualsRuntimeTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 2ba491c532904adb8e973b27786a82a7 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: From 0471ca2dbbf9808537d7d3244f0b6c6dafc51358 Mon Sep 17 00:00:00 2001 From: Kurtis Date: Fri, 16 Oct 2026 13:31:20 -0700 Subject: [PATCH 2/3] Add additional tests --- org.mixedrealitytoolkit.uxcore/CHANGELOG.md | 4 + .../Slider/Slider.cs | 53 +++++-- .../Slider/SliderSounds.cs | 29 ++-- .../Tests/Runtime/SliderSoundsTests.cs | 82 +++++++++++ .../Tests/Runtime/SliderSoundsTests.cs.meta | 11 ++ .../Tests/Runtime/SliderTests.cs | 133 ++++++++++++++++++ .../Tests/Runtime/SliderTests.cs.meta | 11 ++ 7 files changed, 301 insertions(+), 22 deletions(-) create mode 100644 org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderSoundsTests.cs create mode 100644 org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderSoundsTests.cs.meta create mode 100644 org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs create mode 100644 org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs.meta diff --git a/org.mixedrealitytoolkit.uxcore/CHANGELOG.md b/org.mixedrealitytoolkit.uxcore/CHANGELOG.md index 34b31f70a..b81281358 100644 --- a/org.mixedrealitytoolkit.uxcore/CHANGELOG.md +++ b/org.mixedrealitytoolkit.uxcore/CHANGELOG.md @@ -18,6 +18,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). * Fixed "Attribute 'SerializeField' is not valid on this declaration type. It is only valid on 'field' declarations" error on `DialogButton` in Unity 6.3. [PR #1108](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1108) * Fixed `SeeItSayItLabelEnablerTests` failure when running on platforms without speech recognition support. [PR #1141](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1141) * Fixed slider handle visuals not updating when slider value is modified from Inspector in play mode. [PR #1144](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1144) +* Guarded against division by zero and zero-length tracks in `Slider`. [PR #1144](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1144) +* Fixed `Slider.SnapSliderToStepPositions` snapping relative to 0 instead of `MinValue` when using custom ranges. [PR #1144](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1144) +* Fixed `SliderSounds` pitch calculation to normalize against slider range instead of passing raw unnormalized values. [PR #1144](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1144) +* Guarded against division by zero and null references in `SliderSounds`. [PR #1144](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1144) ## [3.3.0] - 2025-11-12 diff --git a/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs b/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs index 593f57625..abf6bb96e 100644 --- a/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs +++ b/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs @@ -143,7 +143,14 @@ public float Value /// /// The slider's normalized value calculated using the and properties. /// - public float NormalizedValue => (MaxValue - MinValue) != 0 ? (value - MinValue) / (MaxValue - MinValue) : 0; + public float NormalizedValue + { + get + { + float range = MaxValue - MinValue; + return range > ZeroTolerance ? (value - MinValue) / range : 0f; + } + } [SerializeField] [Tooltip("Controls whether this slider is increments in steps or continuously.")] @@ -238,12 +245,22 @@ public Transform SliderEnd #region Private Fields + private const float ZeroTolerance = 1e-6f; + private bool hasStarted; /// /// Private member used to adjust slider values /// - private float SliderStepVal => (MaxValue - MinValue) / sliderStepDivisions; + private float SliderStepVal + { + get + { + int divisions = Mathf.Max(1, sliderStepDivisions); + float range = MaxValue - MinValue; + return range > ZeroTolerance ? range / divisions : 0f; + } + } #endregion @@ -378,24 +395,38 @@ private void UpdateValue(float newValue, bool forceUpdate = false) } } - private float SnapSliderToStepPositions(float value) + internal float SnapSliderToStepPositions(float value) { - var stepCount = value / SliderStepVal; - var snappedValue = SliderStepVal * Mathf.RoundToInt(stepCount); - return snappedValue; + float stepVal = SliderStepVal; + if (stepVal <= ZeroTolerance) + { + return value; + } + + var stepCount = (value - MinValue) / stepVal; + var snappedValue = MinValue + stepVal * Mathf.Round(stepCount); + return Mathf.Clamp(snappedValue, MinValue, MaxValue); } - private void UpdateSliderValue() + internal void UpdateSliderValue() { + if (interactorsSelecting.Count == 0) + { + return; + } + Vector3 interactionPoint = interactorsSelecting[0].GetAttachTransform(this).position; Vector3 interactorDelta = interactionPoint - StartInteractionPoint; + Vector3 trackDir = SliderTrackDirection; + float sqrMag = trackDir.sqrMagnitude; - var handDelta = Vector3.Dot(SliderTrackDirection.normalized, interactorDelta); + float handDelta = sqrMag > ZeroTolerance ? Vector3.Dot(trackDir, interactorDelta) / sqrMag : 0f; - var normalizedStartValue = (StartSliderValue - MinValue) / (MaxValue - MinValue); - float normalizedValue = Mathf.Clamp(normalizedStartValue + handDelta / SliderTrackDirection.magnitude, 0f, 1.0f); + float range = MaxValue - MinValue; + float normalizedStartValue = range > ZeroTolerance ? (StartSliderValue - MinValue) / range : 0f; + float normalizedValue = Mathf.Clamp01(normalizedStartValue + handDelta); - UpdateValue(normalizedValue * (MaxValue - MinValue) + MinValue); + UpdateValue(normalizedValue * range + MinValue); } #endregion diff --git a/org.mixedrealitytoolkit.uxcore/Slider/SliderSounds.cs b/org.mixedrealitytoolkit.uxcore/Slider/SliderSounds.cs index fad4b03ec..90bfc56c0 100644 --- a/org.mixedrealitytoolkit.uxcore/Slider/SliderSounds.cs +++ b/org.mixedrealitytoolkit.uxcore/Slider/SliderSounds.cs @@ -140,31 +140,37 @@ private void Start() audioSource = gameObject.EnsureComponent(); } - if (alignWithStepSlider && slider.UseSliderStepDivisions) + if (alignWithStepSlider && slider != null && slider.UseSliderStepDivisions) { - tickEvery = 1.0f / slider.SliderStepDivisions; + int divisions = Mathf.Max(1, slider.SliderStepDivisions); + tickEvery = 1.0f / divisions; } } private void OnEnable() { - slider.firstSelectEntered.AddListener(OnInteractionStarted); - slider.lastSelectExited.AddListener(OnInteractionEnded); - slider.OnValueUpdated.AddListener(OnValueUpdated); + if (slider != null) + { + slider.firstSelectEntered.AddListener(OnInteractionStarted); + slider.lastSelectExited.AddListener(OnInteractionEnded); + slider.OnValueUpdated.AddListener(OnValueUpdated); + } } private void OnDisable() { - slider.firstSelectEntered.RemoveListener(OnInteractionStarted); - slider.lastSelectExited.RemoveListener(OnInteractionEnded); - slider.OnValueUpdated.RemoveListener(OnValueUpdated); + if (slider != null) + { + slider.firstSelectEntered.RemoveListener(OnInteractionStarted); + slider.lastSelectExited.RemoveListener(OnInteractionEnded); + slider.OnValueUpdated.RemoveListener(OnValueUpdated); + } } private const float NeutralPitchShift = 1f; - private void OnValueUpdated(SliderEventData eventData) + internal void OnValueUpdated(SliderEventData eventData) { - if (!(playSoundsOnlyOnInteract && !isInteracting) && playTickSounds && AudioSource != null && passNotchSound != null) { float delta = eventData.NewValue - eventData.OldValue; @@ -172,7 +178,8 @@ private void OnValueUpdated(SliderEventData eventData) var now = Time.timeSinceLevelLoad; if (accumulatedDeltaSliderValue >= tickEvery && now - lastSoundPlayTime > minSecondsBetweenTicks) { - AudioSource.pitch = Mathf.Lerp(startPitch, endPitch, eventData.NewValue); + float normalizedProgress = slider != null ? slider.NormalizedValue : eventData.NewValue; + AudioSource.pitch = Mathf.Lerp(startPitch, endPitch, normalizedProgress); if (AudioSource.isActiveAndEnabled) { AudioSource.PlayOneShot(passNotchSound); diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderSoundsTests.cs b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderSoundsTests.cs new file mode 100644 index 000000000..85b5ddb3b --- /dev/null +++ b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderSoundsTests.cs @@ -0,0 +1,82 @@ +// Copyright (c) Mixed Reality Toolkit Contributors +// Licensed under the BSD 3-Clause + +// Disable "missing XML comment" warning for tests. While nice to have, this documentation is not required. +#pragma warning disable CS1591 + +using MixedReality.Toolkit.Core.Tests; +using NUnit.Framework; +using System.Collections; +using UnityEngine; +using UnityEngine.TestTools; +using Object = UnityEngine.Object; + +namespace MixedReality.Toolkit.UX.Runtime.Tests +{ + /// + /// Behavioral and regression tests for the component. + /// + public class SliderSoundsTests : BaseRuntimeTests + { + [UnityTest] + public IEnumerator SliderSounds_CustomRange_NormalizedPitchAccurate() + { + GameObject go = new GameObject("SliderWithSounds"); + Slider slider = go.AddComponent(); + slider.MinValue = 0f; + slider.MaxValue = 100f; + slider.Value = 0f; + + AudioSource audioSource = go.AddComponent(); + SliderSounds sounds = go.AddComponent(); + sounds.AudioSource = audioSource; + sounds.StartPitch = 0.5f; + sounds.EndPitch = 1.5f; + sounds.PlaySoundsOnlyOnInteract = false; + sounds.PlayTickSounds = true; + sounds.TickEvery = 0.01f; + sounds.MinSecondsBetweenTicks = 0f; + + // Create a 1-sample dummy AudioClip so AudioSource has something to play + AudioClip clip = AudioClip.Create("TestClick", 1, 1, 44100, false); + sounds.PassNotchSound = clip; + + // Trigger value update to 50% (value = 50 in a 0..100 range) + slider.Value = 50f; + + // In baseline, audioSource.pitch was set to Mathf.Lerp(0.5f, 1.5f, 50f) = 50.5f (wildly distorted!) + // With fix, it should map normalized value 0.5f -> pitch 1.0f. + Assert.AreEqual(1.0f, audioSource.pitch, 0.05f, "AudioSource pitch must normalize slider range instead of passing raw value to Lerp."); + + Object.Destroy(clip); + Object.Destroy(go); + yield return null; + } + + [UnityTest] + public IEnumerator SliderSounds_StepDivisionSafety_DoesNotThrow() + { + GameObject go = new GameObject("SliderWithStepSounds"); + Slider slider = go.AddComponent(); + slider.MinValue = 0f; + slider.MaxValue = 10f; + slider.SliderStepDivisions = 0; + slider.UseSliderStepDivisions = false; + + SliderSounds sounds = go.AddComponent(); + sounds.AudioSource = go.AddComponent(); + sounds.PlayTickSounds = true; + sounds.TickEvery = 0f; + + // Verifies no division by zero when TickEvery or step divisions are zero + Assert.DoesNotThrow(() => + { + sounds.OnValueUpdated(new SliderEventData(0f, 5f)); + }); + + Object.Destroy(go); + yield return null; + } + } +} +#pragma warning restore CS1591 diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderSoundsTests.cs.meta b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderSoundsTests.cs.meta new file mode 100644 index 000000000..8e894e664 --- /dev/null +++ b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderSoundsTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: a101d300dd834d3180547c907ee55cb7 +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs new file mode 100644 index 000000000..573157252 --- /dev/null +++ b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs @@ -0,0 +1,133 @@ +// Copyright (c) Mixed Reality Toolkit Contributors +// Licensed under the BSD 3-Clause + +// Disable "missing XML comment" warning for tests. While nice to have, this documentation is not required. +#pragma warning disable CS1591 + +using MixedReality.Toolkit.Core.Tests; +using NUnit.Framework; +using System.Collections; +using UnityEngine; +using UnityEngine.TestTools; +using Object = UnityEngine.Object; + +namespace MixedReality.Toolkit.UX.Runtime.Tests +{ + /// + /// Behavioral and regression tests for the component. + /// + public class SliderTests : BaseRuntimeTests + { + [UnityTest] + public IEnumerator Slider_ValueChange_DeliversEventsAndUpdatesValue() + { + GameObject go = new GameObject("SliderTest"); + Slider slider = go.AddComponent(); + slider.MinValue = 0f; + slider.MaxValue = 100f; + slider.Value = 10f; + + float receivedOld = -1f; + float receivedNew = -1f; + int callCount = 0; + + slider.OnValueUpdated.AddListener((SliderEventData data) => + { + receivedOld = data.OldValue; + receivedNew = data.NewValue; + callCount++; + }); + + slider.Value = 25f; + Assert.AreEqual(10f, receivedOld, 1e-4f); + Assert.AreEqual(25f, receivedNew, 1e-4f); + Assert.AreEqual(1, callCount); + Assert.AreEqual(0.25f, slider.NormalizedValue, 1e-4f); + + // Test clamping + slider.Value = 150f; + Assert.AreEqual(100f, slider.Value, 1e-4f); + Assert.AreEqual(1.0f, slider.NormalizedValue, 1e-4f); + + slider.Value = -50f; + Assert.AreEqual(0f, slider.Value, 1e-4f); + Assert.AreEqual(0.0f, slider.NormalizedValue, 1e-4f); + + Object.Destroy(go); + yield return null; + } + + [UnityTest] + public IEnumerator Slider_MathAndDivisionSafety_DoesNotNaNOrThrow() + { + GameObject go = new GameObject("DegenerateSlider"); + Slider slider = go.AddComponent(); + + // Degenerate case 1: MinValue == MaxValue + slider.MaxValue = 5f; + slider.MinValue = 5f; + slider.Value = 5f; + + Assert.IsFalse(float.IsNaN(slider.NormalizedValue), "NormalizedValue must not be NaN when MinValue == MaxValue."); + Assert.AreEqual(0f, slider.NormalizedValue, 1e-4f); + + // Degenerate case 2: Track endpoints coincide (zero track length) + GameObject startObj = new GameObject("Start"); + startObj.transform.SetParent(go.transform); + startObj.transform.position = Vector3.zero; + + GameObject endObj = new GameObject("End"); + endObj.transform.SetParent(go.transform); + endObj.transform.position = Vector3.zero; + + slider.SliderStart = startObj.transform; + slider.SliderEnd = endObj.transform; + + Assert.AreEqual(0f, slider.SliderTrackDirection.sqrMagnitude, 1e-6f); + + // Trigger internal UpdateSliderValue directly + Assert.DoesNotThrow(() => + { + slider.UpdateSliderValue(); + }, "UpdateSliderValue must safely handle zero-length tracks without division-by-zero or throwing."); + + Assert.IsFalse(float.IsNaN(slider.Value), "Slider.Value must remain non-NaN after zero-length track evaluation."); + + // Degenerate case 3: sliderStepDivisions <= 0 + slider.SliderStepDivisions = 0; + slider.UseSliderStepDivisions = true; + float snapped = slider.SnapSliderToStepPositions(2.5f); + Assert.IsFalse(float.IsNaN(snapped), "SnapSliderToStepPositions must not return NaN when step divisions are 0."); + + Object.Destroy(go); + yield return null; + } + + [UnityTest] + public IEnumerator Slider_StepPositions_SnapsRelativeToCustomRange() + { + GameObject go = new GameObject("StepSliderTest"); + Slider slider = go.AddComponent(); + slider.MaxValue = 5f; + slider.MinValue = 1f; + slider.SliderStepDivisions = 2; // Steps: 1, 3, 5 (stepVal = 2) + slider.UseSliderStepDivisions = true; + + // Value at 1.2f should snap to 1.0f + slider.Value = 1.2f; + Assert.AreEqual(1.0f, slider.Value, 1e-4f); + + // Value at 2.4f should snap to 3.0f + slider.Value = 2.4f; + Assert.AreEqual(3.0f, slider.Value, 1e-4f); + + // Value at 4.6f should snap to 5.0f + slider.Value = 4.6f; + Assert.AreEqual(5.0f, slider.Value, 1e-4f); + + Object.Destroy(go); + yield return null; + } + } +} +#pragma warning restore CS1591 diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs.meta b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs.meta new file mode 100644 index 000000000..f385d2f91 --- /dev/null +++ b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs.meta @@ -0,0 +1,11 @@ +fileFormatVersion: 2 +guid: 6ea1b2791d8b4823b3e9b82ba738277c +MonoImporter: + externalObjects: {} + serializedVersion: 2 + defaultReferences: [] + executionOrder: 0 + icon: {instanceID: 0} + userData: + assetBundleName: + assetBundleVariant: From ba23d52afccbf4f311a58cc1e205254c7c4c8bbd Mon Sep 17 00:00:00 2001 From: Kurtis Date: Fri, 16 Oct 2026 13:50:43 -0700 Subject: [PATCH 3/3] Remove allocations across the slider hot paths --- org.mixedrealitytoolkit.uxcore/CHANGELOG.md | 1 + .../Slider/Slider.cs | 8 ++-- .../Slider/SliderEventData.cs | 9 ++++ .../Slider/SliderSounds.cs | 25 +++++++---- .../Tests/Runtime/SliderTests.cs | 43 +++++++++++++++++++ 5 files changed, 75 insertions(+), 11 deletions(-) diff --git a/org.mixedrealitytoolkit.uxcore/CHANGELOG.md b/org.mixedrealitytoolkit.uxcore/CHANGELOG.md index b81281358..511f027af 100644 --- a/org.mixedrealitytoolkit.uxcore/CHANGELOG.md +++ b/org.mixedrealitytoolkit.uxcore/CHANGELOG.md @@ -11,6 +11,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Changed * Removed redundant `Update()` from `CanvasSliderVisuals` in favor of event-driven updates and `OnValidate()`. [PR #1144](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1144) +* Eliminated heap allocations across `Slider` and `SliderSounds` event delivery hot paths. [PR #1144](https://github.com/MixedRealityToolkit/MixedRealityToolkit-Unity/pull/1144) ### Fixed diff --git a/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs b/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs index abf6bb96e..f7d376ee3 100644 --- a/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs +++ b/org.mixedrealitytoolkit.uxcore/Slider/Slider.cs @@ -246,7 +246,7 @@ public Transform SliderEnd #region Private Fields private const float ZeroTolerance = 1e-6f; - + private readonly SliderEventData sliderEventData = new SliderEventData(0f, 0f); private bool hasStarted; /// @@ -326,7 +326,8 @@ protected virtual void Start() InitializeStepDivisions(); } - OnValueUpdated.Invoke(new SliderEventData(value, value)); + sliderEventData.Initialize(value, value); + OnValueUpdated.Invoke(sliderEventData); } /// @@ -391,7 +392,8 @@ private void UpdateValue(float newValue, bool forceUpdate = false) { var oldSliderValue = this.value; this.value = clampedValue; - OnValueUpdated.Invoke(new SliderEventData(oldSliderValue, clampedValue)); + sliderEventData.Initialize(oldSliderValue, clampedValue); + OnValueUpdated.Invoke(sliderEventData); } } diff --git a/org.mixedrealitytoolkit.uxcore/Slider/SliderEventData.cs b/org.mixedrealitytoolkit.uxcore/Slider/SliderEventData.cs index d1ae0412b..fa77ef6a7 100644 --- a/org.mixedrealitytoolkit.uxcore/Slider/SliderEventData.cs +++ b/org.mixedrealitytoolkit.uxcore/Slider/SliderEventData.cs @@ -28,5 +28,14 @@ public SliderEventData(float oldValue, float newValue) /// The current value of the slider. /// public float NewValue { get; private set; } + + /// + /// Reinitializes this instance with updated values to avoid GC allocations in hot interaction paths. + /// + internal void Initialize(float oldValue, float newValue) + { + OldValue = oldValue; + NewValue = newValue; + } } } diff --git a/org.mixedrealitytoolkit.uxcore/Slider/SliderSounds.cs b/org.mixedrealitytoolkit.uxcore/Slider/SliderSounds.cs index 90bfc56c0..1696a40ec 100644 --- a/org.mixedrealitytoolkit.uxcore/Slider/SliderSounds.cs +++ b/org.mixedrealitytoolkit.uxcore/Slider/SliderSounds.cs @@ -22,7 +22,6 @@ public class SliderSounds : MonoBehaviour /// public bool PlaySoundsOnlyOnInteract { get => playSoundsOnlyOnInteract; set => playSoundsOnlyOnInteract = value; } - [Header("Audio Clips")] [SerializeField] [Tooltip("Sound to play when interaction with slider starts")] @@ -117,6 +116,7 @@ public class SliderSounds : MonoBehaviour public AudioSource AudioSource { get => audioSource; set => audioSource = value; } #region Private members + private Slider slider; // Check to see if the slider is being interacted with @@ -125,11 +125,20 @@ public class SliderSounds : MonoBehaviour // Play sound when passing through slider notches private float accumulatedDeltaSliderValue = 0; private float lastSoundPlayTime; + + // Cached delegates to prevent GC allocations during enable/disable cycles + private UnityEngine.Events.UnityAction onInteractionStartedAction; + private UnityEngine.Events.UnityAction onInteractionEndedAction; + private UnityEngine.Events.UnityAction onValueUpdatedAction; + #endregion private void Awake() { slider = GetComponent(); + onInteractionStartedAction = OnInteractionStarted; + onInteractionEndedAction = OnInteractionEnded; + onValueUpdatedAction = OnValueUpdated; } private void Start() @@ -151,9 +160,9 @@ private void OnEnable() { if (slider != null) { - slider.firstSelectEntered.AddListener(OnInteractionStarted); - slider.lastSelectExited.AddListener(OnInteractionEnded); - slider.OnValueUpdated.AddListener(OnValueUpdated); + slider.firstSelectEntered.AddListener(onInteractionStartedAction); + slider.lastSelectExited.AddListener(onInteractionEndedAction); + slider.OnValueUpdated.AddListener(onValueUpdatedAction); } } @@ -161,14 +170,14 @@ private void OnDisable() { if (slider != null) { - slider.firstSelectEntered.RemoveListener(OnInteractionStarted); - slider.lastSelectExited.RemoveListener(OnInteractionEnded); - slider.OnValueUpdated.RemoveListener(OnValueUpdated); + slider.firstSelectEntered.RemoveListener(onInteractionStartedAction); + slider.lastSelectExited.RemoveListener(onInteractionEndedAction); + slider.OnValueUpdated.RemoveListener(onValueUpdatedAction); } } private const float NeutralPitchShift = 1f; - + internal void OnValueUpdated(SliderEventData eventData) { if (!(playSoundsOnlyOnInteract && !isInteracting) && playTickSounds && AudioSource != null && passNotchSound != null) diff --git a/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs index 573157252..32f4c0c98 100644 --- a/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs +++ b/org.mixedrealitytoolkit.uxcore/Tests/Runtime/SliderTests.cs @@ -7,8 +7,12 @@ using MixedReality.Toolkit.Core.Tests; using NUnit.Framework; using System.Collections; +using System.Diagnostics; using UnityEngine; using UnityEngine.TestTools; +using UnityEngine.TestTools.Constraints; +using Debug = UnityEngine.Debug; +using Is = UnityEngine.TestTools.Constraints.Is; using Object = UnityEngine.Object; namespace MixedReality.Toolkit.UX.Runtime.Tests @@ -128,6 +132,45 @@ public IEnumerator Slider_StepPositions_SnapsRelativeToCustomRange() Object.Destroy(go); yield return null; } + + [UnityTest] + [Category("Performance")] + public IEnumerator Slider_ValueChange_AllocatesZeroGC() + { + const int BenchmarkIterations = 1000; + GameObject go = new GameObject("SliderPerf"); + Slider slider = go.AddComponent(); + slider.MinValue = 0f; + slider.MaxValue = 100f; + slider.Value = 10f; + + slider.OnValueUpdated.AddListener((SliderEventData data) => { }); + + // Warm up + slider.Value = 20f; + + // 1. Throughput measurement + Stopwatch sw = Stopwatch.StartNew(); + for (int i = 0; i < BenchmarkIterations; i++) + { + slider.Value = 20f + (i * 0.05f); + } + sw.Stop(); + double usPerCall = (sw.Elapsed.TotalMilliseconds * 1000.0) / BenchmarkIterations; + Debug.Log($"[Perf] Slider.Value change: {usPerCall:F4} µs/call across {BenchmarkIterations} iterations"); + + // 2. Unity Native Zero-GC assertion + Assert.That(() => + { + for (int i = 0; i < 50; i++) + { + slider.Value = 20f + (i * 0.5f); + } + }, Is.Not.AllocatingGCMemory()); + + Object.Destroy(go); + yield return null; + } } } #pragma warning restore CS1591