BlendshapeSync RemapCurve: fix for RemapCurve in between original curve value and consistency and UI improvements

[anatawa12]

Description

This PR fixes a bug that RemapCurve does not map curve as expected.

There are two sub-cases for this bug:

• If an animation curve has a keypoint with exactly the same value as a keypoint on the RemapCurve, its left and right tangents will be averaged. We should calculate the left and right tangents separately.
• If there is no keypoint corresponds to the keypoint on the RemapCurve, we should add KeyPoint to change angle of curve.

I think both of them is significant because both can affect merging two blendshapes into one like BreastBig/Small on body to BreastSize on outfit and splitting single BlendShapes to multiple like BreastSize to BreastBig/Small blendshape.

This PR also introduces several UI and consistency improvmeents:

• Provide rect for Animation Curve editor to deny adding keyframes to out of range. This also helps users to understand RemapCurve uses 0...1 instead of 0...100
• Deny non-linear curves in the RemapCurve. supporting bezier curves in RemapCurve would require >3d bezier curve support in animation curve so I added small logic to normalize curve to not have non-linear segements.

Screenshots

on master and the remap curve on the left

With commit 236d0f9

With commit 1032426

Commit description

• 9ff036c: UI: denies adding keyframe at non (0,1) region
• b354ede: consistency: Deny non-linear curves in the RemapCurve.
• efa3229: make curve always have key at 0 and 1. Assuming those to have would help us to implement
• 7982925: refactor: for future testing.
• 3b73b78: refactor: mapping logic can be complex with full curve support so perf improvement for general case
• 236d0f9: fixes half of the problem

[bdunderscore]

Hmm - I was under the impression that the current logic would handle nonlinear curves by computing the numerical derivative at the sampled point(s) and deriving the tangent accordingly. This does break with split tangents, though, as you note. But I'd be okay with limiting this to linear curves for the initial release.

[bdunderscore]

Do we need to SetKeyBroken here? Should we inherit the broken-key state of the original curve?

[anatawa12]

SetKeyBroken(true) is Inherited from menu operation.
As far as i know, linear tangent mode will never become Broken = false. Setting curve linear will make Broken = true and any operation sets Broken = false will change both tangent mode.
Additionally, linear tangent is purely determined by sibling key so it will never be linked (non-borken).
To prevent bugs in Unity Editor or other tools, I think we should not produce state that is not possible to produce with unity editor.

https://github.com/Unity-Technologies/UnityCsReference/blob/d253e09acffc9803eb9d7cd77f4768d38860d66e/Editor/Mono/Animation/CurveEditor/CurveMenuManager.cs#L312-L317

[Copilot]

Pull request overview

This PR targets Modular Avatar’s Blendshape Sync remap-curve workflow by improving how remap curves are edited in the inspector and by refining the animation-curve remapping logic so it can better preserve curve shape (including tangent behavior) when applying a remap.

Changes:

• Introduces a custom curve attribute + property drawer to constrain the curve editor to a 0..1 domain.
• Normalizes RemapCurve in OnValidate to enforce linear tangents and ensure keys at time 0 and 1.
• Refactors remap logic into MapCurve(...) and updates tangent scaling to use separate left/right slope estimates.

Reviewed changes

Copilot reviewed 4 out of 5 changed files in this pull request and generated 6 comments.

Show a summary per file

File	Description
Runtime/UI/CurveAttribute.cs.meta	Adds Unity metadata for the new runtime attribute script.
Runtime/UI/CurveAttribute.cs	Adds a PropertyAttribute used to pass curve editor bounds to the inspector.
Runtime/ModularAvatarBlendshapeSync.cs	Applies the curve attribute to RemapCurve and enforces linear/key endpoint normalization in OnValidate.
Editor/Inspector/InspectorCommon.cs	Adds a CustomPropertyDrawer to render curves with bounded rect input.
Editor/BlendshapeSyncAnimationProcessor.cs	Refactors curve remapping into MapCurve and updates tangent/value remapping behavior.

Files not reviewed (1)

• Runtime/UI/CurveAttribute.cs.meta: Generated file

[anatawa12]

incorrect. the left/right tangent refers value space mathematical left / right slope. we should use left slope for decreasing tangent and right slope for increasing tangent.

[anatawa12]

@bdunderscore where should I place attributes like this and corresponding editor class? I could not find attributes in modular avatar runtime assembly so I added to UI directory since it's inspector user interface related but just Runtime/ or another directory can also be good I think.

[anatawa12]

I have implemented support for splitting basic bezier / hermite curve.

We still need to support other tangent modes, constant curve with infinite tangent, and splitting bezier curve exactly at the place tangent become infinite (current code converts to constant curve).

Refactoring and writing tests are also in progress in my local.

[anatawa12]

I failed manually implementing ClampAuto mode so I have changed the way we handle tangent mode to converting curves to free curves before processing instead of manually computing tangent in our code.
Constant curve (curves with infinite tangent) and splitting at vertical are also now supported.

[anatawa12]

I have added the tests for RemapCurve and as far as I tried it looks working well so I think this is ready for review & merge with one important spec discussion proposed.

I hope most of important decision in code are written in comment, but I'm sorry if it's too poor.

[anatawa12]

I have added fuzz testing but I found the test is flaky because of low precision value returned from AnimationCurve.Evaluate so I disabled this test by default.
(In addition 100 * 100 matrix is too huge so when we enable the test we have to reduce number of cases)
It looks the precision problem is came from t-axis bezier processing so limiting fuzz testing to hermite curve might improve flakyness though.

