-
-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Enable fading of samples in the Sample Track #5616
base: master
Are you sure you want to change the base?
Conversation
🤖 Hey, I'm @LmmsBot from github.com/lmms/bot and I made downloads for this pull request, click me to make them magically appear! 🎩
Linux
macOSWindows
🤖{"platform_name_to_artifacts": {"Linux": [{"artifact": {"title": {"title": "(AppImage)", "platform_name": "Linux"}, "link": {"link": "https://12504-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.72%2Bg6b8df1b-linux-x86_64.AppImage"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12504?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "macOS": [{"artifact": {"title": {"title": "", "platform_name": "macOS"}, "link": {"link": "https://12505-15778896-gh.circle-artifacts.com/0/lmms-1.3.0-alpha.1.72%2Bg6b8df1bbc-mac10.14.dmg"}}, "build_link": "https://circleci.com/gh/LMMS/lmms/12505?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link"}], "Windows": [{"artifact": {"title": {"title": "32-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/dwsumo97n1vv1e81/artifacts/build/lmms-1.3.0-alpha-msvc2017-win32.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37649975"}, {"artifact": {"title": {"title": "64-bit", "platform_name": "Windows"}, "link": {"link": "https://ci.appveyor.com/api/buildjobs/nyg8t4j0be3qpf2v/artifacts/build/lmms-1.3.0-alpha-msvc2017-win64.exe"}}, "build_link": "https://ci.appveyor.com/project/Lukas-W/lmms/builds/37649975"}]}, "commit_sha": "e44d5bb43a3d8bb5259c39ddb165c5310c6f6588"} |
True, there are glitches. You can also see those at the top. I'd have to move the lines a slight bit inside the widget I suppose. The widget should not be considered final, though. |
Big up! |
src/core/Track.cpp
Outdated
@@ -708,7 +708,6 @@ void TrackContentObjectView::paintTextLabel(QString const & text, QPainter & pai | |||
elidedPatternName = text.trimmed(); | |||
} | |||
|
|||
painter.fillRect(QRect(0, 0, width(), fontMetrics.height() + 2 * textTop), textBackgroundColor()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree with this change. We need this to ensure text stays legible, especially since we allow clips to be colored. I think you should either let the rectangle display on top of the fade curve, or adjust the fade curve's height so that it doesn't share any space with the rectangle.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it now. With the text, it looks a bit strange when not zoomed in. Maybe the fade handles should only be displayed when zoomed in close enough?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks great now while zoomed in! I'll have to test it out before I can give an opinion on different zoom levels, I'll try to do that tomorrow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it looks fine zoomed out, it's easy enough to see the duration of the fades. That being said, the boxes representing the handles seem to be drawn too low, with a regular height clip you have to press above the box in order to adjust the fade.
Love this feature. I would like to request one thing though, and that is to make the color darker instead of lighter where the curve is cutting off the clip. It makes it seem like the cut off volume is highlighted (thus in focus), but it would make more sense to have the active area be the highlighted/in focus one (thus the non-active area would be darker). |
I completely agree. Every other DAW, darken the color instead of brightening. For example bitwig, you can see it in the image i posted before |
Great job. I'd make it a little less dark, and also would be great if in the future there will be the possibility to choose the curve of the fading. If you watch bitwig one you can get what i mean, we only have 1 possibility of fading, while would be cool to have the possibility to choose between a straight line, or a curve in both direction. FL achieves that using a point in the middle of the line. Moving that point up or down will choose the shape of the line. |
I tested this PR and when sample is not long enough fading marker cannot be grabbed. Instead sample moves through timeline. https://drive.google.com/file/d/1A5LRGgYVgs7lDgJypMpWpCqGGRh0r0zV/view?usp=sharing |
I'm working on that. Will most likely be something like a bezier curve. |
I cannot open this, but I know what problem you are referring to. The areas for grabbing are 1/32 of the widget width and will thus vanish when the widget is too small. I will fix that. |
I find the feature very interesting! Is it finished? I tested it and works but it's not saving the fading in the project file. |
Unfortunately, this feature is far from being finished. The fading positions are not saved in a project file yet. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like this PRs work so far! I gave it an initial review, still not a complete one. There are code style issues to be fixed, specially about indentation (you probably used space-indenting on your editor). Some other smaller ones like variable naming and spacing. As far as the logic goes I still didn't find any issues, but I didn't dig deep enough yet.
When you merge master/rebase, double check one method mentioned in the review (SampleBuffer::visualize
), since Github isn't accusing a conflict but there were changes to this method recently. I've seen it happen before that one PR overwrites changes from another one by accident, so be watchful with that one.
I'll be coming back to this PR to another more thoughtfully review!
@@ -48,6 +48,86 @@ class QRect; | |||
// may need to be higher - conversely, to optimize, some may work with lower values | |||
const f_cnt_t MARGIN[] = { 64, 64, 64, 4, 4 }; | |||
|
|||
|
|||
class ControlPoint |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we have a separate header for that and CubicBezier
? Maybe one for each is a overkill but we could get them both in a single separate header. I can see it being reused in other places like AutomationPattern
for example!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if he is actively working on it at the moment, but as long as the Bezier header is done in a way that makes it usable by any other class that need to calculate Bezier curves it should be fine!
include/SampleTrack.h
Outdated
bool m_leftCorner = false; | ||
bool m_rightCorner = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bool m_leftCorner = false; | |
bool m_rightCorner = false; | |
bool m_moveLeftCorner = false; | |
bool m_moveRightCorner = false; |
As far as I understood from this first review, those are used on the mouse events for dragging the corners right? Maybe call them m_moveLeftCorner
to make it consistent with the other members like m_moveLeftP1
for example, also making their function clearer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are used for that. I'll rename those.
Thanks a lot for the initial review @IanCaio and @Veratil. I'll look into it tomorrow. This PR is far from being finished however, it is just a draft for the basic concept I've put together. I think it won't be possible to get this PR ready before the refactor deadline as many things are still missing (crossfading, saving the fades in the project, performance issues for very long samples). |
Add in saving and then we can add in the other stuff (in later PRs)? 😁 |
Not a thing! We can still try to merge it before the refactor begins: there are other things in motion before we can start it, so there's a little time. Even if it doesn't have all the features you want to include, if it's usable and stable we could review, merge it and continue working on it after the refactor to add the things missing. As for the performance, I'll have to look better into it. The easiest and simplest solution is probably a look-up table for the values of the curve. There are other alternatives that will probably require some more rewriting, i.e.: actually applying the fading to the samplebuffer data instead of multiplying during playtime. That would require us to have a copy of the original frames that are faded for when we drag the handles again. If the performance load isn't huge we can maybe leave this for after the refactor as well. |
The problem is that we have to find the value of the Bezier curve for a specific sample. The Bezier curves are parametrized with a value @PhysSong: As you have already worked on Bezier curves, do you know a better solution? You notice the performance impact when you apply the fades to a whole song loaded as a sample. It is only impacted when moving the fades though. There is no issue while playing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly style review, one or two comments about other issues.
I still need to read a little bit more on Bezier curves and on how it's implemented in the code. When you mentioned performance issues I thought those were related to the Bezier curve points being calculated during play time (
Well, it's still an issue but much less critical if the performance is only impacted when setting the fades instead of when playing the samples. If we don't figure it out until the refactor we can merge it as is, documenting the need to optimize it, and figure it out after the refactor (since it won't affect the user as negatively as playtime performance issues). By the way, I think the conflicts I mentioned showed up after the last commit. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the unneeded virtual
keywords as Dom said in Track.h
for leaveEvent
and mouseMoveEvent
, code style looks good. 👍
Regarding the Bezier curve: there are several method to calculate/approximate the value.
Also, I think Newton's method is not good for some cases where the derivative goes to zero. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious about opinions on these
@thmueller64 Are you still working on this? |
@PhysSong thanks for the reminder. I have implemented linear interpolation in the meantime which didn't noticeably improve the performance. I found that the real bottleneck lies in redrawing the waveform. When I turn off drawing, moving the fades works seemlessly without any delay. Performant drawing of the audio waveforms like in other DAWs has to be addressed seperately in my opinion. |
Agreed. We also need some comments about where things are done, and not being familiar with that side of the codebase, it took me a while to realize that the drawing was done in I think the best way to speed up drawing would be to cache the base waveform, then calculate modified data if a fade touches it. Right now it looks like there's no caching and it's recalculated each time. This is a lot of overhead waste. |
Just a heads up, that's one of the things I think we should fix on the refactor. |
I think it has to work even without caching. For example, when you turn the amplification knob in the audio file processor on a sample of like 2min+, you have to update every value with a simple multiplication. On my hardware, the waveform plot also struggles to catch up in time and is very unresponsive. |
You're right, we definitely need to figure out a better way to speed up the waveform visualization. My cache idea I think is better overall for UI responsiveness when not changing any dependent variables, rather than real-time updating. With many sample buffers and constantly calling the visualize function to calculate the same thing over and over again just makes things slow.
With my cache idea this will still slow down for you unfortunately. We can still look to find some calculation savings though for this loop. Figuring out what is constant during the loop and what can change could speed up some parts. |
Declare functions virtual for SampleTCO, remove rectangle clashing with fades Add fading for sampletracks. Add von Hann window as fading function and make it the default. Fix formatting issues. Fix build issue on macOS. Dark shading, border adjustments, reinclusion of text. Fix build issue on Windows. Better clicking behavior. Prevent moving of handles into border. Add cubic Bezier as fade function. Menu for choosing fade style. Apply suggestions from code review Co-authored-by: IanCaio <[email protected]> Change naming of variables Apply retab to replace spaces Apply suggestions from code review Co-authored-by: Kevin Zander <[email protected]> Apply retab to replace spaces Update src/tracks/SampleTrack.cpp Co-authored-by: Kevin Zander <[email protected]> Removed unnecessary virtual keyword Update include/Track.h Co-authored-by: Hyunjin Song <[email protected]> Update src/core/SampleBuffer.cpp Co-authored-by: Hyunjin Song <[email protected]> Enable saving the fade positions and the fading shape Double check that fades and control points lie in [0, 1] Update src/core/SampleBuffer.cpp Co-authored-by: Hyunjin Song <[email protected]> Fix format and further conflicts.
b46aa02
to
4434840
Compare
auto x = xb + ((frame - first) * double(w) / nbFrames); | ||
// Partial Y calculation | ||
auto py = ySpace * m_amplification; | ||
auto py = ySpace * m_amplification * fade; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been thinking about how to speed up some of the recent loop calculations in some of the PRs I've seen. The compiler should already see that some of these values are the same each loop and optimize it, but just in case we can help out even more.
This line for instance, if we pull ySpace * m_amplification
out of the loop, then we can save a multiplication every loop (in case of bad optimization).
Additionally for the x
line above, can we pull out double(w) / nbFrames
to save heavy division? I know this will process left to right, so it could change the result if we did the division first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created a separate issue for the whole drawing concept #5908 . I hope it is OK if I resolve this for now and defer the discussion to the issue.
} | ||
|
||
|
||
void SampleTCOView::enterEvent(QEvent *e) | ||
{ | ||
if (e != nullptr) { m_inside = true; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can e
be a nullptr
? I guess if called manually, but I don't think it will ever be nullptr
when Qt generates it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not. I think it is good to check anyway, to be on the safe side.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well the reason I bring it up is because the event is called only if raised, so e
will never be nullptr
. The only time it will be is if we were to call it manually without.
Co-authored-by: Kevin Zander <[email protected]>
Co-authored-by: Kevin Zander <[email protected]> Co-authored-by: IanCaio <[email protected]>
@Veratil I did some crude profiling of the visualize function to see where we need to optimize. It turned out that most of the time is spent in drawing the waveform with Before for loop: 0,017852 So we have to figure out a better concept there. Reducing the number of points drawn for example. One standard approach would be to take the maximum of the samples per pixel. This is done in Audacity for example: https://manual.audacityteam.org/man/audacity_waveform.html |
Good find! We can do that in another PR. 👍 |
I drastically improved sample drawing speeds in #5927 by around 5x-10x, so the changes in this PR should be viable now. However, the fade shapes provided are incorrect for the purpose of crossfading between uncorrelated audio signals. Using them will result in a 3 dB dip in volume partway through the fade. Nearly every DAW gets this wrong and LMMS shouldn't be one of them. I explain this in a video here: https://youtu.be/-5cB3rec2T0 Taking the square root of any constant-gain crossfade will result in one that is constant-power. I recommend doing this with the raised cosine fade so there's no slope discontinuity at the highest point in the fade. I also heavily recommend making this the default shape, as is currently the case in REAPER and should be the case in every DAW. You can verify it was done correctly by fading between two white noise sources and verifying that there isn't a 3 dB dip in RMS in the middle of the fade. You can also verify this by ensuring the power (square) of each fade function adds up to a flat line, as shown: |
This PR introduces a way to conveniently apply fades to samples like in other DAWs. This is a work in progress and if there is interest in the feature, I will continue to work on this to get it ready to use. I can also think of some possible extensions:
It is planned to add crossfading
Current State:

Original State:
