Skip to content
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

Should timestamp_period on RendererOptions be behind the wgpu-profiler feature flag? #328

Closed
bzm3r opened this issue Jun 4, 2023 · 3 comments

Comments

@bzm3r
Copy link

bzm3r commented Jun 4, 2023

The RendererOptions struct has an additional timestamp_period member due to #304:

https://github.com/linebender/vello/blob/6d57093cc275589843766b8c898793f8147ed323/src/lib.rs#LL81C4-L81C4

As the doc comment explains, timestamp_period is used when the wgpu-profiler. Should the timestamp_period member therefore be put behind the wgpu-profiler flag, like so:

pub struct RendererOptions {
    /// The format of the texture used for surfaces with this renderer/device
    /// If None, the renderer cannot be used with surfaces
    pub surface_format: Option<TextureFormat>,
    /// The timestamp period from [`wgpu::Queue::get_timestamp_period`]
    /// Used when the wgpu-profiler feature is enabled
    #[cfg(feature = "wgpu-profiler")]
    pub timestamp_period: f32,
}

Or is it not being behind the feature flag intentional?

@DJMcNab
Copy link
Member

DJMcNab commented Jun 4, 2023

The intention there was to not require code changes on the user side because wgpu-profiler was enabled.

For example, if a library depends on Vello , and then a crate depends on that library and Vello, then enables profiling in the top-level crate would break the library, if it managed its own renderers.

Perhaps we should instead accept a reference to the queue in the options? It's a bit messy any way around

@bzm3r
Copy link
Author

bzm3r commented Jun 4, 2023

Hmm, I think I understand.

Is there any prior art here that might be relevant?

@DJMcNab
Copy link
Member

DJMcNab commented Feb 20, 2024

That field was removed in #398

Thanks for your well documented feature request

@DJMcNab DJMcNab closed this as completed Feb 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants