Skip to content

Rendering looped sections multiple times on export (#4624)#4639

Merged
zonkmachine merged 8 commits into
LMMS:masterfrom
StevenChristy:feature-4624
Jan 27, 2019
Merged

Rendering looped sections multiple times on export (#4624)#4639
zonkmachine merged 8 commits into
LMMS:masterfrom
StevenChristy:feature-4624

Conversation

@StevenChristy

Copy link
Copy Markdown
Contributor

@tresf suggested in #4624 that this feature might be useful for inclusion in LMMS.

This pull request adds the ability to render looped sections multiple times when exporting. A spinbox is added to the export dialog that allows one to specify how many times the looped section would be rendered while exporting. This defaults to 1 but the user can specify a value to allow the section between looping markers to be rendered up to 9 times (change the spinbox maximum in export_project.ui to increase this if a greater number than 9 is desired).

Feature was tested with both Export Project and Export Tracks but is not applicable to Export to MIDI. The method for calculating export progress needed to be changed to allow the progress bar to give a correct estimate for the percentage completion. There are no known limitations or side effects other than what is described above.

@LmmsBot

LmmsBot commented Sep 30, 2018

Copy link
Copy Markdown

Downloads for this pull request

Generated by the LMMS pull requests bot.

@PhysSong

PhysSong commented Oct 6, 2018

Copy link
Copy Markdown
Member

I did some simple tests. It worked well even with automated time signatures. 🎉
However, I think you should try to address our coding convention. There are some codes which don't agree with our coding conventions. Please let me know if you want to know what to change.
For GUI changes, I'd ask @Umcaruje and @RebeccaDeField for reviews.

@StevenChristy

Copy link
Copy Markdown
Contributor Author

@PhysSong, thanks. I haven't had a chance to address this yet, but I noticed that the existing code doesn't seem to follow the coding conventions you stated. At the risk of being a little too snarky, I think its worth mentioning that if the existing code doesn't follow the conventions that the project doesn't really have a convention... However, I will make the changes you've requested.

@Umcaruje / @RebeccaDeField, please review the GUI changes. Thank you.

@StevenChristy

Copy link
Copy Markdown
Contributor Author

Here is a screenshot in case it helps:
lmms_multiloop_export

@StevenChristy

Copy link
Copy Markdown
Contributor Author

@PhysSong, the whitespace changes you requested were addressed. I also corrected one inconsistency in the .ui file that didn't appear to me to do much, but might have been important for other platforms.

@zonkmachine

zonkmachine commented Jan 6, 2019

Copy link
Copy Markdown
Contributor

Testing the AppImage above. It looks like the code has been changed since so I'll try and compile this later. I think this works really well and looks good.

up to 9 times (change the spinbox maximum in export_project.ui to increase this if a greater number than 9 is desired).

This is too little. There is room for more there so think big. I saw one user complaining about issues with tracks of the size 2h and longer. One may want to render a long drone for a hippie convention etc. I'd go with 9999 999 as max. :)

@zonkmachine

zonkmachine commented Jan 19, 2019

Copy link
Copy Markdown
Contributor

This is good to merge. I haven't gone over the code with a magnifying glass but it looks good.

Some thoughts.

  • 9 times is too little for number of times to loop. At least 999 times is the way to go.
  • You could show time of track to be exported as a function of 'time of loop' * 'number of times'. This would be good to know when looping many times.

@zonkmachine

Copy link
Copy Markdown
Contributor

Merge?

@zonkmachine

Copy link
Copy Markdown
Contributor

change the spinbox maximum in export_project.ui to increase this if a greater number than 9 is desired

Done. Bumped it to 99.

@zonkmachine

Copy link
Copy Markdown
Contributor

@StevenChristy Merged, thanks for implementing this!

Comment thread src/core/Song.cpp
m_playPos[Mode_PlaySong].m_timeLine->loopBegin() : MidiTime(0,0);
m_exportLoopEnd = m_playPos[Mode_PlaySong].m_timeLine->loopBegin() < m_exportSongEnd &&
m_playPos[Mode_PlaySong].m_timeLine->loopEnd() <= m_exportSongEnd ?
m_playPos[Mode_PlaySong].m_timeLine->loopEnd() : MidiTime(0,0);

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 found that m_timeLine is null if LMMS runs without GUI. As a consequence, this breaks the command-line rendering.
I think we can work around this by not setting m_exportLoopBegin and m_exportLoopEnd when the timeline is null because we don't use that in CLI renders.

FYI, that check is not needed elsewhere because LMMS doesn't support "export between loop markers" feature on the command line(for a similar reason).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants