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

Separate bevy_gpu from bevy_render #7000

Closed
wants to merge 6 commits into from
Closed

Separate bevy_gpu from bevy_render #7000

wants to merge 6 commits into from

Conversation

kurtkuehnert
Copy link
Contributor

@kurtkuehnert kurtkuehnert commented Dec 21, 2022 •
edited
Loading

Objective

  • This PR is based on Rename Render* to Gpu* #6968.
  • I wanted to see whether it was possible/useful to split the general GPU code from the render crate.
  • bevy_render is a very large crate and there have been discussions about whether to split it into multiple smaller crates
  • This is the first attempt at that.

Rational

  • This refactor should make building a separate GPU compute infrastructure easier.
  • It makes the distinction between CPU and GPU resources clear (IMO).
  • This split should be useful to people that want to build their own renderer (based on wgpu).
  • IMO this is an important step in making the rendering module easier to understand. Especially, people new to bevy struggle a lot with comprehending the rendering code. Splitting up the GPU abstraction from our rendering logic will make this a little easier.

Solution

  • The entire wgpu/naga/encase backend is now located in the bevy_gpu crate.
  • The bevy_render crate does not depend on these libraries directly anymore.
  • This change increases the compile time on my machine by about 2%.

Details

This separation was actually quite painless (except for the hundreds of renames in comments and use statements).

A couple of small tweaks had to be made to split the two crates properly.

  • The PipelineCache::extract_shaders method depends on the Extract Param of the render module. Thus I moved it out of bevy_gpu. I am still unsure where to put this code. For now, it lives in bevy_render::lib.

  • The AsBindGroup trait is a render abstraction and thus had to be moved out of bevy_gpu.

  • The SpecializedMeshPipeline was moved out of the bevy_gpu::gpu_resource module into the bevy_render::mesh module. This is more in line with what we do for other specialized pipelines as well.

  • Finally, I removed two or three references in the documentation of bevy_gpu, which linked back into bevy_render.
    IMO these were not necessary and they keep the dependency tree clean.

How to Merge

This will be a large breaking change. If we want to go through with this PR it would probably be best to do it soon and not wait for another year.

If we decide that this is something we want then I will need help configuring the cargo features and dependencies, etc correctly.

We could merge this PR on top of, or after #6968. I would prefer the former to keep the rebasing to a minimum.

If there is anything about the new API that you want to see renamed/moved, please let me know!

Changelog

  • Todo (this PR breaks quite a few things)

Migration Guide

  • Todo

@kurtkuehnert kurtkuehnert marked this pull request as ready for review December 22, 2022 11:08
@james7132 james7132 added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Dec 23, 2022
@james7132
Copy link
Member

As mentioned in the original description, this is a large breaking change. It's prone to merge conflicts, and difficult to review. Is it possible to break this down into smaller parts and piecewise migrate everything to the new crate? Marking this down as complex due to the scale of the refactor.

@james7132 james7132 added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Dec 23, 2022
@kurtkuehnert
Copy link
Contributor Author

I could split this PR into two parts, one for the move of Gpu* types into bevy_gpu::mod and a second one, which migrates all the render/gpu-resources. This would mean that I would probably have to start over. Moving types between crates is quite painful and time intensive. As far as I am aware, no refactoring tool can do this automatically. I had to find and replace the names in dozens examples, use statements, and docs. I still believe that all this trouble is worth it in the end.

If the reviews agree that the split into two parts makes their lives easier, then I will take the time and redo this PR. Otherwise, I would be glad to only rebase this one a couple of times until it is ready to be merged.

@kurtkuehnert
Copy link
Contributor Author

I am also unsure whether it would be easier to just review and merge this PR in its entirety instead of performing the rename (of the parent PR) first. 6 would have multiple breaking changes in a row affecting the same types, which would be very annoying.

@kurtkuehnert
renamed type names Render{Device, Queue, Adapter, AdapterInfo, Instan…
753da19 
…ce, Context} to GPU{Device, Queue, Adapter, AdapterInfo, Instance, Context}

renamed variable names render_{device, queue, adapter, adapter_info, instance, context} to gpu_{device, queue, adapter, adapter_info, instance, context}
@kurtkuehnert
renamed occurrences of the variable name device that refer to a `GP…
3d0967a 
…UDevice` to `gpu_device`

renamed occurrences of the variable name `queue` that refer to a `GPUQueue` to `gpu_queue`
@kurtkuehnert
renamed GPU* to Gpu*
1cc0598
@kurtkuehnert
rebased onto main
3666712 
fixed GPU* in docs
type aliased GpuCommandEncoder
renamed GpuContext::command_encoder to GpuContext::gpu_command_encoder
@kurtkuehnert
added GpuPlugin
d8b5527 
extracted the gpu code into a separate crate

renamed GPU* to Gpu*

renamed occurrences of the variable name `device` that refer to a `GPUDevice` to `gpu_device`
renamed occurrences of the variable name `queue` that refer to a `GPUQueue` to `gpu_queue`

renamed type names Render{Device, Queue, Adapter, AdapterInfo, Instance, Context} to GPU{Device, Queue, Adapter, AdapterInfo, Instance, Context}
renamed variable names render_{device, queue, adapter, adapter_info, instance, context} to gpu_{device, queue, adapter, adapter_info, instance, context}
@kurtkuehnert
rebased onto bevyengine#6968
a650d23
@IceSentry
Copy link
Contributor

Yeah, there's a lot of noise right now because of the rename, but once that PR gets merged this one should be easy enough to review IMO

This was referenced Dec 31, 2022
Closed
Closed
Closed
@alice-i-cecile alice-i-cecile added X-Controversial There is active debate or serious implications around merging this PR and removed D-Complex Quite challenging from either a design or technical perspective. Ask for help! labels Jan 6, 2023
@kurtkuehnert kurtkuehnert mentioned this pull request Jan 11, 2023
Open
@kurtkuehnert
Copy link
Contributor Author

Closed in favor of #7111

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
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-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kurtkuehnert @james7132 @IceSentry @alice-i-cecile