docs: update vello hybrid readme and lib.rs to include feature-flags#1359
Conversation
There was a problem hiding this comment.
Welcome! Thank you for addressing this issue; this implementation is good (barring the one confusing comment about the standard error traits).
However, it's uncovered a different issue that the std feature flag on this crate doesn't make sense (std is not default, it enables wgpu, and we also unconditionally enable Vello Common's std feature).
To let us land this, can you please just remove the mention of the std feature entirely from these docs, and I will remove it entirely as a follow-up.
| //! | ||
| //! - `wgpu` (enabled by default): Enables the GPU rendering backend via wgpu and includes the required sparse shaders. | ||
| //! - `wgpu_default` (enabled by default): Enables wgpu with its default hardware backends (such as Vulkan, Metal, and DX12). | ||
| //! - `std`: Enables the standard library for integration with vello_common and standard error traits. |
There was a problem hiding this comment.
Can you clarify why you think this feature enables the standard error traits? I've not been able to find the code which does so. The reason I checked is that the Error trait also moved to core earlier than our MSRV, so it would have been a bug if we did so.
There was a problem hiding this comment.
Okay, I've updated the PR and removed the std feature flag mentions from the readme and the lib.rs file.
My apologies for the confusion regarding the error trait part in the documentation. Since the flag linked to vello_common/std, I made the assumption that it was a bridge for no_std compatibility, specifically for the error trait, which is a common pattern. I've cleared that out now.
Thanks for pointing that out to me. I shouldn't have made that assumption, though.
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
Thanks! I've pushed one small tweak, and will now land this.
2 participants
Resolves #1355
I added the feature flags for Vello Hybrid in the README and also in its
lib.rs, which weren't thereI had to add a new features flag section since it already has a features section, but I didn't want to mix it up
I also added the note from the
Cargo.tomlabout the default vs wgpu configuration, which seemed like a useful detail.This is my first time contributing here, so let me know if the formatting looks off or if you'd rather I merge the two feature sections