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

Use framework linkage for OpenGL on macOS #1741

Conversation

dgovil
Copy link
Contributor

@dgovil dgovil commented Mar 16, 2024

This commit fixes the output of the MaterialXTargets.cmake to have proper system linkage for OpenGL in the cmake config.
Without this change, downstream projects on a Mac like USD cannot build against MaterialX properly.

There was an issue introduced in b50dea8 where the output started being like so

set_target_properties(MaterialXRenderGlsl PROPERTIES
  INTERFACE_COMPILE_DEFINITIONS "GLAD_GLAPI_EXPORT"
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
  INTERFACE_LINK_LIBRARIES "MaterialXRenderHw;MaterialXGenGlsl;OpenGL::GL;-framework Foundation;-framework Cocoa;-framework Metal"
)

Prior to that commit it would end up being a hardcoded path to the SDK which is also not great, because it means any SDK update would make the file unusable.

/Library/Developer/CommandLineTools/SDKs/MacOSX13.5.sdk/System/Library/Frameworks/OpenGL.framework;

Switching to -framework like the rest of the system links fixes that and makes it portable regardless of what SDK paths are on a system

Issue introduced in b50dea8

Signed-off-by: Dhruv Govil <dgovil2@apple.com>
@jstone-lucasfilm
Copy link
Member

This looks like a good improvement, thanks @dgovil!

Since we're currently focusing on MaterialX 1.39 for release this summer, would you mind retargeting this pull request to the dev_1.39 branch, and we can review it there?

@dgovil dgovil changed the base branch from main to dev_1.39 March 16, 2024 05:58
@dgovil
Copy link
Contributor Author

dgovil commented Mar 16, 2024

Changed the target to dev_1.39, though I do think the bug its fixing should be considered a breaking change in terms of letting other projects compile against MaterialX 1.38.

I'm not sure if there's room for patch updates, like a 1.38.9.1 or just a bug fix 1.38.10 release to back port fixes?
If not for this, I think it would make sense in the future just to alleviate possible issues for people who aren't ready to make as big a jump up.

@jstone-lucasfilm
Copy link
Member

@dgovil It should be fine to create a new 1.38.10 release as well, if there are important fixes that need to be included in the 1.38.x series, but so far we've been focused on the 1.39 release this summer. Let us know if you believe this change is important enough to warrant an additional 1.38.10 release, and we can bring that up in the next MaterialX TSC.

@dgovil
Copy link
Contributor Author

dgovil commented Mar 16, 2024

I think it might be worth having a 1.38.10 to back port some fixes like this. My worry is this one makes it harder for macOS devs to use 1.38.9, and I'm not sure they'd be ready to move to 1.39

I can bring it up in the next TSC meeting.

@jstone-lucasfilm jstone-lucasfilm changed the title Fix OpenGL linkage on macOS to use framework link. Use framework link for OpenGL on macOS Mar 16, 2024
Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks good to me, thanks @dgovil!

@jstone-lucasfilm jstone-lucasfilm changed the title Use framework link for OpenGL on macOS Use framework linkage for OpenGL on macOS Mar 16, 2024
@jstone-lucasfilm jstone-lucasfilm merged commit 7ede137 into AcademySoftwareFoundation:dev_1.39 Mar 16, 2024
31 checks passed
kwokcb added a commit to kwokcb/MaterialX that referenced this pull request Mar 26, 2024
* Use framework linkage for OpenGL on macOS (AcademySoftwareFoundation#1741)

This commit fixes the output of the MaterialXTargets.cmake to have proper system linkage for OpenGL in the cmake config.
Without this change, downstream projects on a Mac like USD cannot build against MaterialX properly.

* Static analysis fixes

This changelist addresses a handful of static analysis warnings flagged by Cppcheck and MSVC.

* Move dynamic analysis tests to MacOS (AcademySoftwareFoundation#1743)

This changelist moves dynamic analysis tests to MacOS in GitHub Actions, allowing them to take advantage of more recent compiler environments and hardware.

* Add environment light intensity in GLSL (AcademySoftwareFoundation#1737)

Introduce a new uniform (u_envLightIntensity) that allows applications to control the light intensity directly in generated shaders. The uniform is a linear multiplier on environment lights. Applications can control the value of the uniform as needed.

* Remove 'baked' suffix from TextureBaker names (AcademySoftwareFoundation#1744)

The texturebaker appended a "_baked" token suffix to the generated material names that changed material name. Some pipelines get impacted by this rename, since the original material name is lost. Removing the suffix to retain original material name.

* Update development build to 1.38.10

* Move prefilter functions to referencing GLSL files

This changelist moves environment prefilter functions into the GLSL files that reference them, addressing shader compilation errors when GenOptions::hwDirectionalAlbedoMethod is set to DIRECTIONAL_ALBEDO_TABLE or DIRECTIONAL_ALBEDO_MONTE_CARLO by the client.

* Move light uniforms into referencing GLSL files (AcademySoftwareFoundation#1750)

This change moves the new environment light intensity uniforms into their referencing GLSL files, addressing shader compilation errors when GenOptions::hwDirectionalAlbedoMethod is set to DIRECTIONAL_ALBEDO_TABLE or DIRECTIONAL_ALBEDO_MONTE_CARLO by the client.

Additionally, the change moves the environment light intensity option from the UI to the command line of the viewer, since isolated adjustments of the indirect lighting term are an unintuitive rendering control for most users.

---------

Co-authored-by: Dhruv Govil <dgovil2@apple.com>
Co-authored-by: Jonathan Stone <jstone@lucasfilm.com>
Co-authored-by: Ashwin Bhat <1727158+ashwinbhat@users.noreply.github.com>
jstone-lucasfilm pushed a commit that referenced this pull request Mar 26, 2024
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.

None yet

2 participants