Skip to content

Support building with objc-arc, enable by default.#3226

Open
furby-tm wants to merge 2 commits intoPixarAnimationStudios:devfrom
wabiverse:wabi-swift-clang-objc-arc
Open

Support building with objc-arc, enable by default.#3226
furby-tm wants to merge 2 commits intoPixarAnimationStudios:devfrom
wabiverse:wabi-swift-clang-objc-arc

Conversation

@furby-tm
Copy link
Copy Markdown
Contributor

@furby-tm furby-tm commented Aug 13, 2024

  • Changes to support building with ObjC ARC (Automatic Reference Counting), also known as Clang compiler flag -fobjc-arc.

Description of Change(s)

  • Modified the CMake build to set target compile options on the hgiMetal and garch targets, to enable -fobjc-arc by default on Apple platforms, when the compiler matches any of the Clang derivatives.

  • HgiMetal was originally not compiling without this revision, it appears the usage of MTLResource here was not what was intended as far as I'm aware -- at least from the following call made to didModifyRange, which does not exist on MTLResource but it does exist on MTLBuffer, so I have changed it to MTLBuffer instead. I believe this means we can get rid of these Arch pragmas as well.

  • Guarded all retain and release calls with !__has_feature(objc_arc) which are otherwise a compiler error, forbidden when ARC is enabled.

  • HgiMetal: Modified the implicit casting from GetRawResource() -> uint64_t to the id<MTLTexture> variables (colorTexture and depthTexture) within HgiInteropMetal.CompositeToInterop() to first cast them to void *, and then to id<MTLTexture> bridging casts.
    Edit 8.12.24 10:05AM (PDT): Minor revision, to use a reinterpret_cast instead of the C-style cast.

  • Garch: Modified the return statement of GarchSelectCoreProfileMacVisual() to do a void * bridging cast.


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

Signed-off-by: furby™ <devs@wabi.foundation>
@jesschimein
Copy link
Copy Markdown
Collaborator

Filed as internal issue #USD-9971

@jesschimein
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@dgovil
Copy link
Copy Markdown
Collaborator

dgovil commented Aug 21, 2024

Thanks for putting this up, this is a great change.

I think we had a couple notes:

  1. It would be good to just enable ARC across the board. It's generally preferable, and cleansup the code. It also reduces the risk of someone down the road accidentally forgetting which mode they're in and causing issues.
  2. I don't think you need to check for both APPLE and CLANG. I don't believe USD supports APPLE without CLANG, so you could treat any if (APPLE) as being implicitly CLANG as well.

I think the two notes would simplify a lot of things going forward.
@meshula , we didn't have any concerns with enabling ARC on our end (and truthfully, we probably should have from the get go). Would you have any issues with ARC just being turned on all the time (on APPLE)?

@meshula
Copy link
Copy Markdown
Member

meshula commented Aug 21, 2024

I'm not aware of any objections to turning on ARC all the time on Apple, or on non-Apple deployments of Swift or ObjC.

@pixar-oss
Copy link
Copy Markdown
Member

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

@furby-tm
Copy link
Copy Markdown
Contributor Author

furby-tm commented Aug 23, 2024

@dgovil thanks for looking into this, if ARC is to be enabled by default is this something that should be added to the clang defaults here?

Under the existing if (APPLE) condition per your suggestion.

If so, then I can additionally cleanup (remove) any of my revisions to the existing cmake code.

AFAIK this revision covers any of the few minor breaking changes that enabling ARC will have on the codebase, but will test this theory once changing the clang defaults to always enable ARC on apple platforms.

@dgovil
Copy link
Copy Markdown
Collaborator

dgovil commented Aug 23, 2024

Yeah, I think putting it in the clang defaults makes the most sense to me. That would mean any future targets just automatically get it as a result, so nobody needs to remember to enable it.

Signed-off-by: furby™ <devs@wabi.foundation>
@furby-tm
Copy link
Copy Markdown
Contributor Author

Everything checks out on my end, now with ARC enabled in the clang defaults.

@dgovil
Copy link
Copy Markdown
Collaborator

dgovil commented Aug 23, 2024

Awesome! I look forward to this landing. Thanks again for catching that and doing the work for it.

@jesschimein
Copy link
Copy Markdown
Collaborator

/AzurePipelines run

@azure-pipelines
Copy link
Copy Markdown

Azure Pipelines successfully started running 1 pipeline(s).

furby-tm added a commit to wabiverse/USD that referenced this pull request Sep 9, 2024
* Note blitCmds.mm is still using the wrong MTLResource
  type on L.361, which should be MTLBuffer, but that issue
  is addressed in the following PR:
  PixarAnimationStudios#3226

Signed-off-by: furby™ <devs@wabi.foundation>
dgovil pushed a commit to dgovil/USD that referenced this pull request Sep 11, 2024
* Note blitCmds.mm is still using the wrong MTLResource
  type on L.361, which should be MTLBuffer, but that issue
  is addressed in the following PR:
  PixarAnimationStudios#3226

Signed-off-by: furby™ <devs@wabi.foundation>
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