Skip to content

Conversation

liushuyu
Copy link
Member

@liushuyu liushuyu commented Jun 4, 2017

This change is Reviewable

ivhacks and others added 30 commits April 8, 2017 15:34
Add 2017 kicker competition winner to demo projects.
… SF2 plugin directory when running LMMS from build directory without install. (LMMS#3502)
…MS#3503)

* Fixed LMMS#3182. Always using master channel for preset previews.
* Dual Filter

* Design Tweaks
The Delay plugin had an issue with the delay knob having the incorrect max value,
this was resulting in incorrectly scaled times
This has been corrected.
EQ plugin now responds to wet / dry control
* Make factory samples relative
Fixes LMMS#3491
Related LMMS#1719
m_bufferLastUpdated is now correctly set to the current frame upon updating
the buffer.
LFO controller now has correct frequency with multiple connections.
Add a new value of "24 Bit Float" to the "Depth" combo box in the
project export dialog.

Add a new enum value to ProjectRenderer::Depth and extend the evaluation
of the different enum values in ProjectRenderer.

Add the new case of a depth of 24 to AudioFileWave and remove some
repetition with regards to SF_FORMAT_WAV in the code. It's only set once
now and then the depth is "added" in a switch statement.
Pull the class OutputSettings out of the class ProjectRenderer so that
it can be used in other contexts as well. Also move the enum
ProjectRenderer::Depth into the new class OutputSettings and rename it
to BitDepth. Adjust all places that referenced
ProjectRenderer::OutputSettings accordingly.

Adjust the two places where an instance of OutputSettings is created:
the main function and ExportProjectDialog::startExport.

Store an instance of OutputSettings in AudioFileDevice and remove
several members and methods which are now replaced by this instance. Add
a getter for the OutputSettings to AudioFileDevice. Storing an instance
of OutputSettings in the base class AudioFileDevice enables the
simplification of the following constructors and general code in the
following classes:
* AudioFileDevice
* AudioFileOgg
* AudioFileWave

Because OutputSettings contains everything related to sample rate, bit
rate settings and bit depth these parameters could be removed from the
parameter list of the aforementioned constructors.

Simplify the signature of the factory method AudioFileDeviceInstantiaton
(defined in AudioFileDevice.h) and reorder the parameters by significance.

Move the logic of how the minimum and maximum bitrate is calculated
using the nominal bitrate into AudioFileOgg::minBitrate() and
AudioFileOgg::maxBitrate(). Previously this was defined in the
constructor of ProjectRenderer where it does not belong as it an
implementation detail of the OGG export.

Remove the code that converted the bit depth enum to an integer from
ProjectRenderer as it is now solely represented as an enum.

Remove class members for the minimum and maximum bit rate from
AudioFileOgg and adjust the code in the implementation to use the values
stored in OutputSettings.
Only show widgets on the export dialog that are relevant to the selected
file format (Wave/Ogg):
* Sample rate is always shown.
* Bit depth settings are only shown when Wave is selected.
* Bit rate settings are only shown when Ogg is selected.

Remove the label that informs the user that not all settings apply to
all export formats as it is not needed anymore. The english text of that
label was: "Please note that not all of the parameters above apply for
all file formats."
If the variables bit rate is not enabled the nominal bit rate will be
used for the minimum and maximum bit rate in the encoder.

If the variable bit rate is enabled the current implementation will
compute the minimum bitrate by subtracting 64 kBit/s from the nominal
bit rate. The maximum bit rate is computed by adding 64 kBit/s to it.

Example: The nominal bit rate is set to 160 kBit/s and variable bit rate
is enabled in the export dialog. The minimum bit rate is then set to 96
kBit/s and the maximum bit rate to 224 kBit/s.
…-For-1.2

24 Bit WAV export, variable bit rate Ogg and export dialog improvements
Provide support for fallback config values

Makes autosave and some other values checked by default.  Supersedes LMMS#3541
* ReverbSC: Method to change samplerate (LMMS#3401)

* ReverbSC: added mutex for protected malloc

* ReverbSC: small CMake fix to remove warning message

* ReverbSC: samplerate changed to uint32_t. more CMakeFile tweaks

* Fix dc block on oversampling
This is where we want new users directed to.
The previous delay code was incorrectly not utalising the whole buffer, causing glitches when
incressing the delay time, due to outputting incorrect data, This was apparent when using the
lfo in the Delay and Flanger plugins.

This has been rectified. The read index is now offset from the write index. and the complete buffer is used in a circular fashon.

Flanger - resolved issue where the lfo could create negative delay lengths
The LFO rate was not correctly syncronising to tempo

This has been rectifited, to utalise the TempoSyncKnob as intended, returning a period,
instead of a frequency. The knob now reports the correct values in the GUI.

Flanger LFO maximum period incressed to 60 seconds
@karmux
Copy link
Contributor

karmux commented Jul 10, 2017

@PhysSong what's the issue still?

There were only 4 conflicting files in this merge:

CONFLICT (content): Merge conflict in src/CMakeLists.txt
CONFLICT (content): Merge conflict in plugins/vst_base/CMakeLists.txt
CONFLICT (content): Merge conflict in plugins/ReverbSC/CMakeLists.txt
CONFLICT (content): Merge conflict in .travis/osx..install.sh

Instead creating new PR and starting everyting over the remaining conflict should be resolved here.

@liushuyu
Copy link
Member Author

liushuyu commented Jul 11, 2017

@PhysSong what's the issue still?

Well, I think @PhysSong wants to do a cleaner and correct merge from the ground up. It's harder though, but I suppose it could have a much cleaner history graph

@tresf
Copy link
Member

tresf commented Jul 11, 2017

Well, I think. @PhysSong wants to do a cleaner and correct merge from the ground up. It's harder though, but I suppose it could have a much cleaner history graph

@PhysSong please do. 👍 You may use @liushuyu's PR as a baseline for comparison and then we can close this out.

@PhysSong
Copy link
Member

@liushuyu Could you tell me how did you make and add commits?

@liushuyu
Copy link
Member Author

@liushuyu Could you tell me how did you make and add commits?

Do you want to rework the merge and start everything from the ground up or just fix mine? The procedures can be different

@PhysSong
Copy link
Member

@tresf Which merge method should be used? If I know that, I'll check and do some tests with it.

@liushuyu
Copy link
Member Author

@tresf Which merge method should be used? If I know that, I'll check and do some tests with it.

I personally recommend regular 3 way git merge in this case. Using rebase will mess up everything ( 'cause i have tried)

@tresf
Copy link
Member

tresf commented Jul 11, 2017

@tresf Which merge method should be used? If I know that, I'll check and do some tests with it.

This isn't my area of expertise. Please feel free to try a few things out.

@PhysSong
Copy link
Member

I tried both git merge and git rebase,

git checkout -b new_master stable-1.2
git rebase master

gave me the best result.
However, It will make branch comparison in Github like this: This branch is 105 commits ahead, 69 commits behind LMMS:stable-1.2.
I saw Github says @liushuyu's merge branch is 48 commits ahead, 5 commits behind LMMS:stable-1.2. @liushuyu Please tell me how did you do.

@liushuyu
Copy link
Member Author

PhysSong Which branch did you take as base branch? It should be master

@liushuyu
Copy link
Member Author

And @PhysSong please use git merge in this case, rebase will override the history making GitHub think there are new commits added

@PhysSong
Copy link
Member

@liushuyu Yes. git rebase has the problem you said.
If I use git merge, however, git fails to handle cherry-picked commits properly. See this for more details.

As I know, any of these methods doesn't generate correct and clean commit histories(unless someone force-push commits into master - definitely not good).

@liushuyu liushuyu force-pushed the merge branch 2 times, most recently from 3860f68 to a76e5b3 Compare July 11, 2017 15:10
@liushuyu
Copy link
Member Author

Problem solved.
@PhysSong Please review the merge again and see if there's something worth noting

@PhysSong
Copy link
Member

@liushuyu Seems good, except three comment changes in #3614 isn't included.
It isn't necessary, but including them would be better.

@liushuyu
Copy link
Member Author

@liushuyu Seems good, except three comment changes in #3614 isn't included.
It isn't necessary, but including them would be better.

It's included, but a new separate commit was not created for the merge.

@PhysSong
Copy link
Member

@liushuyu The include directory change in 242d80d is included. However, Three comment changes aren't.

@liushuyu
Copy link
Member Author

@liushuyu The include directory change in 242d80d is included. However, Three comment changes aren't.

Ah. Okay, so you think it's very vital to include those changes as well?

Let's just merge it earlier if there's no other problems that affect the actual functionality to avoid merge the **** over and over and over and over again

@PhysSong
Copy link
Member

@liushuyu Okay. Merging it now and editing those lines later would be better. Now it seems to be fine.

@liushuyu
Copy link
Member Author

Manually push my branch directly to master, avoid using GitHub merge buttons to prevent from further f*** up.

@PhysSong
Copy link
Member

PhysSong commented Jul 12, 2017

@liushuyu I agree. Direct pushing would be better than pressing GitHub merge button.
@tresf Could you do this?

@PhysSong
Copy link
Member

Some commits are added after your merge, so direct pushing is improper now.
@liushuyu I did a new merge based on yours. If you don't mind, @tresf use this.

@tresf
Copy link
Member

tresf commented Jul 12, 2017

If you don't mind, @tresf use this.

That looks very clean. I'm leaving the merging up to you guys though, I hope that's OK.

@PhysSong
Copy link
Member

I pushed cc38221, a new merge.
@liushuyu Since it is hard to give you credit directly, I mentioned you in the commit message.

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.