Bela PR review patches, last part

[mossheim]

Last round of review patches for the Bela PR. this one is a bit bigger than the other ones, but i have tried to keep the commits small and logical so that it can be reviewed incrementally, and so that each code transformation is clear.

[giuliomoro]

There was need for three fixups to make it compile (I added each fixup where needed instead of having them all at the end). I also added a couple of commits on top. Here https://github.com/BelaPlatform/supercollider/tree/topic/bela-cpp-3

Let me know what you think.

Haven't tested it yet, though! Fingers crossed!

I think that one outstanding issue is that BelaScope should use the C version Scope_c instead of Scope, right?

[mossheim]

thanks for that @giuliomoro . sorry, i really would like to have an environment to test/compile this. i tried playing around a bit with the repo instructions, but on Arch it was not straightforward. i meant to go back and try again with systemd-nspawn.

the commits you added all look good to me, except i don't understand https://github.com/BelaPlatform/supercollider/commit/6b184c2ff5f5a323781590452a2089a0c49bad2f. since there can only be one live instance of SC_BelaDriver, and the context object's lifetime is already confined to the lifetime of that instance, when would this branch ever be taken?

I think that one outstanding issue is that BelaScope should use the C version Scope_c instead of Scope, right?

i'd really like to save discussion of that until after the main PR is merged, if that's all right. after this PR, and a cleanup of the Bela readme, we can merge your PR and start working on these other issues in smaller PRs and tickets that are more easily managed. or perhaps there is something small and easy we can do right now that i've missed?

[giuliomoro]

since there can only be one live instance of SC_BelaDriver, and the context object's lifetime is already confined to the lifetime of that instance, when would this branch ever be taken?

I am not sure what - if any - guarantees the Sc internal AudioDriver API makes about calling DriverSetup() during the lifetime of the SC_AudioDriver object. So, as the pointer is only deleted in the destructor, if DriverSetup() is called twice ([it is not now but it may be in a future version of Sc, as far as we know]), we would be leaking it and I wanted to guard against this. I'd be happy to remove that if you think this is not necessary, but then I would suggest that the SC_AudioDriver.h contains comments about when and how the virtual methods are called.

[mossheim]

ah yeah, good point! can you please remove the if{} and just leave the two lines:

        delete mWorld->mBelaScope;
        mWorld->mBelaScope = nullptr;

since the branch isn't necessary and is already performed by delete.

after that, if you're satisfied with these changes, you can merge your branch (https://github.com/BelaPlatform/supercollider/tree/topic/bela-cpp-3) into your bela-ready-to-merge branch yourself if you like, or i can update my branch here and you can merge it via GitHub.

[giuliomoro]

I force-pushed that in the meantime :) will merge my branch in

[mossheim]

thanks!