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

ZynAddSubFX can't create a new instrument bank #4642

Closed
SeleDreams opened this issue Oct 2, 2018 · 12 comments
Closed

ZynAddSubFX can't create a new instrument bank #4642

SeleDreams opened this issue Oct 2, 2018 · 12 comments
Milestone

Comments

@SeleDreams
Copy link
Contributor

SeleDreams commented Oct 2, 2018

I'm posting this issue because this issue that was present since some times in LMMS and was thought fixed #3886 still exists

the steps to reproduce this bug are pretty easy :

  • Starting LMMS on Ubuntu (this bug also exists on the stable 1.1.3 build)
  • Adding a ZynAddSubFX track to the project
  • Creating a new bank

screenshot of the issue :
capture_decran_2018-10-02_10-44-28

@tresf tresf added this to the 1.3.0 milestone Oct 2, 2018
@SeleDreams SeleDreams changed the title Error of ZynAddSubFX when creating a new bank on Ubuntu 18.04 ZynAddSubFX can't create a new instrument bank Oct 2, 2018
@tresf
Copy link
Member

tresf commented Oct 3, 2018

So I'm not sure if this is just an old bug inside ZynAddSubFX (it is, see #4642 (comment)), but when I add a debug line to the method which creates the bank folder, it's attempting to write to a location called ~/banks at precisely this location of code.

Although from a casual observer, the directory ~/banks appears to be a user-writable location, I don't believe ~ is a valid directory as it is interpreted literally as a folder named with tilde character.
For home, I would (instead expect to see a helper function for resolving to $HOME.

Attempts to fix this hard-coded value will initially fail unless .zynaddsubfxXML.cfg is purged because Zyn always prefers the value from the config file so as long as the first element isn't blank.

A temporary workaround is the following:

sed -i -e 's/~\/banks/\/tmp/' ~/Documents/lmms/.zynaddsubfxXML.cfg

This will allow the banks to be written temporarily to /tmp, which is a writable directory.

@musikBear
Copy link

Actually zasfx 'banks' are nothing but a normal folder, with at least one zasfx preset inside. If you make that zasfx will understand it as a bank, and it will be accessible from both zasfx, and indeed lmms -at least in windows....

@tresf
Copy link
Member

tresf commented Oct 3, 2018

@musikBear thanks, but this is about the bug where the folder cannot be created on Linux. When testing, Windows does not suffer this problem.

@tresf
Copy link
Member

tresf commented Oct 3, 2018

Apparently upstream patched this ~ behavior three years ago... :)

@tresf
Copy link
Member

tresf commented Oct 3, 2018

@fundamental can you explain what the permanence of ~/banks should be? For example, if we patch this to use /tmp would there be any ramifications? I ask because even if we cherry-picked zynaddsubfx/zynaddsubfx@ca9ec78, we'd still run into the problem where $HOME/banks/foo's parent $HOME/banks wouldn't exist, right?

@fundamental
Copy link
Contributor

fundamental commented Oct 4, 2018 via email

@tresf
Copy link
Member

tresf commented Oct 4, 2018

Pardon my lack of understanding, I didn't understand if this was something that must be kept persistent. Assuming is is we'll have to find a portable home for it. This means we'll have to write our own mkdir logic. How is it done in modern zyn? How does the plugin get past $HOME/banks not existing?

@fundamental
Copy link
Contributor

fundamental commented Oct 4, 2018 via email

@tresf
Copy link
Member

tresf commented Mar 17, 2019

It appears newbank() does attempt to create the directory. To be consistent with upstream, $HOME/banks is a quick fix with the disadvantage of not being portable (as well as being non-standard, user folders should always be identified by the application name).

#ifdef WIN32
    if(mkdir(bankdir.c_str()) < 0)
#else
    if(mkdir(bankdir.c_str(), S_IRWXU | S_IRWXG | S_IROTH | S_IXOTH) < 0)
#endif

