OpenUSD icon indicating copy to clipboard operation
OpenUSD copied to clipboard

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

Open erikaharrison-adsk opened this issue 2 years ago • 10 comments

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
  • [X] I have verified that all unit tests pass with the proposed changes
  • [X] I have submitted a signed Contributor License Agreement

erikaharrison-adsk avatar Sep 13 '23 17:09 erikaharrison-adsk

Filed as internal issue #USD-8704

jesschimein avatar Sep 14 '23 18:09 jesschimein

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

erikaharrison-adsk avatar Oct 02 '23 15:10 erikaharrison-adsk

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.

erikaharrison-adsk avatar Feb 12 '24 23:02 erikaharrison-adsk

/AzurePipelines run

jesschimein avatar Apr 01 '24 21:04 jesschimein

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar Apr 01 '24 21:04 azure-pipelines[bot]

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.

tcauchois avatar Apr 17 '24 23:04 tcauchois

@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.

debloip-adsk avatar May 01 '24 16:05 debloip-adsk

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!

tcauchois avatar May 03 '24 18:05 tcauchois

/AzurePipelines run

jesschimein avatar May 08 '24 17:05 jesschimein

Azure Pipelines successfully started running 1 pipeline(s).

azure-pipelines[bot] avatar May 08 '24 17:05 azure-pipelines[bot]