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

Improved metronome (on/off during song, pattern and bb playback) #2278

Merged
merged 1 commit into from Aug 29, 2015

Conversation

Projects
None yet
4 participants
@michaelgregorius
Contributor

michaelgregorius commented Aug 17, 2015

There is a new tool button that can be used to turn the metronome on and
off. Per default the metronome is turned off. When enabled the metronome
will play during on song playback, pattern playback and BB playback. During
export it is ignored.

A new icon was added as well.

The state is currently stored in the Mixer. It might make sense to put
the metronome configuration in its own class in the future. The state is
currently not stored in the file but this might be a good choice for now
until a better place is found for the metronome data.

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Aug 17, 2015

Contributor

I saw in the forum that there seems to be a need for an improved metronome functionality so I thought to give it a first shot. :)
metronome-icon

Also related to #475.

Contributor

michaelgregorius commented Aug 17, 2015

I saw in the forum that there seems to be a need for an improved metronome functionality so I thought to give it a first shot. :)
metronome-icon

Also related to #475.

@tresf

This comment has been minimized.

Show comment
Hide comment
@tresf

tresf Aug 17, 2015

Member

Well need addition!

Also related: #869 (@musikBear), #1357 (@Reaper10)

Member

tresf commented Aug 17, 2015

Well need addition!

Also related: #869 (@musikBear), #1357 (@Reaper10)

Show outdated Hide outdated src/core/Mixer.cpp
if( ( currentPlayMode == Song::Mode_PlayPattern || currentPlayMode == Song::Mode_PlaySong
|| currentPlayMode == Song::Mode_PlayBB ) &&
m_metronomeActive &&
!Engine::getSong()->isExporting() &&
p != last_metro_pos )
{

This comment has been minimized.

@Wallacoloo

Wallacoloo Aug 28, 2015

Member

What do you think of decomposing the conditional a bit? Maybe something like:

bool isPlaying = currentPlayMode == Song::Mode_PlayPattern || currentPlayMode == Song::Mode_PlaySong
         || currentPlayMode == Song::Mode_PlayBB;
if (isPlaying && m_metronomeActive && !Engine::getSong()->isExporting() &&
        p != last_metro_pos) ...

Hope I'm not being too pedantic. Everything else looks good.

@Wallacoloo

Wallacoloo Aug 28, 2015

Member

What do you think of decomposing the conditional a bit? Maybe something like:

bool isPlaying = currentPlayMode == Song::Mode_PlayPattern || currentPlayMode == Song::Mode_PlaySong
         || currentPlayMode == Song::Mode_PlayBB;
if (isPlaying && m_metronomeActive && !Engine::getSong()->isExporting() &&
        p != last_metro_pos) ...

Hope I'm not being too pedantic. Everything else looks good.

@Wallacoloo

This comment has been minimized.

Show comment
Hide comment
@Wallacoloo

Wallacoloo Aug 28, 2015

Member

Tested & works as described (via git checkout michaelgregorius/metronome-improvements && git rebase master && make install).

I've left one code comment for @michaelgregorius to address. Pending further feedback, this should be good to merge.

Member

Wallacoloo commented Aug 28, 2015

Tested & works as described (via git checkout michaelgregorius/metronome-improvements && git rebase master && make install).

I've left one code comment for @michaelgregorius to address. Pending further feedback, this should be good to merge.

@musikBear

This comment has been minimized.

Show comment
Hide comment
@musikBear

musikBear Aug 29, 2015

@michaelgregorius Big 👍 The metronome has been one of the most criticized features, and a toggle options is brilliant. The Master's two different sounds, and yours toggle-option, makes metronome a really good feature in lmms!

musikBear commented Aug 29, 2015

@michaelgregorius Big 👍 The metronome has been one of the most criticized features, and a toggle options is brilliant. The Master's two different sounds, and yours toggle-option, makes metronome a really good feature in lmms!

Improved metronome (on/off during song, pattern and bb playback)
There is a new tool button that can be used to turn the metronome on and
off. Per default the metronome is turned off. When enabled the metronome
will during on song playback, pattern playback and BB playback. During
export it is ignored.

A new icon was added as well.

The state is currently stored in the Mixer. It might make sense to put
the metronome configuration in its own class in the future. The state is
currently not stored in the file but this might be a good choice for now
until a better place is found for the metronome data.

Also removed some repeated calls to Engine::getSong() and
Engine::fxMixer().
@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Aug 29, 2015

Contributor

@Wallacoloo I have incorporated your proposal for a decomposition of the conditional. I have also rebased against master and squashed the commit.

Contributor

michaelgregorius commented Aug 29, 2015

@Wallacoloo I have incorporated your proposal for a decomposition of the conditional. I have also rebased against master and squashed the commit.

@michaelgregorius

This comment has been minimized.

Show comment
Hide comment
@michaelgregorius

michaelgregorius Aug 29, 2015

Contributor

@musikBear Thanks for the kind words! Knowing that a feature is wanted by users and therefore is also used is always a great motivation. 😃

Contributor

michaelgregorius commented Aug 29, 2015

@musikBear Thanks for the kind words! Knowing that a feature is wanted by users and therefore is also used is always a great motivation. 😃

@Wallacoloo

This comment has been minimized.

Show comment
Hide comment
@Wallacoloo

Wallacoloo Aug 29, 2015

Member

👍

Member

Wallacoloo commented Aug 29, 2015

👍

Wallacoloo added a commit that referenced this pull request Aug 29, 2015

Merge pull request #2278 from michaelgregorius/metronome-improvements
Improved metronome (on/off during song, pattern and bb playback)

@Wallacoloo Wallacoloo merged commit 45c4aa6 into LMMS:master Aug 29, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@michaelgregorius michaelgregorius deleted the michaelgregorius:metronome-improvements branch Aug 29, 2015

@musikBear musikBear referenced this pull request Mar 6, 2018

Open

Better metronome #475

1 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment