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

Fixes issue #2129 ("can't export as loop with CLI") #2131

Merged
merged 1 commit into from Aug 12, 2015

Conversation

@michaelgregorius
Copy link
Contributor

@michaelgregorius michaelgregorius commented Jun 23, 2015

Adds a new command line option to render a song as a loop ("-l", "--
loop").

Also cleaned up the code which parses the command line options by
pulling out methods that print the version and the help.

@michaelgregorius michaelgregorius changed the title Fixes issue 2129 ("can't export as loop with CLI") Fixes issue #2129 ("can't export as loop with CLI") Jun 23, 2015
@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jun 23, 2015

Could you please update the man page and include this feature in it? I think this should be documented 🍷

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 23, 2015

Done. I have also added lmms.io and the github page as URLs and have updated the date of the man page.

@tresf
Copy link
Member

@tresf tresf commented Jun 23, 2015

Also, if you don't mind adding another change, I never put this feature into the man pages either: #1784

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jun 23, 2015

These old sf links are invalid and should be removed.

I changed the sf links in #2124

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 23, 2015

@tresf I'd prefer if you document #1784 because you know better what that feature is about. Thanks!

@tresf
Copy link
Member

@tresf tresf commented Jun 23, 2015

Fair enough, although it can be copied directly from the help if you change your mind. :)

https://github.com/LMMS/lmms/blob/master/src/core/main.cpp#L198

@tresf
Copy link
Member

@tresf tresf commented Jun 23, 2015

I changed the sf links in #2124

NP, we'll just hit a merge conflict on one of the PR's then and we can fix it then.

@Umcaruje
Copy link
Member

@Umcaruje Umcaruje commented Jun 23, 2015

NP, we'll just hit a merge conflict on one of the PR's then and we can fix it then.

👍

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 23, 2015

I have changed my mind. :)

The allowroot option is now also documented in the man page.

@Wallacoloo
Copy link
Member

@Wallacoloo Wallacoloo commented Jun 23, 2015

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 23, 2015

@Wallacoloo Exporting as a loop means that the full song is rendered but only till the end of the song. The intention of this feature is to be able to loop the exported file seamlessly. This means that for example reverb tails that you normally want to have included in your rendering will not be rendered.

@Wallacoloo
Copy link
Member

@Wallacoloo Wallacoloo commented Jun 24, 2015

@michaelgregorius gotcha. In GUIs, I recall seeing this option presented as a drop-down menu labeled "loop mode" with options "wrap remainder" (which takes any nonzero output after the end of the song and wraps it around back to the start by adding it into the output at the beginning of the song), "truncate remainder" (I.e. your implementation) and "render remaining".

Given that those are 3 mutually-exclusive options, I, personally, think it makes more sense to implement the CLI option as "--loop-mode=X", even if we only implement the truncate and render modes for now.

What are your thoughts on that?

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 24, 2015

@Wallacoloo I assume that implementing the wrapping option that you have proposed would need quite some effort. I don't have that much time (and also my own sideprojects) and therefore would like to propose to merge the changes so far and open a separate issue for your proposal. 😃

I have also tried to provide a command line option for the "Export between loop markers" option but unfortunately doing so in a non-GUI context makes the application crash with the following stack:

0   MidiTime::operator int  MidiTime.h  114 0x4d6ba8    
1   TimeLineWidget::loopBegin   TimeLineWidget.h    91  0x545603    
2   Song::startExport   Song.cpp    650 0x5415da    
3   ProjectRenderer::run    ProjectRenderer.cpp 162 0x53510c    
4   ??          0x7ffff681192c
5   start_thread            0x7ffff7bc5354  
6   clone           0x7ffff434ebfd  

The problem is that Song::startExport accesses the TimeLineWidget of the PlayPos instance which is 0 because we are in a non-GUI context. Yet another example that LMMS really needs to have its layers separated (which is no small feat I know).

@tresf
Copy link
Member

@tresf tresf commented Jun 24, 2015

@Wallacoloo I assume that implementing the wrapping option that you have proposed would need quite some effort.

From what I read, his proposition was to make the command line option configurable now by taking a parameter, so that when we do add this wrap-around feature, we'll have less changes to the CLI.

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 24, 2015

[...] even if we only implement the truncate and render modes for now.

Sorry, I somehow missed that part. :)

So the only option for now would be something like --loop-mode=truncate? By the way, currently there is no other option that uses =. So to be consistent with the other options I assume it would rather be -l truncate and --loop-mode truncate?

lmms.1 Outdated
.IP "\fB\-h, --help
Show usage information and exit.
.SH SEE ALSO
.BR http://lmms.io/
.BR https://github.com/LMMS/lmms
.BR http://lmms.sf.net/
.BR http://lmms.sf.net/wiki/

