Upgrade to Qt 5 #2509

fu5ha · 2019-03-27T06:47:47Z

This is now working! This is a required change for #2393 to work on macOS. Required changes to build:

Windows

In order to build with WITH_DISNEY_MATERIAL=OFF, the only change needed is to first download and build (or download a precompiled version of) Qt 5, I tested with the latest Qt 5.12.2. I recommend building from source or using this precompiled version https://github.com/martinrotter/qt5-minimalistic-builds (this is what I tested with) because the official builds are massive.

With that done, the cmake command needs to be modified slightly, removing -DQT_QMAKE_EXECUTABLE and replacing it with -DCMAKE_PREFIX_PATH, which should point to the root qt5 folder. For example, for me that is -DCMAKE_PREFIX_PATH=G:\qt5.

Finally, in the Visual Studio solution, the debugging environment variables need to be changed slightly (I'm not sure if this is true with the official builds but it is with the precompiled one I linked), we need to now include qt5\lib and qt5\bin in PATH. (This is under Environment when you right-click a target and to go properties then select All Configurations).

Building with WITH_DISNEY_MATERIAL=ON requires a patched SeExpr, and I've made a version of appleseed-deps which supports this, https://github.com/termhn/appleseed-deps on the qt5 branch. The BuildAll.bat should work. You can also just comment out everything except the SeExpr part of that build script and copy it over to your current appleseed-deps if you have one.

macOS

If you built using the recommended way, you'll need to do a couple of homebrew things.

brew tap-unpin cartr/qt4
brew uninstall pyqt@4 qt@4 # There was another thing that needed to be uninstalled that said it was dependent of qt@4 but I forget what it was called, also uninstall that, you also might be able to just brew unlink instead of uninstall
brew install qt5

Then when building appleseed without Disney material support, the only change needed is to add the cmake variable -DCMAKE_PREFIX_PATH, which should point to the root qt5 folder. For the homebrew installation method, this is /usr/local/opt/qt, i.e. -DCMAKE_PREFIX_PATH=/usr/local/opt/qt

In order to build with WITH_DISNEY_MATERIAL=ON, you'll also need a patched SeExpr. You should be able to do the normal thing with this but just use my fork and check out the appleseed-qt5 branch https://github.com/termhn/SeExpr The only difference is you'll need again need to set -DCMAKE_PREFIX_PATH to the root of qt5, which for the homebrew installation method is /usr/local/opt/qt

git clone https://github.com/termhn/SeExpr
git checkout appleseed-qt5
mkdir build
cd build
cmake -DCMAKE_PREFIX_PATH=/usr/local/opt/qt ..
sudo make install

Linux

I haven't tested or attempted, but you should be able to basically follow the macOS instructions but you'll need to figure out how to install or build Qt 5 yourself.

fu5ha · 2019-03-27T19:52:47Z

Alright, with just a couple extra changes this works quite well with -DWITH_DISNEY_MATERIAL=OFF after initial testing, running on macOS!

There's a couple of minor style differences (the main one I can see is the location of the tab bar where the light paths/render tabs appear)

fu5ha · 2019-03-27T20:07:23Z

Oh, also, you need to set CMAKE_PREFIX_PATH to the root of wherever your qt installation is because FindQt5 is bundled with Qt5 instead of with CMake itself now. If you install qt5 with homebrew on mac (you need to uninstall qt4 as well) then that would be -DCMAKE_PREFIX_PATH=/usr/local/opt/qt.

oktomus · 2019-03-27T20:30:13Z

As long as it's a work in progress, you should add the WIP label and once it's ready, ask for a review.

fu5ha · 2019-03-27T20:31:26Z

@oktomus Didn't know I had permissions to label it myself :P

oktomus · 2019-03-27T20:31:26Z

About what you said on CMAKE_PREFIX_PATH, can't we add FindQt5 directly in appleseed ? What do you think @est77 ?

fu5ha · 2019-03-27T20:33:22Z

For CMAKE_PREFIX_PATH, there are several(many) cmake scripts that it needs to be able to find, not just one FindQt5.cmake, so I think it makes most sense to leave it with Qt. We could attempt to set CMAKE_PREFIX_PATH to some common install locations by default and then just make people set it if they have it in a nonstandard location, though.

fu5ha · 2019-03-27T20:41:25Z

Oh, and also, on Windows I would recommend building your own qt5 or using one of these builds: https://github.com/martinrotter/qt5-minimalistic-builds because the standard installers are massive (3.7GB download, what the heck??)

oktomus · 2019-03-27T20:42:59Z

When building on windows for instance, we specify QT_QMAKE_EXE_PATH or something, can't we extract the QT CMAKE_PREFIX_PATH from that ?

fu5ha · 2019-03-27T20:45:29Z

Yes you could, but I don't necessarily see a reason to do that, the CMAKE_PREFIX_PATH replaces the old QT_QMAKE_EXE variable (don't need that anymore).

