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

Tracking Issue: GPU type refactor and bevy_gpu split #7157

Open
kurtkuehnert opened this issue Jan 11, 2023 · 3 comments
Open

Tracking Issue: GPU type refactor and bevy_gpu split #7157

kurtkuehnert opened this issue Jan 11, 2023 · 3 comments
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR

Comments

@kurtkuehnert
Copy link
Contributor

kurtkuehnert commented Jan 11, 2023

There have been discussions about splitting up the bevy_render module.
As a first and pretty straightforward step, we can refactor our wgpu GPU abstraction into its own crate.

The idea is to pull the entire render_resources module with all the wgpu and encase reexports, as well as most of the code of the render module into the new bevy_gpu crate.

This raises two major design questions:

Naming

The refactor opens up the possibility for more precise and consistent type and variable names.
Currently, we use no prefix for some types (e.g. Buffer, ComputePipeline) or the Render prefix for the remaining ones (e.g. RenderDevice, RenderQueue).
I propose three alternatives:

1. Gpu Prefix

  • rexport all wgpu types with the Gpu prefix (e.g. GpuDevice, GpuBuffer, etc.)
  • use a similar gpu_ prefix for variable names (e.g. gpu_device, gpu_buffer, etc.)
  • see Rename Render* to Gpu* #6968

2. Prefixless

  • stick with the wgpu prefixless names (e.g. Device, Buffer, etc.)
  • do not prefix variable names as well (e.g. device, buffer)
  • see Drop Render* prefix #7109

3. Hybrid

  • only use the Gpu prefix for some of the types and variables, similar to our current implementation
  • how do we decide when to use which?
  • this makes the API pretty inconsistent IMO

A quick poll on discord showed that many people are in favor of this change.

Exporting

Additionally, since this refactor is a major breaking change we can reconsider our GPU type reexporting strategy (which was marked with a todo comment: TODO: decide where re-exports should go).
We have got two options:

A. Root Export

  • export all types at root level (e.g. bevy::gpu::TypeName)

B. Module Export

  • keep the current render_resource module and simply rename it to gpu_resource (e.g. bevy::gpu::gpu_resource::TypeName)
  • we could elevate some types like Device, Queue, etc. to the root level
  • again: how do we decide where which type belongs?

Solution

Both decisions are unrelated, so we can mix and match.

I think this refactor will take place in two phases.
First, the rename to either one of these three options described above.
Second, the split of all GPU-related types into the bevy_gpu crate and the reconsidered exporting strategy.

We could further subdivide these changes into smaller PRs, but this would break bevy main multiple times, and would probably confuse a lot of our users.

I prototyped phase one for both the Gpu Prefix in #6968 and the Prefixless alternative in #7109.

Based on that, phase two is implemented in PRs #7000 (Gpu Prefix & Module Export ) and #7111 (Prefixless & Root Export).

Let me know which propositions you like the most, or which alternatives I have not considered yet.
Please vote for your preferred strategy in this GitHub poll: #7169.

@kurtkuehnert kurtkuehnert added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Jan 11, 2023
@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR and removed C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Jan 11, 2023
@IceSentry
Copy link
Contributor

@kurtkuehnert I would suggest making a poll in a github discussion

@kurtkuehnert
Copy link
Contributor Author

Is that discoverable enough? I feel like I would not reach enough people with a separate GitHub discussion.
Maybe another discord poll?
Unfortunately, I can not create a poll right here in this issue, or can I?

@IceSentry
Copy link
Contributor

IceSentry commented Jan 11, 2023

Well, you could link it in this issue and post it in the #rendering-dev channel for visibility

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change X-Controversial There is active debate or serious implications around merging this PR
Projects
Status: Candidate
Development

No branches or pull requests

3 participants