But it's still too late I feel. LMMS already attempts to setup a sane bank location (we set it to the presets location) as long as one of them is empty. Are we too late? We can attempt to overwrite the value of ~/banks if it's found in the config but is this going to break the loading of banks that are built-in? Is this a matter of needing TWO bank locations? First being ones we ship, second being ones we create?

@tresf
Copy link
Member

tresf commented Mar 17, 2019

After re-reading the source, I think it's safe to simply add a call to LocalZynAddSubFx::setPresetDir(...) from within LocalZynAddSubFx::setLmmsWorkingDir(...). Why? Well:

  • Zyn allows 100 bank directories.
  • We already loop over the hard-coded ones before appending.
  • We already have the ConfigManager::inst()->userPresetsDir() + "/ZynAddSubFX" value as part of the call to LocalZynAddSubFx::setLmmsWorkingDir(...).

Edit: Well, unfortunately Zyn is hard-coded to use ~/banks in slot 0 as the only user-writable bank location. 🤔. We'll have to hard-code it as well.

@tresf
Copy link
Member

tresf commented Mar 17, 2019

It appears newbank() does attempt to create the directory.

Correction... no, it doesn't. It DOES create the new bank directory, but not the parent directory. This means on systems without $HOME/banks, zynaddsubfx/zynaddsubfx@ca9ec78 still doesn't fix it.

@fundamental how does Zyn handle this? Is there a startup task in newer versions that we're missing that creates this directory? How does $HOME/banks get created on a new system? This portion of the code appears largely untouched, even in 3.0.

We can still handle this LMMS-side, but we'd be creating a user directory that's only ever used by Zyn. I'd expect Zyn to have the code to create this somewhere since new banks aren't possible without it.

@tresf tresf modified the milestones: 1.3.0, 1.2.0 Mar 18, 2019
tresf added a commit to tresf/lmms that referenced this issue Mar 18, 2019
tresf added a commit that referenced this issue Mar 18, 2019
tresf added a commit that referenced this issue Mar 18, 2019
Allow new Zyn bank creation on Linux
Closes #4642
@tresf
Copy link
Member

tresf commented Mar 18, 2019

Ended up adding an additional mkdir which I still feels is an upstream bug. @JohannesLorenz is investigating if 3.0 has this bug too, but in the meantime, we're patched. This will be patched in 1.2.0-RC9 (or higher). If it doesn't work, try deleting ~/lmms/.zynaddsubfxXML.cfg and trying again.

Ideally, (long-term) we'd like to make these settings portable and store banks somewhere inside <LMMS_WORKING_DIR>, but for now -- as a stop-gap -- we'll at least allow the "New Bank" button to work as it would normally.

Spekular added a commit to Spekular/lmms that referenced this issue Jul 13, 2019
* Allow sub plugins for instruments aswell

* Move m_key member of Effect into Plugin
* Pass key to Instrument ctors and instantiaters
* Add pluginKeys to all plugin selector widgets, and let them pass the keys
  when instantiating the instruments; or, if the keys must be passed over
  threads, pass the keys to the Engine using `Engine::setDndPluginKey()`
* As instrument plugin libraries now also need to get their key passed, their
  second argument, which was always the same as the first, is now used to pass
  the sub plugin keys. This affects *all* instrument plugins.
* Plugin.h: Add more virtuals to `SubPluginFeatures` in order to draw logos
  and images into instrument selector widgets
* LadspaSubPluginFeatures: Implement the `displayName` virtual because the
  new behaviour to resolve displayNames is to first look at the
  SubPluginFeatures, which, without override, returns the superior plugin's
  name (Plugin.cpp)

Additional:

* PluginFactory.h: Allow setting up search paths without discovering plugins
  yet
* Plugin.h: Add full documentation (should be checked)

* Fix Engine.cpp not compiling on some compilers

* Fix or remove wrong or useless debug printfs

* Fix missing IntrumentTrack header

