📚Separate main() into its own function in lib.rs #5
Conversation
|
I have not tested this on Windows yet, and there seem to be some issues the CI caught |
FlannyH
requested review from
Jasper-Bekkers,
a team and
tosti007
and removed request for
a team
| @@ -1,2 +1,5 @@ | |||
| /target | |||
| /Cargo.lock | |||
| .DS_Store | |||
| .vscode/launch.json | |||
There was a problem hiding this comment.
Maybe just add the entire .vscode folder?
| @@ -601,3 +597,149 @@ impl<'lib> IRCompiler<'lib> { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
| // Moved from main.rs to lib.rs | |||
There was a problem hiding this comment.
Nit: this comment doesn't really add anything.
| pub fn compile_dxil_to_metallib_from_path( | ||
| path_compiler_lib: &Path, | ||
| path_dxil: &Path, | ||
| ) -> Result<Vec<u8>, Box<dyn std::error::Error>> { |
There was a problem hiding this comment.
If you're turning these into public functions, they should have doc-comments.
| path_dxil: &Path, | ||
| ) -> Result<Vec<u8>, Box<dyn std::error::Error>> { | ||
| // Load DXIL binary from path | ||
| let dxil_binary = std::fs::read(path_dxil)?; |
There was a problem hiding this comment.
Since you're only reading a file into binary, let the user pass a byte slice. This would make it compatible with our asset pipeline that does not give you access to plain files.
I.e. there is no point to provide a "convenience _from_path() variant, only the byte-slice version suffices.
Perhaps the same for the compiler library itself. Even better: how expensive is it to load the compiler and interface for every shader? Perhaps we need to provide an object-based API of this sort:
let compiler = MetalCompiler::new("path/to/libirmetallibbinary.so")?; // TODO: Should there be a function with a sensible default path? Is it available on the system?
let _compiled = compiler.compile(&[0x13, 0x37, ...])?;| print!("Testing shader \"{shader_path}\": "); | ||
| match compile_dxil_to_metallib_from_path(&Path::new("/usr/local/lib/libmetalirconverter.dylib"), &Path::new(shader_path)) { | ||
| Ok(_) => println!("\tOk."), | ||
| Err(e) => println!("Error: {e:?}"), |
There was a problem hiding this comment.
Let's return anyhow::Result from fn main()?
| /// Takes a DXIL binary, cross-compiles it to metal and returns a metallib binary | ||
| pub fn compile_dxil_to_metallib( | ||
| path_compiler_lib: &Path, | ||
| dxil_binary: &Vec<u8>, |
There was a problem hiding this comment.
Clippy will probably complain that this should be a slice, instead of a borrow of a forcibly-owned Vec.
| } | ||
|
|
||
| /// Takes a DXIL binary, cross-compiles it to metal and returns a metallib binary | ||
| pub fn compile_dxil_to_metallib( |
There was a problem hiding this comment.
None of this code should live in saxaboom, since it's specific to how breda sets up it's bindings.
|
This entire PR can be closed. All the functionality added here was also added in the breda repository, making this redundant |
|
Sounds good! It looks like all of this example code is more-or-less add-hoc depending on project-specifics :) |
Refactored the main() function into its own function in lib.rs, so it's also accessible when used as a library rather than a standalone application.
compile_dxil_to_metallib()takes in a Vec<u8> containing a DXIL binary, compiles it, and returns a Vec<u8> containing a Metallib binarycompile_dxil_to_metallib_from_path()takes in a path to a DXIL binary file, and returns a Vec<u8> containing a Metallib binary