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

[qtquick3d] devendor meshoptimizer #41708

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

Conversation

m-kuhn
Copy link
Contributor

@m-kuhn m-kuhn commented Oct 22, 2024

@Neumann-A does this need to be upstreamed to be acceptable?

@m-kuhn m-kuhn changed the title [qtquick3d] devendor messhoptimizer [qtquick3d] devendor meshoptimizer Oct 22, 2024
@FrankXie05 FrankXie05 self-assigned this Oct 22, 2024
@FrankXie05
Copy link
Contributor

FrankXie05 commented Oct 22, 2024

This patch is too large. 🤨 Is this patch necessary now?

@FrankXie05 FrankXie05 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Oct 22, 2024
@m-kuhn
Copy link
Contributor Author

m-kuhn commented Oct 22, 2024

I could reduce the patch and not remove the library (or use file(REMOVE_RECURSE src/3rdparty/meshoptimizer) instead)

@FrankXie05
Copy link
Contributor

We can download it through vcpkg_download_distfile.
eg:

vcpkg_download_distfile(ALSA_VERSION_SCRIPT_PATCH

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Oct 22, 2024

let me reduce it, I think it's more transparent and maintainable

DEFINES
QT_BUILD_QUICK3DUTILS_LIB
INCLUDE_DIRECTORIES
../3rdparty/xatlas
Copy link
Contributor

Choose a reason for hiding this comment

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

xatlas should probably also be devendored then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

probably, I only did the one that was causing pain for me and xatlas would require a new port afaics.

@Neumann-A
Copy link
Contributor

@Neumann-A does this need to be upstreamed to be acceptable?

Why are you asking me. Having it upstream would be better for future updates. You probably need to move the find_package logic into a configure.cmake file which has a feature depending on INPUT_<something>='system'. qtmultimedia has a few of those if i remember correctly.

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Oct 22, 2024

@Neumann-A does this need to be upstreamed to be acceptable?

Why are you asking me.

For this kind of valuable feedback:

Having it upstream would be better for future updates. You probably need to move the find_package logic into a configure.cmake file which has a feature depending on INPUT_<something>='system'. qtmultimedia has a few of those if i remember correctly.

@m-kuhn
Copy link
Contributor Author

m-kuhn commented Oct 23, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants