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

deps.qt: Add qtcharts #192

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

deps.qt: Add qtcharts #192

wants to merge 1 commit into from

Conversation

pkviet
Copy link
Member

@pkviet pkviet commented Jun 5, 2023

Description

This enables building of qtcharts (as well as qtopengl, qtopenglwidgets) on which it depends.

Motivation and Context

This is a companion to other PRs in obs-studio enabling stats charts for SRT [1] & maybe regular stats.

[1] obsproject/obs-studio#9027

How Has This Been Tested?

Builds locally on windows and passes CI on my fork.

Types of changes

  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@pkviet pkviet marked this pull request as draft June 5, 2023 18:50
@pkviet pkviet force-pushed the qcharts branch 3 times, most recently from dd9bb31 to ab36763 Compare June 26, 2023 18:43
@pkviet pkviet marked this pull request as ready for review June 27, 2023 07:59
Copy link
Member

@RytoEX RytoEX left a comment

Choose a reason for hiding this comment

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

As far as I can tell, qtopengl and qtopenglwidgets are optional dependencies of qtcharts, not required dependencies. Could you verify?

deps.qt/qt6.ps1 Outdated Show resolved Hide resolved
deps.qt/qt6.zsh Outdated Show resolved Hide resolved
@pkviet
Copy link
Member Author

pkviet commented Jul 7, 2023

As far as I can tell, qtopengl and qtopenglwidgets are optional dependencies of qtcharts, not required dependencies. Could you verify?

correct:
https://github.com/qt/qtcharts/blob/dev/src/charts/CMakeLists.txt#L123-L128

@pkviet
Copy link
Member Author

pkviet commented Jul 7, 2023

@pkviet pkviet changed the title deps.qt: Add qtcharts and its dependencies deps.qt: Add qtcharts Jul 7, 2023
@pkviet
Copy link
Member Author

pkviet commented Jul 8, 2023

@RytoEX mmh even though qt6OpenGL is not mandatory it seems it builds with Qt6OpenGL and Qt6OpenGLWidgets on default even if the latter are not specified.
See the screenshot:
DependenciesGui_2023-07-08_06-49-18
I'll investigate if I do need OpenGL/OpenGLWidgets or not in my current code and how to force building without these deps.

@pkviet
Copy link
Member Author

pkviet commented Jul 8, 2023

It seems Qt6OpenGL and Qt6OpenGlWidgets are already shipping with the deps.
This is a screenshot from the last obs-deps release (06-22):
explorer_2023-07-08_07-01-35

I checked the various qt dlls in the bin folder but none had QtOpenGL as a dependency ...

@RytoEX
Copy link
Member

RytoEX commented Jul 8, 2023

I do see that. We don't currently ship them with OBS Studio itself because they aren't needed. Qt's default configuration is "dynamic OpenGL". The old Qt 5.15 build instructions explain the difference in configuration options. Maybe even though they are considered optional dependencies of Qt Charts, it tries to opportunistically load them?

@PatTheMav
Copy link
Member

Seems to explicitly require -DQT_FEATURE_opengl:BOOL=NO to disable it opportunistically adding it.

The way the logic works is: "If OpenGL support is not disabled, use it". Thus it becomes a hard requirement of QtCharts. We haven't explicitly disabled it so far, but might be required now.

@RytoEX
Copy link
Member

RytoEX commented Jul 16, 2023

Are the OpenGL DLLs copied in the obs-studio Qt Charts PR in CMake 3? Does Qt Charts fail to work if the OpenGL DLLs are not in the obs-studio directory?

@PatTheMav
Copy link
Member

PatTheMav commented Jul 16, 2023

Are the OpenGL DLLs copied in the obs-studio Qt Charts PR in CMake 3? Does Qt Charts fail to work if the OpenGL DLLs are not in the obs-studio directory?

I think they might be - we resolve which DLLs to copy by recursively resolving the dependency tree, and I can imagine the following happening:

  • obs-studio depends on Qt6::Charts
  • Qt6::Charts depends on Qt6::OpenGL (because the CMake files are generated by Qt at build time)
  • obs-studio has Charts and OpenGL as direct dependencies

I would expect the only way to remove that is build Charts explicitly without it, so the dependency doesn't end up in Charts' CMake package file.

@pkviet
Copy link
Member Author

pkviet commented Jul 16, 2023

Are the OpenGL DLLs copied in the obs-studio Qt Charts PR in CMake 3? Does Qt Charts fail to work if the OpenGL DLLs are not in the obs-studio directory?

Yes currently i'm copying the qt opengl DLLs in the two stats PRs in obs-studio.
(I plan to overhaul them pending mockups from George and ui/ux discussions.)
Qt Charts fails to load without them.
I'm trying to do without the openGL deps following Pat's pointers.
I thus passed -DFEATURE_opengl:BOOL=OFF
But QtGui fails to compile on windows with errors in qwindowswindow.cpp (line 2310):

use of undefined type QOpenGLContext

And another similar error ...

@pkviet
Copy link
Member Author

pkviet commented Jul 19, 2023

Update
With Pat's help I managed to compile without any openGL dependence.

@pkviet
Copy link
Member Author

pkviet commented Sep 1, 2023

Update With Pat's help I managed to compile without any openGL dependence.

Update
With the update from 6.4.3 to 6.5.2, QtMultimedia fails to build when one disables opengl.
So we're back to square one.
Not sure why we would need QtMultimedia btw.

@RytoEX
Copy link
Member

RytoEX commented Sep 1, 2023

Update With Pat's help I managed to compile without any openGL dependence.

Update With the update from 6.4.3 to 6.5.2, QtMultimedia fails to build when one disables opengl. So we're back to square one. Not sure why we would need QtMultimedia btw.

QtMultimedia was to be used for accessibility features to play sound effects on certain actions. It wasn't implemented initially because it required Windows Media features that were not installed by default on Windows N systems, and earlier versions of Qt and FFmpeg would crash if they tried to load the necessary libraries. We pushed for changes in those implementations to safely attempt to load the libraries and just never circled back around to implementing the feature on our end.

This enables building of qtcharts.

Signed-off-by: pkv <pkv@obsproject.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants