Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Playback.cs #45

Closed

Conversation

FischertBilligheim
Copy link
Contributor

The logic and realization of mute/solo functionality of a midi-mixing console is a little bit tricky...
I think using an Array of bool is simpler than using a method to enable/disable a single channel.

E.G think about the "Solo" functionality: I expect, that if I use a channel "solo" - and then I remove the "Solo"-state, the combination of the enabled channels before are restored. That means a combination of 16 channels have to be restored - using an array for that is simple.

I also expect, that not only one channel can be used for "Solo" - it also should be possible to use several of them...

@melanchall
Copy link
Owner

Thanks for your PR! Let's discuss it:

  1. Array of bools totally lacks of value checks. I can pass array of 1000 elements which is obviously wrong. Also what if I take value by index that is greater than length of the array? I'll get IndexOutOfRangeException without any information about what range is valid.
  2. I understand that manipulating by arrays is easy in terms of save/restore entire set of channel states. But with methods like MuteChannel/UnmuteChannel/IsChannelMuted you also can maintain these arrays on your side. Your implementation is heavily tied to your particular case.

Also please note that within the library all private fields should be prefixed with _ (underscore). And instead of 16 you should use FourBitValue.MaxValue + 1.

You can either implement methods within this PR or I can do it by myself :)

@FischertBilligheim
Copy link
Contributor Author

Yes, let's discuss :-)

Sure, I agree - the array-length must be tested!!
What about my latest Change? Would that be sufficiant from your Point-of-view?

And would "16" not be simpler to understand than "FourBitValue.MaxValue + 1"?.
Or what is the Advantage of that construct?

Regards
Thomas

@FischertBilligheim
Copy link
Contributor Author

What about the idea, to do both:

  • Single call’s for single operations
  • The Array for Bulk-Operation

And I have another question: Do you see the need for using a semaphore to synchronize the access to the “mute”-data?

@melanchall
Copy link
Owner

  1. 16 is "magic constant". You should avoid magic constants and name them semantically. For example, you can define constant private const int ChannelsCount = FourBitNumber.MaxValue + 1.
  2. Exception is meaningless. You should always use appropriate exception classes. Within the library there is ThrowIfArgument class providing arguments checks. You can see usage of this class across entire library and within Playback class too. In this case ThrowIfArgument.IsOutOfRange should be used.
  3. There is no check for null.
  4. As I said before what if I call this: ChannelEnabled[1000]? I'll get IndexOutOfRangeException. Without any information about what index is valid.

I still think that separate methods is the best way to implement channels muting. But bulk editing also interesting idea. Thanks for the PR and your ideas! I really appreciate your contribution. I think though it can be closed. I'll implement this API by myself today in a way we both will be satisfied :)

@FischertBilligheim
Copy link
Contributor Author

FischertBilligheim commented Oct 22, 2019

👍
One last hint:
To implement a „mute“ functionality for the midi channels was a compromise: This was very simple to realize.... The real requirement is to use it on a track-chunk level.....But with the current concept of the playback this seems not be so simple..,

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
DryWetMIDI
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

2 participants