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 Line, XLine: Incorrect starting value, early arrival, etc. #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

• Code is tested
• All tests are passing
  • initial phase tests were added for LFPulse, LFSaw, LFPar, LFCub, LFtri, SinOsc
  • start/end value test was added for Line, XLine
  • 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
• This PR is ready for review

[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.

[telephon]

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.

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

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?

[JordanHendersonMusic]

This looks really good fixes many failing tests on my system, thanks!

I've got no issue with the approach.

The comments here are about C++ guidelines and sc formatting and code clarity (except the static cast which should definitely be used instead). If you don't agree with them, I'm quite happy to just approve as is however!