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

Linux support #85

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Linux support #85

wants to merge 2 commits into from

Conversation

polygon
Copy link

@polygon polygon commented Nov 1, 2023

I've made NeuralNote build under Linux and was wondering if there is interest to upstream the changes. This PR would be the first step and contains the required changes to the code and the build system. The changes I was doing are:

  • Remove LTO when building for Linux as it was causing undefined reference issues
  • Refactor font loading into a singleton class to circumvent the "static initialization catastrophe" which seems to happen only under Linux since JUCE tries to read the fonts.conf XML file and the XML parser will use certain static strings that are not initialized at that time. I've written more about this here: Linux support #33 (comment)
  • Create folder for NeuralNote inside the received temporary directory since NeuralNote would try to erase the whole /tmp directory otherwise, usually taking some temp-files of other applications with it

This change also requires an accompanying change to libonnxruntime-neuralnote to get a Linux build there which I have also available. But first wanted to start here to gauge interest in something like this.

There is another issue with external Drag&Drop not working properly in Linux. This seems to be a JUCE issue and I have a workaround available. But since this requires patching the external JUCE dependency I think that would probably be more suitable to put into the build scripts. Also, it's not strictly required for a Linux build.

Edit: Link to patched libonnxruntime-neuralnote: https://github.com/polygon/libonnxruntime-neuralnote/tree/linux

Avoids NeuralNote attemtping to delete the whole assigned temp
directory
@DamRsn
Copy link
Owner

DamRsn commented Nov 1, 2023

Hey, thanks for the contribution!

This looks great, I definitely want to merge this. However I don't have a lot of bandwidth at the moment, so it might take a bit of time to review/test everything on my side.

Is the issue with exporting the MIDI fixed, or if you have a fix what is it?

Few notes after a super quick look at the changes:

  • Let's try to avoid using raw pointers and new. Let's use modern C++ (std::unique_ptr ...) instead if possible.
  • I don't understand in depth the Font issue on Linux, but can't we have it working with a simpler interface? I'd like to avoid all the boilerplate of UIFonts::get().fontx().
  • Could we have all the fonts as member of UIFonts instead of recreating them each time with things like Font(pMONTSERRAT_BOLD).withPointHeight(18.0f)

Also you can open a PR on libonnxruntime-neuralnote so we can review it as well.

@polygon
Copy link
Author

polygon commented Nov 1, 2023

Hey, thanks for the response. I'm also a bit short on time so before doing some of the refactors, I'd like your opinion if that would be fine:

  • Refactoring to use std::unique_ptr instead of raw pointers should not be an issue, will do that
  • Yes, it's a bit more boilerplate indeed. However, I need to initialize the variables on first use to circumvent the issue. I could probably make it so instead of writing UIFonts::get().fontx() it would be necessary to write fontx() but I don't think getting rid of the braces is possible. Would that be acceptable for you? The other alternative would be another patch to JUCE since you seem to be embedding the fonts anyways, so looking up the system font config is kinda unneeded, but JUCE does it anyways
  • I will look into caching the actual Font objects as well, should not be an issue.

Regarding the drag and drop issue, JUCE has this piece of code that manages communication between the Drag&Drop processes:

    void handleExternalDragAndDropStatus (const XClientMessageEvent& clientMsg)
    {
        if (expectingStatus)
        {
            expectingStatus = false;
            canDrop         = false;
            silentRect      = {};

            const auto& atoms = getAtoms();

            if ((clientMsg.data.l[1] & 1) != 0
                && ((Atom) clientMsg.data.l[4] == atoms.XdndActionCopy
                    || (Atom) clientMsg.data.l[4] == atoms.XdndActionPrivate))
            {
                if ((clientMsg.data.l[1] & 2) == 0) // target requests silent rectangle
                    silentRect.setBounds ((int) clientMsg.data.l[2] >> 16, (int) clientMsg.data.l[2] & 0xffff,
                                          (int) clientMsg.data.l[3] >> 16, (int) clientMsg.data.l[3] & 0xffff);
                canDrop = true;
            }
        }
    }

I am not fully understanding of the details of Drag&Drop in Linux/X11 but I found that these if-conditions are no longer satisfied once the dragged data is moved outside of the plugin window. Hence, canDrop becomes false and in the following code which handles what happens after the mouse button is released to end the drop:

    void handleExternalDragButtonReleaseEvent()
    {
        if (dragging)
            X11Symbols::getInstance()->xUngrabPointer (getDisplay(), CurrentTime);

        if (canDrop)
        {
            sendExternalDragAndDropDrop();
        }
        else
        {
            sendExternalDragAndDropLeave();
            externalResetDragAndDrop();
        }
    }

the drop operation would be canceled. This had the funny side effect that while dragging, I could even see the MIDI notes in Bitwig, but once I released the mouse button, they were gone again. My (probably) dirty fix involved replacing if (canDrop) with if (1) to always force the drop to complete. This makes dropping files from NeuralNote to file managers and also to Bitwig Studio work without issues. I've not yet encountered any negative side-effects of always completing the drop.

I'll open a Pull Request for libonnxruntime-neuralnote later today once I am off work.

@KottV
Copy link

KottV commented Dec 25, 2023

Hey!
This is exciting, I compiled and it works. And more over this drag-and-drop "fix" looks like a key for a long standing JUCE DnD Linux bug:)

I only had to add set(CMAKE_POSITION_INDEPENDENT_CODE TRUE) do make the linker happy.

@polygon polygon mentioned this pull request Mar 12, 2024
@raphaelbastide
Copy link

@polygon If any linux build is available somewhere, I would be very happy to try it.

@buscon
Copy link

buscon commented Jun 15, 2024

@polygon If any linux build is available somewhere, I would be very happy to try it.

same here, I am a Linux user and I am happy to try it and give feedback.

juce_recommended_config_flags
juce_recommended_lto_flags)
if (UNIX AND NOT APPLE)
target_link_libraries(${BaseTargetName}
Copy link

@dromer dromer Nov 1, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're mixing tabs and spaces here causing uneven formatting.

The rest of the code uses space, so lets follow the same here.

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.

6 participants