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

BBS merge fixes #5202

Closed
wants to merge 3 commits into from
Closed

Conversation

jamincollins
Copy link
Contributor

@jamincollins jamincollins commented Apr 30, 2024

Description

Fixes for issues introduced with the merges from BBS

Tests

Built and tested on Arch Linux

Signed-off-by: Jamin W. Collins <jamin.collins@gmail.com>
Signed-off-by: Jamin W. Collins <jamin.collins@gmail.com>
@TomPcz
Copy link

TomPcz commented Apr 30, 2024

This indeed fixes #5208, thank you!

@@ -525,28 +525,17 @@ void release_window_pools()
template<typename T>
struct Builder
{
Builder()
template<typename... Args>
Copy link
Owner

Choose a reason for hiding this comment

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

Do you mind to explain the changes a bit?
Like what is it trying to fix?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the bit that fixes #5028. Without it, most of the settings fields (TextCtrls, Checkboxes, etc) become unresponsive.

Copy link
Owner

Choose a reason for hiding this comment

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

Hey @jamincollins
After some evaluation, I have reverted the "control pool" changes which caused these issues.
d72c9f9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll test the latest changes.

@@ -892,8 +892,6 @@ int CLI::run(int argc, char **argv)
// instruct the window manager to fall back to X server mode.
::setenv("GDK_BACKEND", "x11", /* replace */ true);

::setenv("WEBKIT_DISABLE_COMPOSITING_MODE", "1", /* replace */ false);
Copy link
Owner

Choose a reason for hiding this comment

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

It might break the web feature on some Linux destructions. Fedora IIRC

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this setting present, it causes run away memory consumption and a segfault shortly after launch on Linux #5030.

Copy link

@evolarium evolarium May 2, 2024

Choose a reason for hiding this comment

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

I have also been getting segfaults on newer nightly builds of OrcaSlicer on Gentoo Linux. I can confirm that removing this line fixes the segfault issue for me.

Edit: I ended up just putting env WEBKIT_DISABLE_COMPOSITING_MODE=0 in the Exec line of my orca-slicer.desktop file since this won't override the variable if it is already set.

@@ -0,0 +1,8 @@
orcaslicer_add_cmake_project(
Copy link
Owner

Choose a reason for hiding this comment

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

Why?
Orca has Eigen in the source: src\eigen\Eigen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was not being found by OpenCV.

-- Configuring done (1.4s)
CMake Error at deps/build/destdir/usr/local/lib/cmake/opencv4/OpenCVModules.cmake:71 (set_target_properties):
  The link interface of target "opencv_world" contains:

    Eigen3::Eigen

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  deps/build/destdir/usr/local/lib/cmake/opencv4/OpenCVConfig.cmake:126 (include)
  src/libslic3r/CMakeLists.txt:481 (find_package)


-- Generating done (0.1s)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will test with just the find portion and not the additional dep, will update shortly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does look like this can be a more simple change, will update the PR.

Copy link
Owner

@SoftFever SoftFever Apr 30, 2024

Choose a reason for hiding this comment

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

Can you double check? Might be an env issue in your machine?
It compiles fine in CI/CD

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't have one installed on my OS. Only the one in the source tree.

$ pacman -Ss eigen
extra/arpack 3.9.1-2
    Fortran77 subroutines for solving large scale eigenvalue problems
extra/cauchy 0.9.0-3
    A library for transforming Matlab/Octave files to C++ (with Eigen) and Matlab
extra/eigen 3.4.0-2
    Lightweight C++ template library for vector and matrix math, a.k.a. linear algebra
$ pacman -Q eigen
error: package 'eigen' was not found

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've dug into this more. If I have an eigen package installed when the deps are built, then OpenCV's OpenCVModules.cmake has this line:

INTERFACE_LINK_LIBRARIES "\$<LINK_ONLY:dl>;\$<LINK_ONLY:m>;\$<LINK_ONLY:pthread>;\$<LINK_ONLY:rt>;\$<LINK_ONLY:Eigen3::Eigen>;\$<LINK_ONLY:libjpeg-turbo>;\$<LINK_ONLY:libpng>;/home/jamin/proj/3d-printing/OrcaSlicer/deps/build/destdir/usr/local/lib/libtiff.a;/lib64/libz.so;/lib64/libz.so;\$<LINK_ONLY:va>;\$<LINK_ONLY:va-drm>"

If I remove the eigen package and rebuild the deps, the OpenCVModules.cmake has this line:

INTERFACE_LINK_LIBRARIES "\$<LINK_ONLY:dl>;\$<LINK_ONLY:m>;\$<LINK_ONLY:pthread>;\$<LINK_ONLY:rt>;\$<LINK_ONLY:libjpeg-turbo>;\$<LINK_ONLY:libpng>;/home/jamin/proj/3d-printing/OrcaSlicer/deps/build/destdir/usr/local/lib/libtiff.a;/lib64/libz.so;/lib64/libz.so;\$<LINK_ONLY:va>;\$<LINK_ONLY:va-drm>"

The difference between the two being:

\$<LINK_ONLY:Eigen3::Eigen>;

This means that the OpenCV dep is changing its compile behavior based on which system packages are installed.

That doesn't seem desirable to me. IMO, it's one thing to indicate which packages a user needs to have installed in order to build software. It's another thing entirely to tell a user they must not have a package installed in order to build.

While the OpenCV.cmake has this line:

       -DWITH_EIGEN=ON

I don't believe this is actually causing OpenCV to be built with Orca's in-tree Eigen (src/eigen), but instead simply triggering the above edge case.

I've built deps and Orca with the above set to OFF and so far see no negative effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has also been independently hit by another member on the discord.
https://discord.com/channels/1137181739773603922/1137187844696318023/1235691546943295568

Copy link
Owner

Choose a reason for hiding this comment

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

Thanks, Collins.
What Linux distribution are you using?
I can take a look when I'm free.
However the proposed change here isn't desirable.
It might seem harmless at first, but trust me, it'll lead to trouble eventually.
C++ has many quirks unfortunately

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Arch Linux.

@SoftFever
Copy link
Owner

@jamincollins
First of all, thank you!
To ensure a smooth review process, may I request:

  1. Consolidating only relevant changes into a single PR.
  2. Including a meaningful description about the PR and code changes in both the title and description?

Signed-off-by: Jamin W. Collins <jamin.collins@gmail.com>
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.

None yet

4 participants