Migrate the repository to a virtual workspace. #590
Conversation
vello/src/shaders.rs
Outdated
| //let force_gpu_from = Some("binning"); | ||
|
|
||
| #[cfg(feature = "hot_reload")] | ||
| let mut shaders = vello_shaders::compile::ShaderInfo::from_dir(concat!( |
There was a problem hiding this comment.
Can vello_shaders instead export the path to directory the shaders are located in?
There was a problem hiding this comment.
You mean the shader directory name, or some more absolute path?
There was a problem hiding this comment.
Alright I added a new function that returns an absolute path to the shader directory. It depends on using Cargo and the path is computed at compile time.
If someone has a build cache and then moves the directories around, they will need to cargo clean or trigger a rebuild in another way.
| /// The path is determined at compile time. | ||
| pub fn shader_dir() -> &'static PathBuf { | ||
| static SHADER_DIR: OnceLock<PathBuf> = OnceLock::new(); | ||
| SHADER_DIR.get_or_init(|| Path::new(env!("CARGO_MANIFEST_DIR")).join("shader")) |
There was a problem hiding this comment.
If we used concat!, I don't think the runtime string manipulation would be needed.
I guess this is about using the proper separator?
There was a problem hiding this comment.
Just using concat! is buggy because we don't know if CARGO_MANIFEST_DIR ends with a separator or not. Instead of doing some sort of .ends_with dance and potentially missing some other edge cases, better to use the path module.
Also, this is a singleton. The performance overhead here is trivial.
vello_shaders/src/compile/mod.rs
Outdated
|
|
||
| /// Returns the absolute path to the directory containing the WGSL shaders. | ||
| /// | ||
| /// The path is determined at compile time. |
There was a problem hiding this comment.
And may be invalid at runtime, especially if the program is deployed on a different machine
As a different point:
This is also a reproducible builds challenge, which I guess we should document?
Idk, maybe even making it doc(hidden)
There was a problem hiding this comment.
Sure, I added a note about the impact on reproducible builds.
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
Will give a final check on Monday if this hasn't landed by then
|
|
||
| It is used as the rendering backend for [Xilem], a Rust GUI toolkit. | ||
|
|
||
| > [!WARNING] |
There was a problem hiding this comment.
We should change the formatting of this now, to use something which renders well on crates.io
That might just be using the
There was a problem hiding this comment.
Updating readme files is intentionally out of scope and planned as follow-up work to reduce the review friction here.
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
Thanks for getting this done. A few small nits, I don't think any are necessarily blocking
There was a problem hiding this comment.
I'd add a big comment at the top that this is "temporarily" duplicated in the vello folder, and updates should be preferred there.
|
|
||
| at your option. | ||
|
|
||
| In addition, all files in the [`vello_shaders/shader`](https://github.com/linebender/vello/tree/main/vello_shaders/shader) and [`vello_shaders/src/cpu`](https://github.com/linebender/vello/tree/main/vello_shaders/src/cpu) |
There was a problem hiding this comment.
These additional caveats are not important true for vello itself. I imagine this has the same reasoning as above though?
2 participants
The CI, VSCode config,
ARCHITECTURE.mdand someREADME.mdlinks have been updated to reflect the new directory structure.The WGSL shaders now live in the
vello_shaderssubdirectory. Vello hot reloading depends on our repo structure, which is fine as decided at last office hours due to this being a development feature.The examples continue to live in a root
examplesdirectory, as that is how it remained for Parley as well. I did this for consistency and don't really feel strongly either way if people think we should move them undervello/examplesinstead. If we had examples withoutCargo.tomlthat would make more sense, but we don't, so ..The repo
README.mdand thevellosub-directoryREADME.mdare exact clones and both need updating. That is intentionally planned as follow-up work to reduce the review friction here.