Fix UGen initialization: LFUGens pt. 1/3, Trig, Trig1

[mtmccrea]

Purpose and Motivation

A majority of UGens do not initialize with a correct initialization sample, and subsequently output an incorrect first sample. This PR is part of an ongoing effort to fix UGen initializations, the progress of which is tracked here.

This fixes the initialization of a batch of LFUGens:

• Line, XLine
• Vibrato
• LFPulse, LFSaw, LFPar, LFCub, LFTri
• Trig and Trig1 were also fixed (they're in TriggerUGens) - this was needed in order to pass related unit tests

Line was also made to call Line_next_nova_64, which was omitted previously.

(fixed Issues will be updated here)

• Fixes #4279

Types of changes

• Bug fix
• Breaking change - changes are technically breaking for the first output sample (or, e.g., a one-sample phase change from previously)

To-do list

• [x] Code is tested
• [x] All tests are passing
  • The following Unit tests were made to pass with these changes:

TestCoreUGens: test_ugen_generator_equivalences - "SinOsc.kr can match Line.kr.sin"
TestCoreUGens: test_ugen_generator_equivalences - "SinOsc.kr can match Line.kr.cos"
TestCoreUGens: test_ugen_generator_equivalences - "Trig1.ar(_,0) has same effect as (_>0) on variable-amplitude impulses"
TestCoreUGens: test_ugen_generator_equivalences - "Trig.ar(_,0) is no-op when applied to Impulse.ar, whatever the amplitude of the impulses"
TestCoreUGens: test_ugen_generator_equivalences - "Trig1.ar(_,0) is no-op when applied to Impulse.ar"

• Note there are failing tests for Duty in TestCoreUGens, and TestEnvGen_server: test_zero_gate_n_off_not_sent, but they were failing before so are unrelated.
• [ ] Updated documentation - N/A
• [x] This PR is ready for review

[dyfer]

Thanks!

• The following Unit tests were made to pass with these changes

In that case, could you re-enable these in the test suite by removing this?

[mtmccrea]

Note there are failing tests for Duty in TestCoreUGens

Unless I'm mistaken because there are still other (unrelated) failing tests in TestCoreUgens, we wouldn't want to remove this yet.

[dyfer]

Ah I see, thanks for pointing it out. In that case let's not include back TestCoreUGens at this time.

[mossheim]

changes lgtm thanks! is there any plan for further testing and how should we integrate these? is it ok to merge piecemeal or do you want to wait until all have been reviewed?

[mtmccrea]

Thanks, @mossheim.

is there any plan for further testing

I try to do tests with a variety of argument rates, etc. for each UGen: https://github.com/mtmccrea/supercollider/wiki/UGen-Fix-List#notes-on-testing-technique

So I'd consider my testing done, and my hope has been that these spend some time in circulation in the dev branch for more exposure.

how should we integrate these? is it ok to merge piecemeal or do you want to wait until all have been reviewed?

I try to batch these into sensible clusters for each PR so they can be be reviewed and merged individually, otherwise it would run on for years, literally 😆

For UGens that are particularly complex, if the changes are beyond fixing initialization problems, or if changes may be contentious for other reasons, I try to make them separate PRs (E.g. Delay1/2, EnvGen, etc.)

[mossheim]

I try to do tests with a variety of argument rates, etc. for each UGen:

Are these in the test suite or purely manual testing?

I try to batch these into sensible clusters for each PR so they can be be reviewed and merged individually, otherwise it would run on for years, literally

OK, so it is not a problem if we release a version with fixes to some UGens but not all?

[mtmccrea]

The tests are manual. I have a template, but unfortunately each one requires manual setup specific to each UGen.

OK, so it is not a problem if we release a version with fixes to some UGens but not all?

Correct, each PR is standalone. I try to avoid cross-dependencies in general, but I'll note them in the description if there are (this one doesn't have any).

[mossheim]

The tests are manual. I have a template, but unfortunately each one requires manual setup specific to each UGen.

What do you mean? Are you instrumenting the C++ code somehow or using additional tooling?

[mtmccrea]

What do you mean? Are you instrumenting the C++ code somehow or using additional tooling?

My template is in SC, it generates multiple function calls for the UGen under test, with varying i/o and arg rates, then plots the output over some useful number of cycles. Then I inspect plots. By "manual" I mean I have to tune each UGen test to set argument ranges and input signals that are appropriate to the UGen.

I also have to insert print statements in the C++ code in order to inspect the init sample, since this can't be inspected from SC (it's never written out), and compare it to the first output sample.

[mtmccrea]

Pinging on this batch of initialization fixes. It would be great to have this review completed— these changes fix a number of unit tests that have been broken for a loooong time. And further work on fixing other UGen initializations depends on these UGens behaving properly.

[mtmccrea]

I'm not sure if it would help to add a few more tests? Since this changes subtle behaviour it would be good if one could understand (and fix) these changes through the boundary cases.

Sure, currently working on adding unit tests now (will update here when that's complete)... I suppose there's a balance to be struck for the larger project of fixing UGen initialization bugs. If we require unit tests for each fix, it adds quite a bit of overhead to a project that's already a bit of a slog. I don't disagree it's useful of course...

[JordanHendersonMusic]

@mtmccrea I think its the existing unit tests that check for initialisation that are often flaky when ran on through CI. Might want to be extra careful how they are written.

[mtmccrea]

@mtmccrea I think its the existing unit tests that check for initialisation that are often flaky when ran on through CI. Might want to be extra careful how they are written.

I'm not sure what kind of initialization you're referring to, but this pertains to the "initialization sample" problem, which can't actually be checked directly because that sample is never actually written out—instead the approach is to check the proper phase of the signal, which will be off if the UGen state isn't reset in the course of graph initialization.

~~In any case the tests are working so far, just struggling to try reconfiguring clang-format after updating my OS and XCode recently.~~

EDIT: OK so looks like a version bump was merged #6196

[mtmccrea]

OK I managed to get formatting working (and made a janky update to the wiki... which should be more properly updated).

[mtmccrea]

Since this PR fixed a handful of unit tests in TestCoreUGens: test_ugen_generator_equivalences, I've re-enabled that suite. Theres now just 2 unit tests in that suite that fail—Duty tests—so I commented those 2 out so the other 110 tests can run.

So it looks like this CI should pass, so I think this is finally good to go.

[mtmccrea]

@JordanHendersonMusic could you confirm I addressed your concern with the unit tests? Otherwise I think I've addressed everything in the comments/reviews and this should be ready to go.

[JordanHendersonMusic]

Oh yea, I was wrong and thought it was failing for a different reason!

[mtmccrea]

would anyone like to give this an "official" review?