Skip to content

Support loading shaders from a FILE and as SPIRV-BIN#1056

Closed
nhaehnle wants to merge 2 commits intogoogle:mainfrom
nhaehnle:file-and-spirv-bin
Closed

Support loading shaders from a FILE and as SPIRV-BIN#1056
nhaehnle wants to merge 2 commits intogoogle:mainfrom
nhaehnle:file-and-spirv-bin

Conversation

@nhaehnle
Copy link
Contributor

This is convenient in a number of cases. For example, one can more conveniently refer to a standalone shader source file. And one can pass-through a SPIR-V file that may be using a shader-only extension that Amber is unaware of.

This is convenient in a number of cases. For example, one can more
conveniently refer to a standalone shader source file. And one can
pass-through a SPIR-V file that may be using a shader-only extension
that Amber is unaware of.
@google-cla
Copy link

google-cla bot commented Oct 10, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

}

amber::Result LoadFile(const std::string, std::vector<char>&) const override {
return Result("DummyDelegate::LoadFile not implemented");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
return Result("DummyDelegate::LoadFile not implemented");
return Result("PlaceholderDelegate::LoadFile not implemented");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you sure? The class name is DummyDelegate.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, you're right. I'd thought I'd renamed that already. Leave this and I'll rename the delegate in the future.

@nhaehnle
Copy link
Contributor Author

nhaehnle commented Oct 11, 2024

Thank you for taking a look!

I tried to address the check-format complaint.

The license check complains that shader_spirv_bin.spv doesn't have a license. I suppose that's true, but given it's a binary file that may be difficult to satisfy. And while checking in binary files is obviously usually a bit questionable, it's in the nature of the test. Do you have any suggestions?

@dj2
Copy link
Collaborator

dj2 commented Oct 11, 2024

Hm, I've been looking but can't find the documentation for license-check. I also don't have admin, and am unable to ignore the check. @dneto0 do you have the ability to ignore the failing required check and merge this regardless? (or do you know the magic to make license-checker skip the binary file?)

@dj2
Copy link
Collaborator

dj2 commented Oct 24, 2024

ping @dneto0

@dneto0
Copy link
Collaborator

dneto0 commented Nov 19, 2024

Hm, I've been looking but can't find the documentation for license-check. I also don't have admin, and am unable to ignore the check. @dneto0 do you have the ability to ignore the failing required check and merge this regardless? (or do you know the magic to make license-checker skip the binary file?)

I can't bypass the license checker.
However, we should be able to exclude .spv files by modifying the license-checker.cfg in the project root dir.

I'll do it in a new PR.

dneto0 added a commit to dneto0/amber that referenced this pull request Nov 19, 2024
They are SPIR-V binaries.

This should allow google#1056 to land
dj2 pushed a commit that referenced this pull request Nov 20, 2024
They are SPIR-V binaries.

This should allow #1056 to land
@dneto0
Copy link
Collaborator

dneto0 commented Nov 20, 2024

Landed as #1059

@dneto0 dneto0 closed this Nov 20, 2024
@nhaehnle nhaehnle deleted the file-and-spirv-bin branch November 25, 2024 11:05
@nhaehnle
Copy link
Contributor Author

Thanks!

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.

4 participants