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.

Sign up for GitHub

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

Jump to bottom

Consider what to do with wgpu_trace feature #14725

Closed
IceSentry opened this issue Aug 12, 2024 · 6 comments · Fixed by #14842
Closed

Consider what to do with wgpu_trace feature #14725

IceSentry opened this issue Aug 12, 2024 · 6 comments · Fixed by #14842
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Milestone
0.15

Comments

@IceSentry
Copy link
Contributor

IceSentry commented Aug 12, 2024 •
edited
Loading

Description

Wgpu 22 removed the wgpu/trace feature, but the PR to update to wgpu 22 for bevy kept the wgpu_trace feature. We should either remove it or add a compile warning that the feature currently doesn't do anything.

#14401 (comment)

wgpu issue gfx-rs/wgpu#5974

Since this is only temporary, we could consider keeping it in with just a warning. Personally I think we should just remove it.
@IceSentry IceSentry added C-Docs An addition or correction to our documentation C-Code-Quality A section of code that is hard to understand or change labels Aug 12, 2024
@IceSentry IceSentry changed the title Consider what to do with wgpu_trace Consider what to do with wgpu_trace feature Aug 12, 2024
@JMS55 JMS55 added this to the 0.15 milestone Aug 12, 2024
@LikeLakers2
Copy link
Contributor

As per my comment over at #14401 (comment), I personally think it should be removed.

This is because, as far as I'm aware, bevy/wgpu_trace is solely a proxy to wgpu/trace, and doesn't enable any functionality in bevy itself. Having a feature that's solely a proxy seems weird to me - if a user needs wgpu/trace, they can pull in wgpu and enable the trace feature in their own crate.

This does mean that the user will have to update dependencies.wgpu.version every time bevy updates, in order to ensure it actually works... but bevy updates don't come every day, so that effort only needs to happen every once in a while.

@janhohenheim
Copy link
Member

@LikeLakers2 if you pull in a dependency of a dependency, you can use version = * to make it automatically match what you are pulling in anyways.

@TrialDragon
Copy link
Member

TrialDragon commented Aug 19, 2024 •
edited
Loading

This is because, as far as I'm aware, bevy/wgpu_trace is solely a proxy to wgpu/trace, and doesn't enable any functionality in bevy itself.

It technically does I think? Looking at it, it doesn't even seem to enable the wgpu/trace feature from what I can see on a cursory glance (I missed that it was removed when we updated to wgpu 22), gets used in https://github.com/bevyengine/bevy/blob/main/crates/bevy_render/src/renderer/mod.rs#L198-L206 to pass a tracing path for wgpu to use.

@LikeLakers2
Copy link
Contributor

LikeLakers2 commented Aug 20, 2024 •
edited
Loading

@TrialDragon Ahh, good catch.

If I might propose a solution then... perhaps allow that tracing path to be set as a field on bevy::render::settings::WgpuSettings? That way, the user can customize it and bevy no longer needs to worry about whether the feature is enabled - it just passes the value in as-is, and wgpu will handle it if wgpu/trace is enabled.

@TrialDragon
Copy link
Member

That sounds good to me. Presuming there are no objections, this seems ready for implementation, then.

@TrialDragon TrialDragon added A-Rendering Drawing game state to the screen S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it! D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Aug 20, 2024
@LikeLakers2
Copy link
Contributor

@TrialDragon I have little experience with bevy's codebase - but I will try making this change.

@LikeLakers2 LikeLakers2 mentioned this issue Aug 21, 2024
Merged
github-merge-queue bot pushed a commit that referenced this issue Aug 25, 2024
@LikeLakers2 @TrialDragon
Replace the wgpu_trace feature with a field in `bevy_render::settin…
94d40d2 
…gs::WgpuSettings` (#14842)

# Objective
- Remove the `wgpu_trace` feature while still making it easy/possible to
record wgpu traces for debugging.
- Close #14725.
- Get a taste of the bevy codebase. :P

## Solution
This PR performs the above objective by removing the `wgpu_trace`
feature from all `Cargo.toml` files.

However, wgpu traces are still useful for debugging - but to record
them, you need to pass in a directory path to store the traces in. To
avoid forcing users into manually creating the renderer,
`bevy_render::settings::WgpuSettings` now has a `trace_path` field, so
that all of Bevy's automatic initialization can happen while still
allowing for tracing.

## Testing
- Did you test these changes? If so, how?
- I have tested these changes, but only via running `cargo run -p ci`. I
am hoping the Github Actions workflows will catch anything I missed.
- Are there any parts that need more testing?
  - I do not believe so.
- How can other people (reviewers) test your changes? Is there anything
specific they need to know?
- If you want to test these changes, I have updated the debugging guide
(`docs/debugging.md`) section on WGPU Tracing.
- If relevant, what platforms did you test these changes on, and are
there any important ones you can't test?
- I ran the above command on a Windows 10 64-bit (x64) machine, using
the `stable-x86_64-pc-windows-msvc` toolchain. I do not have anything
set up for other platforms or targets (though I can't imagine this needs
testing on other platforms).

---

## Migration Guide

1. The `bevy/wgpu_trace`, `bevy_render/wgpu_trace`, and
`bevy_internal/wgpu_trace` features no longer exist. Remove them from
your `Cargo.toml`, CI, tooling, and what-not.
2. Follow the instructions in the updated `docs/debugging.md` file in
the repository, under the WGPU Tracing section.

Because of the changes made, you can now generate traces to any path,
rather than the hardcoded `%WorkspaceRoot%/wgpu_trace` (where
`%WorkspaceRoot%` is... the root of your crate's workspace) folder.

(If WGPU hasn't restored tracing functionality...) Do note that WGPU has
not yet restored tracing functionality. However, once it does, the above
should be sufficient to generate new traces.

---------

Co-authored-by: TrialDragon <[email protected]>
@torokati44 torokati44 mentioned this issue Aug 26, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Implementation This issue is ready for an implementation PR. Go for it!
Projects
None yet
Milestone
0.15
Development

Successfully merging a pull request may close this issue.

Replace the wgpu_trace feature with a field in bevy_render::settings::WgpuSettings LikeLakers2/bevy
5 participants
@LikeLakers2 @IceSentry @janhohenheim @TrialDragon @JMS55