Adding RadioLanewayCrossfade transition #13750
Conversation
Adding RadioLanewayCrossfade transition. See mixxxdj#13716
(Wow this is fussy!).
More style issues.
Yet more code style issues (Space at end of line.)
More Code tidy Missing case in cueinfo.cpp
Code style
Spelling
avoiding a DEBUG_ASSERT(!"Unknown Loop Type");
fix up tooltip
src/analyzer/analyzersilence.cpp
Outdated
| template<typename ForwardIterator> | ||
| ForwardIterator last_fade_in_sound(ForwardIterator begin, ForwardIterator end) { | ||
| return std::find_if(begin, end, [](const auto elem) { | ||
| return fabs(elem) >= kFadeInThreshold; | ||
| }); | ||
| } | ||
|
|
||
| template<typename Iterator> | ||
| Iterator first_fade_out_sound(Iterator begin, Iterator end) { | ||
| return std::find_if(begin, end, [](const auto elem) { | ||
| return fabs(elem) >= kFadeOutThreshold; | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
this is a copy of the above for very little reason. Consider deduplicating by simply taking the Threshold as a parameter.
There was a problem hiding this comment.
The second one goes backward, while the first is strictly forward. I spent a long time playing around with these, trying to determine why I was getting weird results (and so I was keen to keep them separate). It turns out AutoDj processing can take place before a track is fully analysed, so I am less invested in that now.
There was a problem hiding this comment.
The functions are identical. In this context ForwardIterator and Iterator are just names that resolve to the underlying iterator implementation when called. So there is no difference in terms of what function can be called when. A std::reverse_iterator is a "bidirectional iterator" and thus also a "forward iterator".
There was a problem hiding this comment.
std::find_if only requires a forward iterator.
There was a problem hiding this comment.
If you like I'll refactor all of those into one, including the db60.
There was a problem hiding this comment.
As I went to do this I've had second thoughts.
Can you look again at this, and consider how much if any savings will be made by a refactor here? Since we are assigning parameters to the stack in a call for functions have are already present in std lib (so nothing additional) then I don't see a saving, in fact, we are adding to the time to complete. And, the code is more readable with several functions (note the difference is a const). Unnecessarily concise code does not equal readability, right?
There was a problem hiding this comment.
well, the think that trips me up when reading is that the body is repeated several times. Its difficult to spot where the difference is. I don't expect any meaningful performance improvements from this. If you want to give the difference functions a name, you're welcome to do so, but remove the duplicated bodies.
Something this would work for me:
template<typename It, typename T>
It find_first_above_threshold(It begin, It end, T threshold) {
return std::find_if(begin, end, [threshold](const auto& elem) {
return std::abs(elem) >= threshold;
});
}
template<typename It>
It last_fade_in_sound(It begin, It end) {
return find_first_above_threshold(begin, above, kFadeInThreshold);
}
template<typename It>
It first_fade_out_sound(It begin, It end) {
return find_first_above_threshold(begin, above, kFadeOutThreshold);
}
There was a problem hiding this comment.
I understood what you wanted, and to be honest there is nothing in it really, not from the readability point of view (as I see it). And while having the const and not pushing as much on the stack, should be slightly faster, but not meaningfully faster... and who knows what the the compiler will actually do, so this isn't a big deal.
However, given that this is the analysis section... is performance important?
There was a problem hiding this comment.
Not in terms of what this would a difference, no. I'm purely suggesting this because I find it easier to read.
There was a problem hiding this comment.
How anyone finds lambda functions with closure readable is beyond me anyway, so I have made the changes. And while I remain slightly concerned about performance, this is not the hill I would choose to die on.
(And besides I find braces on the same line as an 'if' far from readable, as studies into this have shown cf Code Complete 1st ed, so perhaps I am not the best to judge).
| "volume last falls below -12Db or at the spin box setting which ever\n" | ||
| "is lower, and potentially starts the next earlier if it starts below\n" | ||
| "-27Db."); |
There was a problem hiding this comment.
its very easy to change the threshold in the analyzer but forget to update it here. Do you think its valuable to have the thresholds documented here or can we just omit them?
There was a problem hiding this comment.
I nearly made it a more general statement (e.g., 'a certain level'), but I figured someone would want to know the exact values. It may well be worth making it more general for the reason you suggested.
I suggest you do. This transition is groundbreaking. |
and removal of commented code.
Yet more code format issues.
Yet even more code style issues.
Comment correction
Adjust lasttSampleBeforeFadeOut (and avoid a regression with lastSoundSample) to bring it into line with mixxxdj#13545 (I assume I was working off an older code base.)
RadioLanewayCrossfade transition.
See issue: #13716.