Migrate the repository to a virtual workspace. #590
Conversation
vello/src/shaders.rs
Outdated
| @@ -62,7 +62,7 @@ pub fn full_shaders( | |||
| #[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.
|
|
||
| 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.
README.md
Outdated
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?
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.