Skip to content

do not adjust beatloop_size when pressing beatlooproll_X_activate#1428

Closed
Be-ing wants to merge 3 commits intomixxxdj:mainfrom
Be-ing:keep_beatloop_size_with_rolling_loop_buttons
Closed

do not adjust beatloop_size when pressing beatlooproll_X_activate#1428
Be-ing wants to merge 3 commits intomixxxdj:mainfrom
Be-ing:keep_beatloop_size_with_rolling_loop_buttons

Conversation

@Be-ing
Copy link
Copy Markdown
Contributor

@Be-ing Be-ing commented Dec 20, 2017

// Disregard existing loops.
m_pSlipEnabled->set(1);
slotBeatLoop(pBeatLoopControl->getSize(), false, true);
slotBeatLoop(pBeatLoopControl->getSize(), false, true, false);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this requires some explainations, and the related use case.
Why you give a beat loop size on one hand an on the other hand the not adjust flag.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The beatlooproll_X_activate buttons are momentary use whereas beatloop_size is a more enduring value. @ronso0 reported it was unexpected and annoying that beatlooproll_X_activate changed beatloop_size and I think that makes sense.

if (!m_pTrack || samples == 0
|| !m_pBeats) {
if ((!m_pTrack || samples == 0 || !m_pBeats) && adjustBeatloopSizeCO) {
clearActiveBeatLoop();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it correct to skip this as well?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, no it is not. I'll just make the following line conditional for adjustBeatloopSizeCO.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Dec 21, 2017

Ready to test?

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Dec 21, 2017

Yes.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Dec 21, 2017

Just tested this:

  1. it's not changing beatlooop_size in the spinbox 👍
  2. 1/16 beatloop_roll only worked in 50% of tests, else it would just keep playing normally with an active loop in the waveforms 👎
  3. when using a rolling beatloop it would highlight the loop button related to loop size in the spinbox, not the one of actual beatloop_roll size 😕 (Tango with beatloop grid open)
  4. using beatloop_roll would leave it's loop_in/_out markers, so reloop would activate that rolling beatloop

3 would be nice-to-have
4 probably won't be solved here, this would require handling multiple loops

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Dec 22, 2017

1/16 beatloop_roll only worked in 50% of tests, else it would just keep playing normally with an active loop in the waveforms

sorry, this also happens in master

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Dec 22, 2017

Okay, then this is ready to merge.

when using a rolling beatloop it would highlight the loop button related to loop size in the spinbox, not the one of actual beatloop_roll size 😕 (Tango with beatloop grid open)

That would require adding beatlooproll_X_enabled Controls separate from beatloop_X_enabled, which is beyond the scope of this little PR.

@daschuer
Copy link
Copy Markdown
Member

1/16 beatloop_roll only worked in 50% of tests, else it would just keep playing normally with an active loop in the waveforms

sorry, this also happens in master

Ups, that is probably related to my work here: #1367

@daschuer
Copy link
Copy Markdown
Member

when using a rolling beatloop it would highlight the loop button related to loop size in the spinbox, not the one of actual beatloop_roll size 😕 (Tango with beatloop grid open)

This is IMHO blocking the merge, because we have beatloop roll on the right click of these buttons.

@Be-ing Be-ing added this to the 2.1.0 milestone Dec 27, 2017
@daschuer
Copy link
Copy Markdown
Member

Let's move this from the 2.1 milestone in favour of a consistent temporary rolling loop solution.
That is the requested feature and hacking around this may create other issues.

@daschuer daschuer modified the milestones: 2.1.0, 2.2.0 Mar 22, 2018
@Be-ing Be-ing modified the milestones: 2.2.0, 2.3.0 Jun 23, 2018
@daschuer
Copy link
Copy Markdown
Member

1/16 beatloop_roll only worked in 50% of tests, else it would just keep playing normally with an active loop in the waveforms

Do we have a bug for it?

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Sep 20, 2018

Edit I don't recall this happened with this PR only, happening with master for a few months I think, so No there's no bug report.

@ronso0
Copy link
Copy Markdown
Member

ronso0 commented Sep 20, 2018

I will test this in master again to be sure

@Be-ing Be-ing modified the milestones: 2.3.0, stalled Feb 7, 2019
@Holzhaus
Copy link
Copy Markdown
Member

Hey, #1367 has been merged in the meantime. What's the status of this? Can you resolve the merge conflicts?

@Be-ing
Copy link
Copy Markdown
Contributor Author

Be-ing commented Nov 26, 2019

@Holzhaus I have no plans to work on this soon. If you want to finish it, please go ahead.

@Holzhaus
Copy link
Copy Markdown
Member

Is there anything else to be done except fixing the merge conflicts?

@github-actions
Copy link
Copy Markdown

This PR is marked as stale because it has been open 90 days with no activity.

@github-actions github-actions Bot added the stale Stale issues that haven't been updated for a long time. label Oct 20, 2020
@Be-ing Be-ing changed the base branch from master to main October 23, 2020 23:52
@Be-ing Be-ing marked this pull request as draft December 8, 2020 07:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

stale Stale issues that haven't been updated for a long time.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants