Skip to content

/gfx

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

Added Metal shader library support #1175

Merged
merged 1 commit into from Feb 4, 2017

Conversation

@JohnColanduoni
Copy link
Member

commented Feb 4, 2017

Fixes #1168
@kvark

kvark approved these changes Feb 4, 2017
View changes

src/backend/metal/Cargo.toml
@@ -33,4 +33,4 @@ libc = "0.2"
objc = "0.1.8"
objc-foundation = "0.1"
bit-set = "0.4"
metal-rs = "0.1"
metal-rs = { git = "https://github.com/fkaa/metal-rs", rev = "5263898c2ce1fb8263653272014366eba45ef88d" }

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Feb 4, 2017

Member

Why do you need a git revision here?
We may just ask @fkaa to publish a new version instead.
Although, I don't mind patching this later on.

This comment has been minimized.

Sign in to view
Copy link
@JohnColanduoni

JohnColanduoni Feb 4, 2017

Author Member

That revision includes the merging of a pull request I recently submitted (to metal-rs) to add the binding for the newLibraryFromFile method.

I think it might be better to hold off because I plan to keep working on the metal backend which may require some more additions to the bindings, and the metal backend is not on crates.io itself at the moment, but that works too.
src/backend/metal/src/factory.rs
@@ -150,6 +151,64 @@ impl Factory {

self.device.new_depth_stencil_state(desc)
}

pub fn create_library<P: AsRef<Path>>
(&mut self,

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Feb 4, 2017

Member

that's some unusual code formatting

This comment has been minimized.

Sign in to view
Copy link
@JohnColanduoni

JohnColanduoni Feb 4, 2017

Author Member

Sorry, I was replicating what was done elsewhere in the file (e.g. Factory::create_buffer_immutable_raw on line 226) when there was a long return type that wouldn't fit if the usual line breaking (i.e. with &mut self on the same line) wouldn't work. How should I format it?
src/backend/metal/src/factory.rs Outdated
-> Result<ShaderLibrary, core::shade::CreateShaderError> {
use core::shade::CreateShaderError;

let lib = match self.device.new_library_with_file(file) {

This comment has been minimized.

Sign in to view
Copy link
@kvark

kvark Feb 4, 2017

Member

nit: might as well make this expression the last one:

match self.device.new_library_with_file(file) {
  Ok(lib) => ShaderLibrary { lib: lib },
  Err(err) => Err(CreateShaderError::CompilationFailed(err.into())),
}

This comment has been minimized.

Sign in to view
Copy link
@JohnColanduoni

JohnColanduoni Feb 4, 2017

Author Member

:P Good catch!
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 4, 2017

Thanks @JohnColanduoni !
Let me know if you don't want to address the minor issues I got, we can deal with them afterwards.
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 4, 2017

Thanks!
Formatting is fine, we'll clean everything up with rustfmt anyway.
@homu r+
@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 4, 2017

📌 Commit 244fbf9 has been approved by kvark
@JohnColanduoni

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Feb 4, 2017 •
edited

The functions wouldn't do much good if they were private XD
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 4, 2017

@homu r-
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 4, 2017

@homu r+
@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 4, 2017

📌 Commit 02fec8e has been approved by kvark
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 4, 2017

Could you squash it as well please?
@JohnColanduoni
Added Metal shader library support
Verified
This commit was signed with a verified signature.
JohnColanduoni John Colanduoni
GPG key ID: 65BD90012FADE2A7 Learn about signing commits
Loading status checks…
a50a180

@JohnColanduoni JohnColanduoni force-pushed the JohnColanduoni:metallib branch from 02fec8e to a50a180 Feb 4, 2017

@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 4, 2017

@homu r-
@kvark

This comment has been minimized.

Sign in to view
Copy link
Member

commented Feb 4, 2017

Thanks!
@homu r+
@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 4, 2017

📌 Commit a50a180 has been approved by kvark

homu added a commit that referenced this pull request Feb 4, 2017

@homu
Auto merge of #1175 - JohnColanduoni:metallib, r=kvark 
Added Metal shader library support

Fixes #1168
Loading status checks…
a40c23c
@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 4, 2017

⌛️ Testing commit a50a180 with merge a40c23c...
@homu

This comment has been minimized.

Sign in to view
Copy link
Contributor

commented Feb 4, 2017

☀️ Test successful - status

@homu homu merged commit a50a180 into gfx-rs:master Feb 4, 2017
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kvark kvark

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@JohnColanduoni @kvark @homu
You can’t perform that action at this time.