LMMS / lmms

Cross-platform music production software
https://lmms.io
GNU General Public License v2.0
8.12k stars 1k forks source link

Playback Should Not Halt when 'Open Project' Dialog is Opened #1384

Closed csimons closed 9 years ago

csimons commented 9 years ago

As of latest commit (1d929d6ce7b5426b2ad3d0f56bf559d7e717f099 at the time of writing), opening the 'Open project' dialog causes playback to stop. This does not happen when the 'Save project' or other similar dialogs are opened.

tresf commented 9 years ago

I like this idea. I've found myself wishing this were the functionality.

Pull request, for reference: #1385

softrabbit commented 9 years ago

Which one is a bigger annoyance, a stuck buffer sound for some or playback going silent for all? See commit e852c8b26771a11dd4c5a738f2b8fe6dec5c558e:

Users reported the last buffer played repeatedly while the dialog asking whether to save project is shown. This behaviour even continued while loading a project. An easy workaround for this is to stop playing song (or whichever TrackContainer is active at the moment) in MainWindow::mayChangeProject().```

Not that it's entirely fixed, I get that buffer sound for some reason when I open a recent project in 1.0.98 on Windows Vista 32-bit, using SDL... :(

csimons commented 9 years ago

Which one is a bigger annoyance

I would think stopping the playback would be the bigger problem. If someone is playing a show out of LMMS and opens a dialog and the whole song stops, the music stops and the user has to scramble to set the cursor where it was before and start playback again.

See commit e852c8b26771a11dd4c5a738f2b8fe6dec5c558e

Playback seems to continue fine when the Save/Import/etc. dialogs are open, which shouldn't behave any differently, and I don't hear any problems when the "Open project" dialog is open with this patch in place (tested with some of the sample projects included in the distribution).

If some kind of weird "looping" of a short segment is happening when the dialog is open, I would think this should be considered a separate bug, one to which the solution surely isn't to just stop all playback every time a dialog is opened.

diizy commented 9 years ago

On 12/03/2014 10:23 AM, Christopher L. Simons wrote:

Which one is a bigger annoyance

I would think stopping the playback would be the bigger problem. If someone is playing a show out of LMMS

This is outside the scope. Live play is currently not a design goal nor priority for LMMS.

tresf commented 9 years ago

Users reported the last buffer played repeatedly while the dialog asking whether to save project is shown. This behaviour even continued while loading a project. An easy workaround for this is to stop playing song (or whichever TrackContainer is active at the moment) in MainWindow::mayChangeProject().

Ah yes.... I remember this one... :) Yeah, that explains the inconsistency right there. I'm curious though if we're just stopping it at the wrong time. Is there a natural waiting period which makes the stop more pleasing to the ears when it's done prior to showing the open dialog? It seems we could move the stop to come after the decisive action of opening a new track... assuming the code permits that as easily.

musikBear commented 9 years ago

quite rarely, an attempt to open a new project, as the current in in playback, causes lmms to studder and then hang. imo, when a user choose 'open', then he is fed up with whats currently playing, and it is intuitive, to stop current playback. The pro here is that the studder-> hang, never would happen. Imo that favors playback-stop, when any IO dialog is chosen. (Could imo include save, where studder also can occour)

Sti2nd commented 9 years ago

This is outside the scope. Live play is currently not a design goal nor priority for LMMS.

Ehm, for you maybe.

If someone is playing a show out of LMMS and opens a dialog and the whole song stops

But with dialog you only mean the open project dialog @csimons ? That is hardly any dialog? It would be a real show stopper if editing a track name or double clicking a knob stopped playback. I am with you on this one, though, I think that when you have a powerful enough computer it wouldn't lag at all and thus stop the song because of other users with other systems... strange

csimons commented 9 years ago

That is hardly any dialog?

This implied the consensus that all file-related dialogs should behave the same way with respect to (not) halting playback; right now they're inconsistent in this regard but hopefully at the end of the discuss we can at least agree to make them behave consistently.

causes lmms to studder and then hang

It looks like the halting of playback then was a kind of workaround to prevent this. Based on this and the observed dissatisfaction from those who've weighed in, I'd propose one of the following:

(1) Stop halting playback upon opening the dialog, but find a "proper" fix so that the studder does not happen. We'd hopefully find a way to consistently reproduce the hang, and then could fix it without halting playback.

(2) We could take the approach taken by Audacity. If you play a long track in Audacity and then pop open the File menu, you'll notice that the Open, Save, Save As, etc. options are all disabled during playback. This would resolve both (a) the issue of unexpected halting of playback and (b) the studder/hang issue.

(3) Take Option 2 for now while we try to find consistent reproduction steps to properly fix the root problem from Option 1.

I'd be happy to make the code changes for any of the above as part of the Pull Request that's now open (#1385).

Thoughts?

tresf commented 9 years ago

I'd be happy to make the code changes for any of the above as part of the Pull Request that's now open (#1385). Thoughts?

Firstly, I truly appreciate your methodical approach at consistency here I, I think the project needs much more of this!

But more on topic, I think the audacity comparison is only slightly relevant since our save process doesn't have the tendency to write very large (100MB+) of data to disk. Our .mmpz files generally write extremely quickly (fractions of seconds rather than seconds). Down the road -- when we get the ability to archive a project and all of its samples and presets - this comparison would probably be more accurate, as our IO time would spike, making it difficult to prevent people from making changes mid-save.

I think the biggest argument for stopping playback is the concurrency issues that can be caused by saving while automating, but proper concurrency shouldn't really brake that either, right? :)

-Tres

diizy commented 9 years ago

On 12/03/2014 04:06 PM, Stian Jørgensrud wrote:

This is outside the scope. Live play is currently not a design
goal nor
priority for LMMS.

Ehm, for you maybe.

No. For LMMS. LMMS is not designed for live play - not by a long shot.

It's like, you can go buy a compact city car, and then use it to uproot trees, but then you can't complain to the salesman if your engine overheats, nor is it covered by the warranty. You're using the product in a way it's not intended for, then you're doing it at your own risk.

Sometime in the future when we get the basics fixed, it may be feasible to start adding functionality for live performance into LMMS but it's not a current priority, it can't be because we have to limit our scope according to our available resources.

Sti2nd commented 9 years ago

LMMS is not designed for live play - not by a long shot.

Very true, and I would prefer Lupp, but me for example which only have Windows right now... It doesn't need to be designed for it to use it for that, as you give an example of. Of course, live capabilities shouldn't stop serious development, but personally the lagging wasn't very important to fix, might be that I seldom experience it... :)

csimons commented 9 years ago

I believe the latest commit in PR #1385, 153c8bab10f034a95ce995d970b5c3f35916078e, fixes the original issue while allowing playback to continue while the dialog is open.

It seems that previous to the original workaround commit (e852c8b26771a11dd4c5a738f2b8fe6dec5c558e), the command to stop playback was never given to the song engine, and the Song::loadProject() method immediately started taking actions to load the new track while the existing one was still playing, and a false assumption within that logic (that playback was stopped) likely was the cause of the original studder/hang issue.

tresf commented 9 years ago

:+1: :o)

tresf commented 9 years ago

Hmm... this is a bit uglier to read... but would it be more practical to halt playback a bit more intelligently in that function?

bool MainWindow::mayChangeProject(bool stopPlayback)
{
    //if( stopPlayback )
    //  Engine::getSong()->stop();

    if( !Engine::getSong()->isModified() )
    {
        // ####### HERE #######
        if( stopPlayback )
            Engine::getSong()->stop();
        return( true );
    }

    QMessageBox mb( tr( "Project not saved" ),
                tr( "The current project was modified since "
                    "last saving. Do you want to save it "
                                "now?" ),
                QMessageBox::Question,
                QMessageBox::Save,
                QMessageBox::Discard,
                QMessageBox::Cancel,
                this );
    int answer = mb.exec();

    if( answer == QMessageBox::Save )
    {
        // ####### HERE #######
        if( stopPlayback )
            Engine::getSong()->stop();
        return( saveProject() );
    }
    else if( answer == QMessageBox::Discard )
    {
        // ####### HERE #######
        if( stopPlayback )
            Engine::getSong()->stop();
        return( true );
    }

    return( false );
}
csimons commented 9 years ago

The above code changes existing behavior (not only in the "Open Project" dialog case); the playback would not be stopped if the fourth "return" statement is reached, whereas it would be stopped with the PR as it is now.

If we put the conditional stop there also, the net effect would be the same as it is currently (stop in all cases given stopPlayback=true) except that it would happen after the save prompt rather than before. If this is desired, I'm happy to make the change.

curlymorphic commented 9 years ago

What was the out come of this. I can find the initial pull request that was canceled, i cant seem to locate any follow up.

csimons commented 9 years ago

I got sidetracked with other things and then forgot about this. I still have the patch, so I'll try to create a fresh pull request this evening to the latest branch.

badosu commented 9 years ago

:+1: Please do it :-).

csimons commented 9 years ago

The new PR ticket is #1852.