* Use QString for SubPluginFeatures' virtuals

The former virtuals returned `const char*`, which lead to invalid reads when
`LadspaSubPluginFeatures` returned pointers to temporary `QByteArray::data`.

* Test deployment preparation in regular builds  (LMMS#4847)

Tests whether contributions break packaging inadvertently

* Add needed macOS shortcuts (LMMS#4851)

Fix insert bars, delete bars, delete notes on Apple keyboard

* Move apt_mingw_cache out of build directory (LMMS#4842)

* Add missing include

Compilation fails with debug build. Fixes regression from
dd99f3a

* Fix DrumSynth sscanf (LMMS#4869)

* Fix failing build due to missing include

Fix the build of MixHelpers.cpp which fails due to a missing include of
cstdio (printf is not defined).

* Add /sbin to AppImage search path
Closes LMMS#4846

* Fix AudioFileProcessor tooltip (LMMS#4868)

* Disable soundio on macOS

Temporarily disable soundio on macOS to address stability issues with PortAudio
Closes LMMS#4864

* Blacklist $HOME as VST directory
Closes LMMS#4854

* Sync Debian version (LMMS#4840)

* Sync Debian version

* Drop temporary logging

* Install 32-bit RemoteVstPlugin to a separate directory (LMMS#4797)

* Fix loading 32bit VSTs on Linux

Fix a regression in LMMS#4797

* Fix loading 32bit VSTs when loading LMMS in the build directory

Fix another regression in LMMS#4797

* Fix MidiJack crash on exit

* Fix uninitialized m_jackClient being used in MidiJack destructor
* Fix destruction order in Mixer.cpp so that MidiJack doesn't access the
  deleted AudioJack instance

Fixes LMMS#4688

* Fix LMMS#3926: QCursor in AFP

Fix a crash that occurred on the following steps:
1. Add an AFP track.
2. Open it, and move the waveform display to overlap the track label
button.
3. Close the AFP window and open it again by clicking the track label.
4. Move the mouse pointer.

The problem occurs because the code makes the implicit assumption that
AudioFileProcessorWaveView::enterEvent (and hence
QApplication::setOverrideCursor) is called before
AudioFileProcessorWaveView::mouseMoveEvent. This is not the case when
the waveform display is on top of the track label. In this case the AFP
windows is opened with the mouse being immediately positioned over the
wave form display. There is no enter event and move events are issues
directly. This then leads to a crash in
AudioFileProcessorWaveView::mouseMoveEvent when trying to determine the
value for is_size_cursor because the override cursor is still null but
is dereferenced directly without checking.

Only adding a check would not solve the problem because in that case the
cursor would not change to the hand cursor when being moved inside the
waveform display.

The solution is to remove all calls to the global methods
setOverrideCursor and restoreOverrideCursor and to only set the cursor
locally.

This fix is based on a patch by gi0e5b06 which is committed under 8a10c52
in his repository but for which he never created a pull request.

* Fix CONTRIBUTORS override

* Debian integration: update build dependencies

It was wrongly done in 231a840.

* Debian: don't bundle CALF LADSPA library separately anymore

It's LMMS specific now

* Better Wine detection and support

* Support more Wine packagings

* Allow building 64-bit RemoteVstPlugin using 32-bit Wine tools if possible

* Provide suitable library paths for creating AppImages

* Check if winegcc works before building RemoteVstPlugin

* Modify VST build systems to make debian integration work correctly

* Allow creating AppImages on systems newer than linuxdeployqt officially supports

Note that the additional -unsupported-allow-new-glibc switch
may result in an AppImage which is unusable on old systems.

* Support FX Mixer for sample tracks and add controls to sample track window (LMMS#3866)

This work is based on LMMS#3632 by @grejppi.

* Fix zyn pitch on project load/export
Closes LMMS#3451

* Add info about LadspaControls::m_noLink

* Fix compilation on Qt4

* Avoid shallow clones in all Debian sid builds (LMMS#4888)

hallow clone may break version detection. This is fatal in Debian builds, so use full clone.

Note: This is safe for stable-1.2 but needs review after merging to master due to submodules.  See LMMS#4888 for more information.

* Add VST always-on-top config option

* Ensure VST windows show properly in taskbar

* Don't show error when loading empty VeSTige instance

* Fix layout of VstSyncData struct

Ensure member of type double is 8-byte aligned for consistent layout between 32- and 64-bit Linux builds.

* Rework after code reading

* Fix possible segfault in `SubPluginFeatures::displayName`
* Minor fixes

* Allow TabWidget to be variable sized

* Allow instrument views to grow with contents

Make InstrumentTrackWindow as large as the InstrumentView requires

* Make instrument window's piano optional

* Make Model class visitable

* Forbid copying the Knob class

* Always instantiate at least a dummy plugin

* Fix too small instrument tabs

Previously, they had been resized by the fixed size parent tab widget. We need
to do this manually now.

* Fix Blackman-Harris window formula (LMMS#4895)

Adds missing parentheses

* Automation Editor: Don't accept drag events when there's no pattern

Fixes LMMS#4857

* TCO drag: Fix Ctrl+Drag crash

Fix some assumptions that source and target of a drag actions are the same
track container. Instead of looking up necessary information (track name,
type and container id) by track index, add it to the metadata.
Refactor canPasteSelection to take QDropEvent instead of the drop event's
QMimeData. Coincidentally, this fixes the method to be consistent with its
documentation.

Fixes LMMS#4844

* Bump zyn submodule
Per LMMS#4642

* Allow new Zyn bank creation on Linux (LMMS#4905)

Allow new Zyn bank creation on Linux
Closes LMMS#4642

* Fix empty editors after closing them and creating a new project (LMMS#4891)

* Remove useless include

* Fix invalid display names

* Travis: use carla instead of carla-git

Due to some breaking changes in the development branch of Carla,
we can't use the package right now.
Fortunately, the carla package now points to 2.0 series. So we will use it.

See also: https://kx.studio/News/?action=view&url=changes-in-kxstudio-repos-regarding-carla-and-jack2

* Travis: fix shellcheck warnings for the Debian sid script

* Remove some Qt4 compatibility code

* SetupDialog: fix file dialog not opening for theme directory 

It was caused by a typo affecting a signal-slot connection.

* Fix instrument window tab sizes

- Fix the instrument window tabs minimum width and height formulae
- Also set minimum height and width for instrument tab

* PluginBrowser: Tree layout and search bar

* [Equalizer] Bright analyzer colors, opacity increased (LMMS#4772)

* [Equalizer] Bright analyzer colors, opacity incr

Brightened spectrum analyzer colors and increased opacity a tad to make more visible

* Fixed RGB Value

* Update EqControlsDialog.cpp

* Fixed color change

* Changed colors again

* Fixed colors, now brighter and bluer

* Ok, its actually bright now lol

* Replace Monstro icons

* Shrink ComboBox arrow section slightly

This is being done for two reasons:

1. The new Monstro icons (and the icons for Microwave when it's finished) are too large.
2. All ComboBoxes (subjectively) look much nicer this way.

* Don't try to connect to nonexistent controllers (LMMS#4939)

Fixes crash on loading presets with controllers.

* Allow sample track TCOs to resize smaller than one bar (LMMS#4933)

Other changes:
* Update TCO position more exact when a drag leaves a TCO and enters `TrackContentWidget` (required to detect that the cursor has really moved when leaving a TCO with length < 1 to the right)
* Use exact length when samples are loaded, don't round it up
* Reset size when reloading same file

* Use local cursor for TrackContentObjectView (LMMS#4918)

Fixes crash on cloning patterns on Qt >= 5.12.

* Add PluginIssue class (LMMS#4901)

* Move macro definition out of class

* Fix -1 offset in plugin tab

In the instrument plugin tab, there was an orange stripe for
TripleOscillator. This was because internally, TabWidget moves up the
widget by 1 (TabWidget.cpp, line 89).

The size of the whole window is:

```
widget->height() + m_tabbarHeight - 1
```

So this code adds an offset of "-1" to the necessary computations.

* Fix bad identation in old code

* Coding conventions

* FileBrowser: Backup expanded directories and restore that state when the tree is reloaded.

* Allow build for Wayland w/o X11Extras, if VST off

@lukas-w reminds in 134dae8 comments that X11Extras help Linux users of
VST effects LMMS#3786. Now LMMS builds and runs on Weston without X11
dependencies, though only if WANT_VST is off.

* Document Effect::checkGate

* Document graph widget

* Update wiki submodule

* Fix wiki submodule

* Fix notes getting stuck under high CPU conditions (LMMS#4908)

* Record chords (LMMS#4938)

* Added check for chord to notes recorded from keyboard

* Make more connections direct for automation (LMMS#4942)

* Improve dcast

* document `dcast`
* make `dcast` not only cast exact, but also upwards
* add `dcast` test
* rename `dcast` -> `dynamicCast`

* AutomatableModelTest: Improve tests

Check whether returned pointers from the cast are equal to the original
pointers, rather than just checking wether they are not `nullptr`.

* Fix CI on windows

* Code style + Extend for TempoSyncKnob

* Travis: fix a debootstrap error from missing keyrings

Uses 18.04's debian-archive-keyring to fix the missing keyrings.

* Don't draw note detuning info over the volume/panning area (LMMS#4965)

* Show/Focus BBEditor on TrackLabelButton click LMMS#4946 (LMMS#4959)

It will be shown even if the parent is hidden.

* Use extracted linuxdeployqt directly

As of probonopd/linuxdeployqt#370,
the AppRun of linuxdeployqt unsets LD_LIBRARY_PATH.
This behavior isn't suitable for our cases, so we use
the extracted binary directly as a workaround.

* Fix invisible note editing handles when a note has detuning info

Fixes a regression in 32df2d7,
the clipping area was restored in a wrong place.
Also, a wrong value was used while restoring.

* Fix the wrong merge

* fix hanging mouse in piano roll (LMMS#4822) (LMMS#4960)

* fix hanging mouse in piano roll (LMMS#4822)

* fix hanging mouse in automation & pianoroll (LMMS#4822);

* fix hanging mouse in automation & pianoroll (LMMS#4822)

removed TODO comment that I forgot in the code

* Fix race conditions in NotePlayHandleManager (LMMS#4966)

NotePlayHandleManager::acquire uses a read lock unless the pool is empty.
If two threads try to acquire NotePlayHandle simultaneously
when the value of s_availableIndex is 1, one thread will try to read s_available[-1].
If the acquire action and the release action are done at the same time,
NotePlayHandleManager::acquire may try to read data
before NotePlayHandleManager::release actually writes.

This commit prevents them by always using the write lock when acquiring a NotePlayHandle.

* Nescaline and Freeboy - Better default sound (LMMS#4968)

* Fix controller loading error on loading projects

Fixes a regression in 91f9f1a,
which added the range check into the wrong if statement.

* Extend ProjectJournal docs, thx to @DomClark

* Fixes LMMS#4781: Don't disconnect LADSPA automation on export

* playing/recording pianoRoll's chord notes (LMMS#4963)

* SampleTrack: call requestChangesInModel before unref SampleBuffer (LMMS#4982)

To avoid a race condition between the gui thread which destroys the
samplebuffer and the mixer thread which increases the buffer's ref-
count, we'll make sure to touch the ref-count only when wh're synced
with the mixer.

* Making clearer the hierarchy. (LMMS#4967)

Add `override` keyword

* i18n: update translations from Transifex

* Bugfix - SampleTrack -> Load & Save: Fix recorded sample track not being
played correctly after saving and loading a project due to sample rate
not getting saved in the project file.

* Bugfix - SampleTrack -> Play: Fix sample track not being played in the
right place when it not played from the begining.

That has created a difference between the ticks and the metronome and
the sample track.

The cause of the problem was that the calculation of the frame to play
was wrong: we had calculated `framesPerTick` according to the current
engine's sample rate instead of the SampleBuffer's sample rate.

* SampleBuffer -> Use processingSampleRate.

SampleBuffer was using baseSampleRate as the default samplerate instead of the actual processingSampleRate.

* i18n: update template strings

* Travis: fix macOS build due to the old default version of Node.js

appdmg recently dropped support for Node.js < 8.5

* MP3 export: initialize ID3 tag with id3tag_init

Fixes wrongly set 'Blues' genre(ID 0) in exported mp3 files.

* updated splash (LMMS#5002)

* Fixes LMMS#4996: Fix metadata when exporting multiple tracks (LMMS#5005)

Co-Authored-By: Hyunjin Song <[email protected]>

* Fix German translation

* Fix "Export Tracks"
* Add "Export MIDI"

[ci skip]

* DMG rebranding (LMMS#5013)

Rebrand macOS DMG background
- Design and artwork courtesy @RebeccaDeField
- Closes LMMS#4289

* fix color profile error (LMMS#5019)

* Bump version to 1.2.0

* Add a save option to discard MIDI connections (LMMS#5021)

* Song: add isSavingProject.
* VersionedSaveDialog: Add support for a custom option dialog.
* AutomatableModel: Support the discardMIDIConnections save option.
* InstrumentTrack: Support the discardMIDIConnections save option.
* SaveOptions: Show the save option dialog on "save as".

* Enable dark title bar on macOS
Closes LMMS#5031

* Travis: drop unneeded prefix setting for npm

* Fix macOS builds

* Increase Mixer fader falloff speed

This also fixes a display bug that is present with large fader falloff speeds.

*  Replace tLimit() with qBound() and remove templates.h (LMMS#5040)

* Replace tLimit() with qBound()

* Remove templates.h

* Add .editorconfig

This file tells the github online editor to use tabs of width 4 instead
of 8. Open
[this](https://github.com/JohannesLorenz/github_test/blob/master/test.cpp)
test file in your browser to see how it works (the width is 5 in the
example).

Actually, it also tells many other editors...

* FadeButtons now remain partially lit as a note plays out (LMMS#4969)

* MDI Subwindow Decoration for VeSTige instruments closes LMMS#2824 (LMMS#2826)

* MDI Subwindow Decoration for VeSTige instruments closes LMMS#2824

* revert changes

* change QMdiSubWindow to our SubWindow in Vestige

* Drop sample on sampletracks (LMMS#5043)

* implements drag and drop samples to sampletracks

* clean up / take care of timeLineWidget heigth in songeditor

* unused memeber removed

* clean up

* Update include/TrackContainerView.h

Co-Authored-By: Spekular <[email protected]>

* Update src/gui/TrackContainerView.cpp

Co-Authored-By: Spekular <[email protected]>

* Update src/gui/TrackContainerView.cpp

Co-Authored-By: Spekular <[email protected]>

* Update src/gui/editors/SongEditor.cpp

Co-Authored-By: Spekular <[email protected]>

* load AFP if we don't drop on a sample track

* take care of timeLineWidget size changes

* clean up

* consolidate some code

* requested changes by code review

* move logic to SampleTrackView

* clean up

* clean up

* clean up

* Closes LMMS#5050: Add Mixer threading comments (LMMS#5069)

[ci skip]
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this issue Jun 28, 2022
sdasda7777 pushed a commit to sdasda7777/lmms that referenced this issue Jun 28, 2022
Allow new Zyn bank creation on Linux
Closes LMMS#4642
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants