Skip to content
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

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

Merged
merged 8 commits into from
Jan 27, 2019

Conversation

StevenChristy
Copy link
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
Copy link

LmmsBot commented Sep 30, 2018

Downloads for this pull request

Generated by the LMMS pull requests bot.

@PhysSong
Copy link
Member

PhysSong commented Oct 6, 2018

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
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
Contributor Author

Here is a screenshot in case it helps:
lmms_multiloop_export

@StevenChristy
Copy link
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
Copy link
Member

zonkmachine commented Jan 6, 2019

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
Copy link
Member

zonkmachine commented Jan 19, 2019

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
Member

Merge?

@zonkmachine
Copy link
Member

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
Member

@StevenChristy Merged, thanks for implementing this!

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
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