expose fade times for root activation/deactivation #52

awood314 · 2024-07-19T02:04:49Z

Instead of the baked-in fades that occur with root node activation and deactivation, this exposes individual fade-in and fade-out times (in ms), which can help in cases where we want to preserve transients.

nick-thompson

Great, thank you! Couple suggestions inline–

nick-thompson · 2024-07-19T14:43:49Z

js/packages/core/src/Reconciler.res

  let visitSet = Set.make()
  let roots = Belt.List.mapWithIndex(Belt.List.fromArray(graphs), (i, g) => {
    NodeRepr.create("root", {"channel": i}, [g])
  })

  visit(delegate, visitSet, roots)
-
-  RenderDelegate.activateRoots(delegate, Belt.List.toArray(Belt.List.map(roots, r => r.hash)))
+  RenderDelegate.activateRoots(delegate, Belt.List.toArray(Belt.List.map(roots, r => r.hash)), rootFadeInMs, rootFadeOutMs)


I'd prefer to keep the custom fade times out of the activateRoots chain, just because we already have a suitable pattern for propagating properties like this to the nodes that need them 3 lines up–

NodeRepr.create("root", {"channel": i, "fadeInMs": rootFadeInMs, "fadeOutMs": rootFadeOutMs}, [g])

With just that change we can keep activateRoots totally ignorant to the business of root node fades, which I think is valuable (any part of the code that doesn't need to know something, shouldn't know it). Plus this way we don't change anything about the instruction format

oh nice! didn't realize you could pass properties into create. I like this much better 👍

nick-thompson · 2024-07-19T14:45:12Z

runtime/elem/builtins/Core.h

        }

-        void activate(FloatType initialGain = FloatType(0)) 
+        void setFades(FloatType fadeInMs, FloatType fadeOutMs) 


This new method goes away too if we use the setProperty chain, which I find cleaner

nick-thompson · 2024-07-19T14:45:51Z

runtime/elem/builtins/Core.h

        {
-            setProperty("active", true);
-            currentGain.store(initialGain);
+            fadeIn.setFadeTimeMs(GraphNode<FloatType>::getSampleRate(), fadeInMs);


I think it'd be cleaner to still have 1 fade but parameterize the GainFade class to accept different values for up/down steps

I had that thought as well, but thought it might get weird if you activate and deactivate quickly. i'll think through that again

nick-thompson · 2024-07-19T14:49:54Z

js/packages/core/index.ts

@@ -152,6 +152,9 @@ class Renderer {
  private _sendMessage: Function;
  private _nextRefId: number;

+  // Fades applied to roots when they are activated and deactivated as a result of render
+  rootFades: { inMs: number, outMs: number} = { inMs: 20, outMs: 20 };


Are these public with the intention of a user writing property updates after constructing the renderer if they want to customize fade times?

let x = new Renderer((batch) => something(batch)); x.rootFades.inMs = 8; x.rootFades.outMs = 8; x.render(...);

Personally I'm not a big fan of the imperative style there; I'd prefer that we either:

Make these things constant for the lifetime of the Renderer instance, in which case we take these as constructor args that you don't get to change later

Make these arguments to the render() function so that it's clear (and more declarative) with each invocation what your fade expectations are

ha, I thought you might not like this, but I agree it does feel a little gross. I think then I'll go with adding a separate renderWithOptions so we don't break the existing interface. sound good?

nick-thompson

Awesome, this looks great! Thanks for covering all the earlier feedback. I just left a few more small nitpicks, if you don't mind. Then happy to merge asap!

nick-thompson · 2024-07-23T14:32:16Z

runtime/elem/GraphRenderSequence.h

@@ -178,7 +178,7 @@ namespace elem
        {
            // Don't promote if our RootRenderSequence represents a RootNode that has become
            // inactive, even if it's still fading out
-            if (rootPtr->getTargetGain() < FloatType(0.5))
+            if (!rootPtr->active() && rootPtr->stillRunning())


Just noticing this– I think this changes the semantics of the conditional. We want to return early if the root node is inactive, even if it's still fading out.

I have a ticket in my todo-list to revisit here to smoothly fade out the feedback taps here, but until we get to that I'd say let's preserve the existing behavior here

nick-thompson · 2024-07-23T14:36:04Z

runtime/elem/builtins/Core.h

+                if (!val.isNumber())
+                    return ReturnCode::InvalidPropertyType();
+
+                fade.setFadeInTimeMs(GraphNode<FloatType>::getSampleRate(), static_cast<float>((js::Number) val));


Super nitpick but you don't need the static_cast<float> because (js::Number) is just a double, and the setFadeInTimeMs and equivalent methods below accept double

nick-thompson · 2024-07-23T14:37:38Z

runtime/elem/builtins/helpers/GainFade.h

+            updateCurrentStep();
+        }
+
+        void setCurrentGain(FloatType gain) {


we use it a couple places in StereoSampleSeq, e.g. https://github.com/splice/platform/blob/7bfd56361fcf97ebfd7a0dcd03b2a434c6b478a5/plugins/studio-one-integration/SpliceSampleSeq.h#L59

nick-thompson · 2024-07-23T14:39:05Z

runtime/elem/builtins/helpers/GainFade.h

-            targetGain = g;
+        void process (const FloatType* input, FloatType* output, int numSamples) {
+            auto const _currentGain = currentGain.load();
+            if (_currentGain == targetGain.load()) {


Let's use the fpEqual helper here, and inside the block multiply against the target gain (so that we mul against the target even when the current is just slightly off but still satisfies fpEqual)

nick-thompson · 2024-07-23T14:42:14Z

runtime/elem/Runtime.h

@@ -354,7 +354,7 @@ namespace elem
            auto ptr = std::dynamic_pointer_cast<RootNode<FloatType>>(it->second);
            if (ptr)
            {
-                ptr->activate(currentRoots.empty() ? FloatType(1) : FloatType(0));
+                ptr->setProperty("active", true);


Just to make sure I'm following here, even though this was maybe my idea anyways from our early convos– this line does change the default behavior of the initial render(), right? But our intention is that a user who wants to disable the fade on the first render should make their first render call with renderWithOptions({rootFadeInMs: 0}, ...graph);

Just want to be clear on that so I can update the docs correctly when we push v4

This reverts commit 226c05b.

nick-thompson · 2024-07-25T15:30:29Z

Merged, thank you!

expose fade times for root activation/deactivation

7fb7947

nick-thompson requested changes Jul 19, 2024

View reviewed changes

awood314 added 3 commits July 19, 2024 09:49

feedback

aa44e01

oops

743a879

fade with independent in/out times

b2de8e9

awood314 requested a review from nick-thompson July 22, 2024 15:53

optimize

16668c7

nick-thompson approved these changes Jul 23, 2024

View reviewed changes

awood314 added 7 commits July 23, 2024 09:04

feedback

f12b6a9

l2read

0ae49af

fix tests and generate new snapshots

226c05b

Revert "fix tests and generate new snapshots"

0338960

This reverts commit 226c05b.

fix tests

4d81418

more tests

2a7d23c

bump wasm builds

d29f4fe

nick-thompson merged commit ced5db5 into elemaudio:develop Jul 25, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

expose fade times for root activation/deactivation #52

expose fade times for root activation/deactivation #52

awood314 commented Jul 19, 2024

nick-thompson left a comment

nick-thompson Jul 19, 2024

awood314 Jul 19, 2024

nick-thompson Jul 19, 2024

awood314 Jul 19, 2024

nick-thompson Jul 19, 2024

awood314 Jul 19, 2024

awood314 Jul 22, 2024

nick-thompson Jul 19, 2024

awood314 Jul 19, 2024

nick-thompson left a comment

nick-thompson Jul 23, 2024

nick-thompson Jul 23, 2024

nick-thompson Jul 23, 2024

awood314 Jul 23, 2024

nick-thompson Jul 23, 2024

nick-thompson Jul 23, 2024

nick-thompson Jul 23, 2024

awood314 Jul 23, 2024

nick-thompson commented Jul 25, 2024

expose fade times for root activation/deactivation #52

expose fade times for root activation/deactivation #52

Conversation

awood314 commented Jul 19, 2024

nick-thompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-thompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nick-thompson commented Jul 25, 2024