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

Port to rhi #1463

Closed
wants to merge 21 commits into from
Closed

Conversation

mlowczynski
Copy link

No description provided.

@mcallegari
Copy link
Owner

mcallegari commented Dec 17, 2023

Hi @mlowczynski thanks for this! I've got a first comment on this PR.
Instead of using a QmlFileSelector to support Qt5/Qt6 builds, I would solve this at cmake level, by including the proper files depending on the Qt version.
For example why not naming ActionsMenu_qt5.qml, RightPanel_qt5.qml and SettingsView3D_qt5.qml instead?
Then they can be included in duplicated qrc (e.g. qmlui_qt5_filedialog.qrc and qmlui_qt6_filedialog.qrc) selected in CMakeList.txt

What do you think?
Thanks

)
set_source_files_properties("qml/virtualconsole/WidgetsList.qml"
PROPERTIES QT_RESOURCE_ALIAS "WidgetsList.qml"
set_source_files_properties("qml/fixturesfunctions/3DView/shaders/rhi/fullscreen_rhi.vert"
Copy link
Owner

Choose a reason for hiding this comment

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

Is this a leftover?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, thank you for pointing out!

qmlui/qmlui.qrc Outdated
Copy link
Owner

Choose a reason for hiding this comment

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

Please keep the original comments to separate components. It's easier to maintain for me

Copy link
Author

Choose a reason for hiding this comment

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

Sure!

@mcallegari
Copy link
Owner

mcallegari commented Dec 17, 2023

I have updated the filedialog branch. Please update on it

Also, I have built the PR sources and the 3D view looks like this:

image

@mlowczynski
Copy link
Author

I have updated the filedialog branch. Please update on it

Also, I have built the PR sources and the 3D view looks like this:

image

I will rebase it to your latest changes! :) I'll also fix this rendering issue and test it more thoroughly.

@mlowczynski
Copy link
Author

Hi @mlowczynski thanks for this! I've got a first comment on this PR. Instead of using a QmlFileSelector to support Qt5/Qt6 builds, I would solve this at cmake level, by including the proper files depending on the Qt version. For example why not naming ActionsMenu_qt5.qml, RightPanel_qt5.qml and SettingsView3D_qt5.qml instead? Then they can be included in duplicated qrc (e.g. qmlui_qt5_filedialog.qrc and qmlui_qt6_filedialog.qrc) selected in CMakeList.txt

What do you think? Thanks

Addressing this issue at the CMake level is potentially feasible, although I haven't attempted it before. The approach you suggested may encounter a few issues. For instance, if we name a file ActionsMenu_qt5.qml, we might be compelled to use this code to create an object of this component:

ActionsMenu_qt5 {
...
}

While an alias to the QML file might resolve this problem, I'm not entirely certain. Moreover, at least three .qrc files (e.g., qmlui_common_to_both_versions_of_qt.qrc, qmlui_qt5.qrc, qmlui_qt6.qrc) would be necessary for this solution to avoid linking two files to same alias. Alternatively, we can create two .qrc files for each version of Qt, but then the common part will be repeated in both files

I lean towards utilizing a QmlFileSelector, as it appears to be an optimal and well-documented solution for an analogous problem (refer to https://doc.qt.io/qt-6/qfileselector.html and https://doc.qt.io/qt-6/qqmlfileselector.html). Introducing backward compatibility with qt5 for a specific file is straightforward – creating a customized file in the +qt5 directory, adding a correct entry to the qmlui/qmlui.qrc file, and reloading the CMake project. Additionally, I appreciate the project structure that this solution enforces, placing all qt5 files in "+qt5" directories.

To summarize, I believe the QmlFileSelector solution is simpler and easier to maintain. However, there may be arguments that I haven't taken into account.

@mlowczynski
Copy link
Author

Due to the bug with Qt3D Layer Filter under RHI backend (https://bugreports.qt.io/browse/QTBUG-109041) we decided to try with QtQuick3D. I have created another pull request with minimal port to Qt6, including changes required for running Qt3D under Qt6 without porting to RHI backend (#1558)

Closing for now

@lilyCalla
Copy link
Contributor

Due to the bug with Qt3D Layer Filter under RHI backend (https://bugreports.qt.io/browse/QTBUG-109041) we decided to try with QtQuick3D. I have created another pull request with minimal port to Qt6, including changes required for running Qt3D under Qt6 without porting to RHI backend (#1558)

Closing for now

I'm not a lawyer, not legal advice, all that, but word of warning, in my understanding QtQuick3d is one of the modules licensed under GPLv3 only instead of being also available under LGPLv3. Sorry if that's unhelpful, just an unfortunate subtle thing with QtQuick licensing that I wanted to make sure you're aware of

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.

3 participants