Also, evidently my Windows building knowledge is just poor because I just built my modified version of SeExpr and then built appleseed.studio with WITH_DISNEY_MATERIAL=ON on macOS and it worked without a hitch (I didn't actually test its functionality yet, but it runs!)

fu5ha · 2019-03-28T03:15:49Z

Alright, I figured out windows as well and it's working with SeExpr too :)

I will wrap up some loose ends and update original PR then ask for review.

fu5ha · 2019-03-28T06:10:32Z

Okay I think this is ready for testing/review. Updated top level comment with instructions. CC @dictoon @est77 @oktomus

pjessesco · 2019-03-29T05:25:17Z

~~It seems this PR also solves #2239.~~ No it doesn't.

fu5ha · 2019-03-29T05:28:34Z

@pjessesco Sweet! It should be trivial to also test #2496 if you've gotten this PR working :D

pjessesco · 2019-03-29T05:36:01Z

Ah, it works well first time but it doesn't works after that.

oktomus · 2019-03-29T19:23:16Z

Travis linux builds are failing. I'm not sure but you may need to update the travis build script.
See https://github.com/appleseedhq/appleseed/blob/master/scripts/travis/build-linux.sh#L82

Here is where it failed https://travis-ci.org/appleseedhq/appleseed/jobs/512359848#L796

oktomus · 2019-03-29T19:24:39Z

Oh you haven't tested for Qt5 on linux. Maybe I can help with that.

dictoon

Amazing work, thank you!

dictoon · 2019-03-29T19:59:29Z

cmake/utilities.txt

+macro (QT5_WRAP_CPP_CPLUSPLUS_FILES outfiles)
+    QT5_GET_MOC_FLAGS (moc_flags)
+
+    set(options)


For some weird reason we insert a space before parentheses; for consistency's sake, please insert these spaces :)

src/appleseed.studio/CMakeLists.txt

dictoon · 2019-03-29T20:02:45Z

src/appleseed.studio/main/main.cpp

    {
 #ifdef __APPLE__
        // Under certain circumstances (under an macOS virtual machine?), a bogus warning
        // message is repeatedly printed to the console. Disable this warning message.
        // See https://github.com/appleseedhq/appleseed/issues/254 for details.
        if (type == QtWarningMsg &&
-            strcmp(msg, "QCocoaView handleTabletEvent: This tablet device is unknown (received no proximity event for it). Discarding event.") == 0)
+            msg == "QCocoaView handleTabletEvent: This tablet device is unknown (received no proximity event for it). Discarding event.")


I wonder if this kludge is still necessary. I'll check once I had it running on a macOS VM.

dictoon · 2019-03-29T20:05:36Z

src/appleseed.studio/main/main.cpp

            return;
        }

        switch (type)
        {
          case QtDebugMsg:
-            fprintf(stderr, "Debug: %s\n", msg);
+            fprintf(stderr, "Debug: %s\n", msg.toLocal8Bit().constData());


I'm not sure we want to use toLocal8Bit() / fromLocal8Bit(). Ideally appleseed would internally only handle UTF-8 strings.

Could convert these to toUtf8() / fromUtf8() instead... I'm not exactly sure the implications of one or the other to be completley honest, the Local8Bit versions seemed safter but I could be off base on that assumption :)

dictoon

Thanks, looks perfect!

dictoon · 2019-03-31T08:58:35Z

This is ready to merge, but we need to wait until appleseedhq/windows-deps#4 in appleseed-deps is in.

dictoon · 2019-03-31T08:59:16Z

Once this is merged, we will need to update the build guides.

oktomus · 2019-03-31T13:53:27Z

Dont we need to make it support linux before merging it?

fu5ha · 2019-03-31T19:11:54Z

We should probably test it, yes... it should Just Work TM by pointing it to qt 5 instead of qt4, but... worth a test.

oktomus · 2019-03-31T20:37:01Z

In any case, the travis linux build script needs to be updated.

fu5ha · 2019-03-31T21:14:21Z

OK, I in theory updated the linux and macos travis build scripts; however, for linux, https://github.com/appleseedhq/prebuilt-linux-deps needs a new release with the patched version of SeExpr

dictoon · 2019-04-01T07:24:11Z

The prerequisite PR (appleseedhq/windows-deps#4) has been merged.

dictoon · 2019-04-01T07:25:31Z

It looks like the Linux dependency package needs to be updated:

In file included from /home/travis/build/appleseedhq/appleseed/src/appleseed.studio/mainwindow/project/expressioneditorwindow.cpp:52:0:
/home/travis/build/appleseedhq/appleseed/prebuilt-linux-deps/include/SeExprEditor/SeExprEditor.h:26:30: fatal error: QtGui/QTextBrowser: No such file or directory
 #include <QtGui/QTextBrowser>

fu5ha · 2019-04-01T07:30:26Z

Yep. Let me know if/how I can help with doing so; it should just be a simple rebuild of SeExpr based on my fork.

dictoon · 2019-04-08T14:17:59Z

I'm testing this PR.

I'm getting this warning when running CMake:

CMake Warning (dev) at CMakeLists.txt:236 (find_package):
  Policy CMP0074 is not set: find_package uses <PackageName>_ROOT variables.
  Run "cmake --help-policy CMP0074" for policy details.  Use the cmake_policy
  command to set the policy and suppress this warning.

  Environment variable Boost_ROOT is set to:

    N:\appleseed\boost_1_69_0

  For compatibility, CMake is ignoring the variable.

We need to silence it.

Later, CMake fails to to find Qt 5:

CMake Error at src/appleseed.studio/CMakeLists.txt:46 (find_package):
  By not providing "FindQt5.cmake" in CMAKE_MODULE_PATH this project has
  asked CMake to find a package configuration file provided by "Qt5", but
  CMake did not find one.

  Could not find a package configuration file provided by "Qt5" with any of
  the following names:

    Qt5Config.cmake
    qt5-config.cmake

  Add the installation prefix of "Qt5" to CMAKE_PREFIX_PATH or set "Qt5_DIR"
  to a directory containing one of the above files.  If "Qt5" provides a
  separate development package or SDK, be sure it has been installed.

dictoon · 2019-04-08T14:23:39Z

I fixed the second error about finding Qt 5 by providing the right path to Qt 5.

dictoon · 2019-04-08T14:55:57Z

It builds and runs correctly.

A few small issues:

The About dialog is missing a title bar and cannot be moved:

List header rows are too thick:

There's a bright horizontal line at the bottom of the window when both the Log and Python Console tabs are shown:

fu5ha · 2019-04-08T16:34:28Z

👍🏻 I will investigate probably tomorrow as today is quite busy for me.

…

On Mon, Apr 8, 2019 at 7:56 AM François Beaune ***@***.***> wrote: It builds and runs correctly. A few small issues: - The About dialog is missing a title bar and cannot be moved: [image: image] <https://user-images.githubusercontent.com/321290/55733618-6dd97f80-5a1e-11e9-8e63-af3c58926962.png> - List header rows are too thick: [image: image] <https://user-images.githubusercontent.com/321290/55733758-af6a2a80-5a1e-11e9-9325-4efb37842238.png> - There's a bright horizontal line at the bottom of the window when both the Log and Python Console tabs are shown: [image: image] <https://user-images.githubusercontent.com/321290/55734009-18ea3900-5a1f-11e9-8884-6b44ec881d34.png> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2509 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA2Ln0K1rY7rhIoCcy02W-abTTsHk7erks5ve1iFgaJpZM4cNEY3> .

dictoon · 2019-04-08T19:08:13Z

Created two tickets for the issues I mentioned earlier: #2535 #2536

fu5ha added 2 commits March 26, 2019 23:46

qt5 initial work

0da9b42

make it work on macos

e0c3dc5

oktomus added the WIP ⚠ label Mar 27, 2019

fu5ha mentioned this pull request Mar 28, 2019

Qt 5 Support appleseedhq/windows-deps#4

Merged

cleanup

98d05e5

fu5ha mentioned this pull request Mar 28, 2019

Use modern OpenGL to draw light paths and scene representation #2496

Merged

oktomus added the PR | Squash This PR must be squashed when merged. label Mar 29, 2019

dictoon reviewed Mar 29, 2019

View reviewed changes

address review comments

35ac4e3

dictoon approved these changes Mar 31, 2019

View reviewed changes

dictoon added On Hold 🔶 and removed WIP ⚠ labels Mar 31, 2019

update build scripts

854f6d0

retry ci

51acc6c

temporarily turn off disney material on travis for linux

7739f25

dictoon merged commit 94b600d into appleseedhq:master Apr 8, 2019

This was referenced Apr 8, 2019

Fix new CMake warnings after switch to Qt 5 #2535

Closed

Fix styling regressions after switch to Qt 5 #2536

Closed

pjessesco mentioned this pull request Apr 9, 2019

OpenGL render in Light Paths tab is glitchy on macOS 10.14 (Mojave) #2239

Closed

dictoon removed the On Hold 🔶 label Apr 9, 2019

Upgrade to Qt 5 #2509

Upgrade to Qt 5 #2509

Conversation

fu5ha commented Mar 27, 2019 • edited by pjessesco

Windows

macOS

Linux

fu5ha commented Mar 27, 2019 • edited

fu5ha commented Mar 27, 2019 • edited

oktomus commented Mar 27, 2019

fu5ha commented Mar 27, 2019

oktomus commented Mar 27, 2019

fu5ha commented Mar 27, 2019

fu5ha commented Mar 27, 2019

oktomus commented Mar 27, 2019

fu5ha commented Mar 27, 2019

fu5ha commented Mar 28, 2019

fu5ha commented Mar 28, 2019

pjessesco commented Mar 29, 2019 • edited

fu5ha commented Mar 29, 2019

pjessesco commented Mar 29, 2019 • edited

oktomus commented Mar 29, 2019

oktomus commented Mar 29, 2019

dictoon left a comment

Choose a reason for hiding this comment

dictoon Mar 29, 2019

Choose a reason for hiding this comment

dictoon Mar 29, 2019

Choose a reason for hiding this comment

dictoon Mar 29, 2019

Choose a reason for hiding this comment

fu5ha Mar 29, 2019

Choose a reason for hiding this comment

dictoon left a comment

Choose a reason for hiding this comment

dictoon commented Mar 31, 2019

dictoon commented Mar 31, 2019

oktomus commented Mar 31, 2019

fu5ha commented Mar 31, 2019

oktomus commented Mar 31, 2019

fu5ha commented Mar 31, 2019

dictoon commented Apr 1, 2019

dictoon commented Apr 1, 2019

fu5ha commented Apr 1, 2019

dictoon commented Apr 8, 2019

dictoon commented Apr 8, 2019

dictoon commented Apr 8, 2019

fu5ha commented Apr 8, 2019 via email

dictoon commented Apr 8, 2019

fu5ha commented Mar 27, 2019 •

edited by pjessesco

fu5ha commented Mar 27, 2019 •

edited

fu5ha commented Mar 27, 2019 •

edited

pjessesco commented Mar 29, 2019 •

edited

pjessesco commented Mar 29, 2019 •

edited