For example, test case TestRemapCurveIsCorrect(Random Curve (seed: 1969480910, 10),Fuzzing Curve (seed: 1969480910, 8) and time = 34.05029,
mappedCurve.Evaluate returned 32.74703 but it will be 33.9950927670292 when we compute the value with BezierSegment. about 1.248062767 of error.
animationCurve.Evaluate(time) returned 10.96229 but 10.8068297173427 with BezierSegment and results remapped valut to 34.44767 with Evaluate but 33.9950981140137.

modified test code with `BezierSegment`

        [Test]
        [Combinatorial]
        public void TestRemapCurveIsCorrect(
            [ValueSource(nameof(TestCurves))] AnimationCurveCase aniCurveCase,
            [ValueSource(nameof(TestRemapCases))] AnimationCurveCase remapCase
        )
        {
            // This tests that remap is working as expect by comparing remapped curve and remap
            var animationCurve = aniCurveCase.Curve;
            var remapCurve = new RemapCurve(remapCase.Curve);
            var mappedCurve = BlendshapeSyncAnimationProcessor.MapCurve(animationCurve, remapCurve);
            Assert.That(remapCurve.IsIdentity, Is.False);

            var endTime = mappedCurve[mappedCurve.length - 1].time;

            for (float time = 0; time <= endTime; time += 0.01f)
            {
                var original = animationCurve.Evaluate(time);
                var expectValue = remapCurve.GetPointOnCurve(original).MappedValue;
                var mappedValue = mappedCurve.Evaluate(time);

                // we want to allow +/- 1% of error for value (in 0-100) and time (in seconds)
                // 1% in value 0-100 is 1.
                // 1% in time axis is 0.01 sec, and may result in error for 0.01 sec * tangent in value axis 
                var keyRange = Enumerable.Range(0, mappedCurve.length - 1).TakeWhile(i => mappedCurve[i].time <= time).Last();
                var maxTangent = Mathf.Max(Mathf.Abs(mappedCurve[keyRange].outTangent), Mathf.Abs(mappedCurve[keyRange + 1].inTangent));
                var allowedError = Mathf.Max(Mathf.Max(Mathf.Abs(expectValue) / 100, 1), maxTangent / 100);

                double original2;
                double expect2;
                {
                    var keyRange1 = Enumerable.Range(0, animationCurve.length - 1).TakeWhile(i => animationCurve[i].time <= time).Last();
                    var startKey = animationCurve[keyRange1];
                    var endKey = animationCurve[keyRange1 + 1];

                    const float oneThird = 1.0f / 3;

                    var startTangentWeight = (startKey.weightedMode & WeightedMode.Out) != 0 ? startKey.outWeight : oneThird;
                    var endTangentWeight = (endKey.weightedMode & WeightedMode.In) != 0 ? endKey.inWeight : oneThird;
                    var timeSpan = endKey.time - startKey.time;

                    var timeAxisBezier = new BezierSegment(0, startTangentWeight, endTangentWeight, 1);
                    var valueAxisBezier = new BezierSegment(startKey.value, 
                        startTangentWeight * startKey.outTangent * timeSpan, 
                        endTangentWeight * endKey.inTangent * timeSpan, 
                        endKey.value);
                    var isHermite = (startKey.weightedMode & WeightedMode.Out) == 0 && (endKey.weightedMode & WeightedMode.In) == 0;
                    original2 = valueAxisBezier.Compute(isHermite ? time : timeAxisBezier.Solve(Mathf.InverseLerp(startKey.time, endKey.time, time)).First(t => t is >= 0 and <= 1));
                    expect2 = remapCurve.GetPointOnCurve((float)original2).MappedValue;
                }
                
                double actual2;
                {
                    var keyRange1 = Enumerable.Range(0, mappedCurve.length - 1).TakeWhile(i => mappedCurve[i].time <= time).Last();
                    var startKey = mappedCurve[keyRange1];
                    var endKey = mappedCurve[keyRange1 + 1];

                    const float oneThird = 1.0f / 3;

                    var startTangentWeight = (startKey.weightedMode & WeightedMode.Out) != 0 ? startKey.outWeight : oneThird;
                    var endTangentWeight = (endKey.weightedMode & WeightedMode.In) != 0 ? endKey.inWeight : oneThird;
                    var timeSpan = endKey.time - startKey.time;

                    var timeAxisBezier = new BezierSegment(0, startTangentWeight, endTangentWeight, 1);
                    var valueAxisBezier = new BezierSegment(startKey.value, 
                        startTangentWeight * startKey.outTangent * timeSpan, 
                        endTangentWeight * endKey.inTangent * timeSpan, 
                        endKey.value);
                    var isHermite = (startKey.weightedMode & WeightedMode.Out) == 0 && (endKey.weightedMode & WeightedMode.In) == 0;
                    actual2 = valueAxisBezier.Compute(isHermite ? time : timeAxisBezier.Solve(Mathf.InverseLerp(startKey.time, endKey.time, time)).First(t => t is >= 0 and <= 1));
                }

                Assert.That(mappedValue, Is.EqualTo(expect2).Within(allowedError), $"at {time}, actual = {mappedValue}, actual2 = {actual2}, original = {original}, expect = {expectValue}, original computed ours = {original2}, expect with our original = {expect2}");
            }
        }

[anatawa12]

I think we should reconsider if we should define remap curve as [0,1] or [0,100].

Not all blendshapes will have [0,100] (most of them are so though) and non-VRChat platforms may supports them (by disabling clamping) and I think raw value mapping is better representation than multiplying by 100 so it might be better to express as [0, 100] curve.
For better support for non-[0,100] curve, we can disable clamping value range in editor but I think it's rare enough so clamping provides better experience. We might better to add option to change clamp value range or detect range by computing blendShape maximum / minimum frame value in the future.