(Re)move identical calls to InstrumentTrack::processAudioBuffer
#6867
Conversation
Remove identical calls to `InstrumentTrack::processAudioBuffer` which appear in the overridden implementations of `Instrument::play` and `Instrument::playNotes`. Instead the call to `processAudioBuffer` has been moved into `InstrumentPlayHandle::play` and `InstrumentTrack::playNote`. These two methods call the aforementioned methods of `Instrument`. Especially in the case of `InstrumentTrack::playNote` the previous implementation resulted in some unncessary "ping pong" where `InstrumentTrack` called a method on an `Instrument` which then in turn called a method on `InstrumentTrack`. And this was done in almost every instrument. In `InstrumentTrack::playNote` an additional check was added which only calls `processAudioBuffer` if the buffer is not `nullptr`. The reason is that under certain circumstances `PlayHandle::doProcessing` calls the `play` method by explicitly passing a `nullptr` as the buffer. This behavior was added with commit 7bc97f5. Because it is unknown if this was done for some side effects the code was adjusted so that it behaves identical in this case. Move the complex implementation for `InstrumentPlayHandle::play` and `InstrumentPlayHandle::isFromTrack` into the cpp file and optimize the includes.
Remove the unused variable `frames` in `VestigeInstrument::play` to fix the stricter GitHub build.
|
@LMMS/developers, can you please write some script that removes the unnecessary white spaces from the methods, etc.? It's a bit tiresome to discuss these in every pull request especially if the code was already there. |
Remove whitespace in methods and add it to the parenthesis of some statements. Remove underscores from parameter names. Remove usage of `this` pointer. Add `auto` keyword to shorten line length in `InstrumentPlayHandle::play`. Move `const_cast` of `NotePlayHandle` closer to the `process` method as this method is the reason that the cast is needed in the first place. One might also add a version of `nphsOfInstrumentTrack` that returns a non-const result but it seems to be a general problem of a missing clear responsibility so I keep it as is.
Add some comments which describe why we need to guard agains nullptr.
Replace the check for a `nullptr` buffer with a check that checks if the NotePlayHandle uses a buffer. This is effectively the same condition that's used by `PlayHandle::doProcessing` to decide if it should call play with a `nullptr` in the first place. Hence it should be the most fitting check.
I believe our I do agree though, that PR reviews asking for existing code to be reformatted (especially with the "request changes" option) can be tedious. I recall complaining about them too in the past. I imagine it's offputting to developers who want to contribute code to LMMS but find they're obligated to clean up somebody else's mess (and not even a mess necessarily - just outdated code style). |
Since I have reason to believe you are using VSCode, you can use the |
Ok, thanks! I will have to check. @sakertooth provided some good advice on how to integrate
Yes, exactly. It feels a bit like: "Oh, you provide us with some nice new feature into which you have invested lots of time? Please jump through these hoops so that it finally gets accepted and does not get to rot in the list of pull requests so that your time ultimately was wasted." I also believe that clean and consistent code is important but it still makes me wonder sometimes what's more important to the project: features and bug fixes or the code formatting. And the fun thing is that the punctual fixes even make the code more inconsistent. However, I am also aware that fixing them all for good would lead to merge conflicts in almost all pull requests which would be even more offputting to the people who have provided them. |
|
In my opinion code formatting, is an unrelated activity, to fixing any other bug or adding features, and should be treated as such and kept in a separate commit. Keeps the change logs easier to read, and when it comes to debugging in the future it's very hard to tell what was intended behavior changes, and what was just formatting. If you do want to insist that any contributor that touches a file, reformats it, then maybe ask them to do so as a follow-up. |
Remove identical calls to
InstrumentTrack::processAudioBufferwhich appear in the overridden implementations ofInstrument::playandInstrument::playNotes. Instead the call toprocessAudioBufferhas been moved intoInstrumentPlayHandle::playandInstrumentTrack::playNote. These two methods call the aforementioned methods ofInstrument. Especially in the case ofInstrumentTrack::playNotethe previous implementation resulted in some unncessary "ping pong" whereInstrumentTrackcalled a method on anInstrumentwhich then in turn called a method onInstrumentTrack. And this was done in almost every instrument.In
InstrumentTrack::playNotean additional check was added which only callsprocessAudioBufferif the buffer is notnullptr. The reason is that under certain circumstancesPlayHandle::doProcessingcalls theplaymethod by explicitly passing anullptras the buffer. This behavior was added with commit 7bc97f5. Because it is unknown if this was done for some side effects the code was adjusted so that it behaves identical in this case.Move the complex implementation for
InstrumentPlayHandle::playandInstrumentPlayHandle::isFromTrackinto the cpp file and optimize the includes.