This comment has been minimized.

@Wallacoloo

Wallacoloo Jun 25, 2015
Member

Any reason to keep http://lmms.sf.net around even though it redirects to https://lmms.io, which is also listed?

@Wallacoloo
Copy link
Member

@Wallacoloo Wallacoloo commented Jun 25, 2015

@michaelgregorius the proposal would be for --loop-mode=truncate and --loop-mode=render (the implied default when no --loop-mode flag is given). You are right that no options currently use =, so yes, --loop-mode truncate, --loop-mode render would likely be best.

However, --loop-mode render [...] is disappointingly close to --loop-mode --render [...], which could create confusion. On the other hand --loop-mode render-remainder is a bit verbose. So, I don't know. Maybe there's a better phrase than "render remainder". Maybe just --loop-mode none or --loop-mode disabled would do.

It does seem like there isn't much use of this --loop-mode <x> switch until (and if) we add support for more looping modes, like wrap-around though. So it should be perfectly fine to just go with -l and --loop-mode for now, and in the future, we can expand the command to -l [mode] and --loop-mode [mode] if we ever need to, in which mode defaults to truncate, thus preserving full backwards-compatibility :-)

So yeah, I think you can pretty much disregard most of what I said then. The code looks good. Thanks for separating out the help/version printing into their own functions! I left an inline comment in the only place that I had a suggestion.

@tresf
Copy link
Member

@tresf tresf commented Jun 25, 2015

@Wallacoloo the sf links will be remove by @Umcaruje's PR per #2131 (comment), #2131 (comment)

@Wallacoloo
Copy link
Member

@Wallacoloo Wallacoloo commented Jun 25, 2015

@tresf oops. Sorry for not reading through this thread all the way :/

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jun 25, 2015

I have pushed another commit that changes the long option from --loop to --loop-mode and have also adjusted the documentation accordingly.

Concerning the options for the loop mode: do I understand correctly that --loop-mode render would just render the song without any loops? If that's the case I'd propose to drop this case altogether and to render the complete song if the there is no -l or --loop-mode option given. The loop option would then only describe cases where you really want to render loops.

@Wallacoloo
Copy link
Member

@Wallacoloo Wallacoloo commented Jun 25, 2015

void printHelp()
{
printf( "LMMS %s\n"
"Copyright (c) 2004-2014 LMMS developers.\n\n"

This comment has been minimized.

@8tab

8tab Jul 25, 2015
Contributor

s/2014/2015/

void printVersion(char *executableName)
{
printf( "LMMS %s\n(%s %s, Qt %s, %s)\n\n"
"Copyright (c) 2004-2014 LMMS developers.\n\n"

This comment has been minimized.

@8tab

8tab Jul 25, 2015
Contributor

s/2014/2015

@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Jul 27, 2015

The second year of the copyright is now always calculated dynamically so there will be no more need to adjust it manually in the future.

@tresf
Copy link
Member

@tresf tresf commented Aug 12, 2015

@michaelgregorius @Wallacoloo is there a reason we can't rebase, squash and commit this?

@tresf tresf mentioned this pull request Aug 12, 2015
@Wallacoloo
Copy link
Member

@Wallacoloo Wallacoloo commented Aug 12, 2015

@tresf This looks fine - green light from me.

@tresf
Copy link
Member

@tresf tresf commented Aug 12, 2015

@Wallacoloo 👍

@michaelgregorius once this is squashed into a single commit and rebased against master, we'll merge this in. Thanks for the hard work. :)

Adds a new command line option to render a song as a loop ("-l", "--
loop-mode").

Also cleaned up the code which parses the command line options by
pulling out methods that print the version and the help.

Updated man page: Added the new option to command line render a loop. Updated
the data of the man page and the URLs.

Added information about option to bypass root user check on startup

Calculate the copyright year dynamically

The command line options for help and version info both print the
copyright as "2004-2014". Until now the value for the second year had to
be adjusted manually. With this patch they are computed dynamically so
that the current year will always be used.
@michaelgregorius michaelgregorius force-pushed the michaelgregorius:cmd-loop-render branch from d8bcc28 to 02f9447 Aug 12, 2015
@michaelgregorius
Copy link
Contributor Author

@michaelgregorius michaelgregorius commented Aug 12, 2015

@tresf Done! 😃

@tresf
Copy link
Member

@tresf tresf commented Aug 12, 2015

Travis is happy... merging. 👍

tresf added a commit that referenced this pull request Aug 12, 2015
Fixes issue #2129 ("can't export as loop with CLI")
@tresf tresf merged commit 9819900 into LMMS:master Aug 12, 2015
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@michaelgregorius michaelgregorius deleted the michaelgregorius:cmd-loop-render branch Aug 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.