Song::processNextBuffer() - add more comments and rename variables #1884
Conversation
…th confusing names
| @@ -353,23 +359,26 @@ void Song::processNextBuffer() | |||
| m_playPos[m_playMode].setCurrentFrame( currentFrame ); | |||
| } | |||
|
|
|||
| f_cnt_t lastFrames = ( f_cnt_t )framesPerTick - | |||
| ( f_cnt_t )currentFrame; | |||
| f_cnt_t framesToPlay = | |||
There was a problem hiding this comment.
framesToPlay is the old playedFrames, now closer to where it is first used
|
This looks great, thank you. If I can get a second set of eyes (@badosu, @curlymorphic) I'll merge. |
|
The new names and comments do make the code easier to understand. Looks good to me. |
|
The problem I have with all of these refactorings in the core is that they're going to conflict with the work that has already been done towards 2.0 which means lots of extra work resolving all the conflicts... |
@diizy duly noted. How do you want to proceed with these? In the case of |
|
Good question. I don't really want to stand in the way of necessary and useful fixes, but then this could mean that the work already done on the 2.0 branch might need to be redone almost from scratch... but then again maybe we can just cross that bridge when we come to it. By the way, feature freeze for 1.2 is long overdue... we really maybe shouldn't be accepting any more non-bugfix commits at this point. |
True, but then that would affect master just the same (unless 2.0 becomes master of course). I think a feature freeze is about due as well, which would mean we branch off our current master into a -Tres |
|
On that topic... I think we need help mergining in (or rejecting, deferring) the PRs that we have out there before we split off to a new branch, no? This way we have a line in the sand and don't cause a bunch of unnecessary rebasing for PRs that are only being held up by peer review. |
Why is 2.0 not listed as a milestone, like 1.2 and 1.3? |
I just hasn't been created. We've putting all of the objectives in 1.3 for now. It is an organization task and one that should probably be driven by the team writing 2.0. But this really depends on when we expect 2.0 to be ready. If Vesa gets enough buy-in, it may replace 1.3 entirely. Vesa would probably be the best to speak about it. -Tres |
|
Well, I think we'll concentrate on 1.2 for now, and see what our situation is like when 1.2 is released. We can then see if we have enough developers, enough coding resources to start working on 2.0, or should we do 1.3 first. |
|
I agree, but that leaves PRs like this still undecided.. Help! :) |
|
The readability in this (namely the comments) make it worth merging in. @diizy I'm sorry for the grief this causes the 2.0 branch. Meanwhile, perhaps we should consider developing 2.0 along side 1.3 on the same branch and use refactoring or a certain naming convention for distinguishing old versus new objects? That way we're more cognizant of eachothers work and how it impacts eachother... Just a thought. |
|
@M374LX good work here and thank you. |
Song::processNextBuffer() - add more comments and rename variables
Improve the documentation of Song::processNextBuffer() by adding more comments and renaming variables with confusing names