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

Autodesk: Make Hydra Scene Browser source files usable in independent builds #2689

Conversation

erikaharrison-adsk
Copy link
Contributor

Description of Change(s)

This PR makes the Hydra Scene Browser source files suitable for compiling into independent builds/libraries. Specifically, the changes are :

  • Add dynamic linking import/export support using the same api header format as the rest of the USD codebase.
  • Support Qt6 by using an intermediary QLatin1String when constructing a QVariant off a char* (QLatin1String already existed in Qt5, however in Qt6 it is required to do so, as constructing a QVariant off a char* is disabled).
  • Remove an unused parameter in a lambda that was causing a warning-treated-as-error on Mac builds.

Fixes Issue(s)

  • N/A
  • I have verified that all unit tests pass with the proposed changes
  • I have submitted a signed Contributor License Agreement

@jesschimein
Copy link
Contributor

Filed as internal issue #USD-8704

@@ -56,7 +57,7 @@ class Hdui_ValueItemModel : public QAbstractItemModel
if (index.column() == 0) {
std::ostringstream buffer;
buffer << _value;
return QVariant(buffer.str().data());
return QVariant(QLatin1String(buffer.str().data()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we use a string type that supports UTF-8 identifiers? Tagging @erslavin for thoughts

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point; in fact it turns out that using UTF-8 strings is actually what Qt5 is doing under the hood in those cases (see the Qt5 documentation for QVariant, confirmed by a Qt employee). We've changed it to do the same, just explicitly, so that it works for both Qt5 and Qt6, and regardless of whether QT_NO_CAST_FROM_ASCII is set or not.

@erikaharrison-adsk
Copy link
Contributor Author

Looks like the build failed and seems unrelated to our changes. Any way to re-trigger, or should it be investigated further?

@erikaharrison-adsk
Copy link
Contributor Author

Hi @jesschimein or is there another person I should tag? Any update on this PR? We've a shipping product that's looking to incorporate this work. Let us know if there's anything on our end we can do to help this along.

@jesschimein
Copy link
Contributor

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@tcauchois
Copy link
Contributor

Hey all, internal feedback is that the API tags should go on (non-inlined) methods rather than classes. Can you update the PR accordingly? Other than that, this looks good.

@debloip-adsk
Copy link
Contributor

debloip-adsk commented May 1, 2024

@tcauchois Unfortunately these files use the Q_OBJECT macro, which makes that approach not super viable. The macro declares a static member variable and additional methods without API tags (see Q_OBJECT definition in qtmetamacros.h), which prevents us from controlling their export visibility individually (and we need these to be exported for things to compile and link). This StackOverflow thread hit the same problem and a suggested solution was to use the PIMPL idiom, but that seems a bit unwieldy to me for the case at hand. Given that the official Qt docs about creating shared libraries also only mention applying the export tags on a class, I feel like the simplest way forward is to keep the API tags on the classes, though yeah it's unfortunate that this has to break from the convention followed by the rest of the codebase.

@tcauchois
Copy link
Contributor

We talked this through a bunch and while we don't want to make a general exception (class-based exports have caused tons of issues on other projects), we're adding a specific exception for subclasses of Qt classes, since the consensus matches your PR.

For code-search purposes, could you add a new macro HDUI_API_CLASS that's a synonym of HDUI_API (should only be needed for hdui/api.h) and use that instead?

Thanks!

@jesschimein
Copy link
Contributor

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@pixar-oss pixar-oss merged commit 637f8ab into PixarAnimationStudios:dev Jun 4, 2024
3 of 5 checks passed
@erikaharrison-adsk erikaharrison-adsk deleted the adsk/feature/hdui_usable_in_independent_builds branch October 5, 2024 01:14
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.

6 participants