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

Move shaders into shader library target #1581

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

Conversation

colincornaby
Copy link
Contributor

This PR does several things:

  • It moves the shaders into a proper Metal shader library target when using the Xcode generator.
  • It compiles the shaders twice - once for Metal 2.1 (macOS 10.14) and once for Metal 2.3 (macOS 11.0.) The Metal 2.3 build of the shaders can take advantage of special features when running on Apple Silicon.
  • plMetalDevice now dynamically loads the correct compiled shader archive based on the host version of macOS
  • The shader library is now cached instead of being re-created each time a shader is needed. (This change is cleanup thats long overdue.)

I'd like some feedback on the implementation in the CMake - and feedback on this change requiring CMake 3.28.

{
NSURL* shaderURL = [NSBundle.mainBundle URLForResource:@"pfMetalPipelineShadersMSL21" withExtension:@"metallib"];
fShaderLibrary = fMetalDevice->newLibrary(static_cast<NS::URL*>(shaderURL), &error);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need to add an assert here against error.

@colincornaby colincornaby force-pushed the build-metal-shaders-as-library branch from 91220e8 to bcb2dc4 Compare April 8, 2024 07:18
NS::Error* error;
if (@available(macOS 11, *))
{
NSURL* shaderURL = [NSBundle.mainBundle URLForResource:@"pfMetalPipelineShadersMSL23" withExtension:@"metallib"];
Copy link
Member

@dpogue dpogue Apr 8, 2024

Choose a reason for hiding this comment

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

In theory, this is roughly equivalent to:

plFileName::Join(NSBundle::mainBundle()->resourceURL(), ST_LITERAL("pfMetalPipelineShadersMSL23.metallib"))

(which I realize is not the "correct" way to do things, but does workaround the bizarre oversight in the metal-cpp library)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll probably leave it the way it is for now (we're already doing something similar for the performance shaders.) But I'll revisit if Apple fixes the missing functions.

Copy link
Member

@Hoikas Hoikas left a comment

Choose a reason for hiding this comment

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

You probably need to assert in the metal pipeline that the CMake version is at the minimum required for this feature to work.

Sources/Plasma/FeatureLib/pfMetalPipeline/plMetalDevice.h Outdated Show resolved Hide resolved
Comment on lines +119 to +122
list(APPEND plClient_SHADERTARGETS
pfMetalPipelineShadersMSL21
pfMetalPipelineShadersMSL23
)
Copy link
Member

Choose a reason for hiding this comment

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

This should probably be pulled in transitively from pfMetalPipeline target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved what I could but this specific part (the XCODE_EMBED_RESOURCES target property) is not transitive.

@colincornaby
Copy link
Contributor Author

You probably need to assert in the metal pipeline that the CMake version is at the minimum required for this feature to work.

What would be the best way to do that? A different cmake_minimum_required or some sort of different check?

@Hoikas
Copy link
Member

Hoikas commented Apr 9, 2024

What would be the best way to do that? A different cmake_minimum_required or some sort of different check?

We may not want to use cmake_ minimum_required() because this has side effects related to setting policies, and we may not want to have different policy scopes playing with our mind. It may be better to test and error if the version is too low:

if(${CMAKE_VERSION} VERSION_LESS 3.9)
    message(FATAL_ERROR "boo")
endif()

@colincornaby colincornaby force-pushed the build-metal-shaders-as-library branch from 383ab5d to d8545bb Compare April 18, 2024 05:17
@colincornaby colincornaby marked this pull request as ready for review April 18, 2024 05:25
@colincornaby
Copy link
Contributor Author

Just checking in on this PR. I made some changes a few weeks back but haven't seen any feedback yet.

@Hoikas
Copy link
Member

Hoikas commented May 1, 2024

Looks like this PR needs to be rebased to build.

@colincornaby colincornaby force-pushed the build-metal-shaders-as-library branch from b9a1837 to 9484a07 Compare May 1, 2024 00:58
@colincornaby
Copy link
Contributor Author

I went ahead and rebased pulling in @dpogue's latest Mac fixes. Waiting to see if the build is green.

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

3 participants