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

Rename Render* to Gpu* #6968

Closed

Conversation

kurtkuehnert
Copy link
Contributor

@kurtkuehnert kurtkuehnert commented Dec 16, 2022

Objective

Solution

  • This PR only focuses on renaming the type and variable names.

  • 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}

  • maybe slightly controversial: I have additionally renamed a few occurrences of the variable name device and queue, which refer to a GPUDevice or GPUQueue, to gpu_device and gpu_queue for consistency's sake.

  • additionally, I have type aliased the CommandEncoder to GpuCommandEncoder


Changelog

Changed

  • Renamed the types Render{Device, Queue, Adapter, AdapterInfo, Instance, Context} to GPU{Device, Queue, Adapter, AdapterInfo, Instance, Context}

  • Renamed CommandEncoder to GpuCommandEncoder

  • The fields of the RenderContext have been renamed like so:

// old
pub struct RenderContext {
    pub device: RenderDevice,
    pub command_encoder: CommandEncoder,
}

// new
pub struct GpuContext {
    pub gpu_device: GpuDevice,
    pub gpu_command_encoder: GpuCommandEncoder,
}

Migration Guide

Rename all occurrences of the types Render{Device, Queue, Adapter, AdapterInfo, Instance, Context} to GPU{Device, Queue, Adapter, AdapterInfo, Instance, Context}.

The fields of the RenderContext have been renamed like so:

// old
pub struct RenderContext {
    pub device: RenderDevice,
    pub command_encoder: CommandEncoder,
}

// new
pub struct GpuContext {
    pub gpu_device: GpuDevice,
    pub gpu_command_encoder: GpuCommandEncoder,
}

@kurtkuehnert
Copy link
Contributor Author

This is quite an invasive change inside the render module. Most subsequent PRs (to this module) will necessitate a rebase.
The variable renames should not break any code outside the engine (except the GPUContext.gpu_device rename), right?

I have split up the second commit, which might be a bit controversial. It makes the variable naming a bit more consistent.

Now that the GPU types are renamed we might want to put them into a separate module or even split the GPU initialization code into its own crate. Currently, the general GPU setup and the rendering code (render graph, render commands, render app, etc.) are pretty intertwined. I do not know if we would like to build a separate GPU compute abstraction that is not part of the render graph or whether we will merge both concepts.

We should also consider renaming further types and modules like the render_resource module for example.
However, these should be part of a follow-up PR.

Let me know what you think about these ideas.

@mockersf mockersf added A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Dec 16, 2022
@james7132
Copy link
Member

james7132 commented Dec 16, 2022

Going to raise an objection to this rename. It's not guaranteed that the hardware doing the rendering is an actual GPU. It could easily just be a software renderer or a null device (in the case of headless environments). As mentioned above, it also adds a lot of churn to almost every rendering API, including many user-facing ones as well. Given these two reasons, I don't think the rename is well justified, as it adds a lot of busywork for contributors and users without gaining us that much clarity.

@tim-blackbird
Copy link
Contributor

I'm seconding James on the points of churn and clarity.

It seems that wgpu also avoided the gpu prefix for the same reason despite the naming scheme in the WebGPU spec (e.g. Device and Adapter).

@james7132 james7132 added the X-Controversial There is active debate or serious implications around merging this PR label Dec 16, 2022
@kurtkuehnert
Copy link
Contributor Author

I do understand your point.
Although, I personally do not think that there is any problem with classifying a software rendering backend to wgpu as a GPUAdapter, etc.

As I see it we have got three options:

  1. stick with the current Render prefix
  • no change
  • these types are not used for rendering alone, but also for GPUCompute
  • until now we do not offer any high-level compute abstraction but in the future, these names will be very confusing
  1. go through with this PR and adopt the GPU prefix
  • all users of these types will have to search and replace a couple of types and maybe some variable names
  • we run into the issue with the GPU - null device - software renderer
  1. we could also just use the same naming scheme as wgpu does and rename the wgpu types (e.g. WgpuAdapter)
  • this could cause some confusion, but since we are already abstracting a way most of the raw wgpu interface this could also be good solution
  • however, this would also result in a rename of types and variables, that will bubble up to our users

I would prefer solution 2 or 3. with 3 being slightly more elegant in my opinion. What do you think?

@kurtkuehnert
Copy link
Contributor Author

Also the further we delay this decision the more user code we break. I should have probably redone this PR last year then we would not have as many users depending on these features.

@cart
Copy link
Member

cart commented Dec 20, 2022

I still consider churn to be acceptable for most Bevy changes at this point. If we agree something is "the right call" then we should do it, even if it breaks things (especially for things like renames which are straightforward migrations).

So I think the "clarity" point is what we should discuss.

Some arguments in favor of the change:

  1. WGPU is GPU api. Anyone building a "real" CPU renderer wouldn't go through a wgpu-like interface. The design was entirely informed by the constraints of gpus and they use terminology rooted in GPU history (ex: shader, bind group, etc).
  2. RenderDevice is "too specific". Not all gpu-compute operations will relate to rendering (ex: users might do simulations or other app logic on the gpu).

My prediction for the reason WGPU avoided the GPU prefix is that it is redundant / implied within the context of WGPU. Within the context of Bevy, GPU cannot be implied context for a Device, as we deal with many devices in many contexts (input devices, display devices, etc). Same goes for Adapter (ex: network adapter).

If we do roll with "Gpu" terminology, we might even want to forgo the "Device" term and just call it Gpu. Thats slightly controversial because it strips out a wgpu term entirely. But GpuDevice feels a bit redundant to me, as GPU already implies device.

@kurtkuehnert
Copy link
Contributor Author

I totally agree with 1 and 2.
Now that I have thought more about it, we should not adopt the wgpu naming convention, due to the reasons that you have mentioned.

But I do not agree that we rename the Device to Gpu.
WebGPU calls the wgpu Instance GPU. If we were to rename the Device to Gpu that would cause a lot of confusion.
Here is a nice comparison I have found between the terms used by all the different graphics APIs https://alain.xyz/blog/comparison-of-modern-graphics-apis.

I am still in favor of this rename, but I will change the prefix to Gpu instead of GPU to match the rust convention.

Maybe we should do a poll in the bevy render-dev channel?

@kurtkuehnert kurtkuehnert force-pushed the render_to_gpu_rename branch 2 times, most recently from dd55779 to 1faa836 Compare December 21, 2022 10:28
kurtkuehnert added a commit to kurtkuehnert/bevy that referenced this pull request Dec 27, 2022
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall I like it, especially based on cart's comment and the discord poll. There's a few instances of GPU vs Gpu that should be fixed, but other than that and assuming the merge conflicts are fixed LGTM

I really like that this opens up the opportunity to split bevy_render a little.

@kurtkuehnert
Copy link
Contributor Author

It seems that most people are in favor of this change.
Ideally, this PR will be followed by the #7000 (or multiple smaller PRs) bevy_gpu/bevy_render split.

I still would like to hear your opinions on some decisions related to the entire refactor.

  1. The Render/GpuContext is only used during the RenderGraph. I think it should be moved there.
    Here I am wondering whether the Render prefix is the better name.

  2. Currently we use multiple different Prefixes for GPU-related stuff: Render, Gpu and Wgpu.
    This PR changes Render to Gpu. I believe that we should also rename the Wgpu* types (like WgpuSettings, WgpuLimits, etc.) to Gpu*. I can do this here or as part of a separate PR.

  3. With the type renames complete, the next step would be to rename the render_resource module to gpu_resource.

  4. The refactor of Separate bevy_gpu from bevy_render #7000 went surprisingly smoothly, except for three minor details.
    4.1 The PipelineCache::extract_shaders method depends on the Extract Param of the render module. Thus I had to move it out of bevy_gpu. I am still unsure where to put this code though.
    4.2 The AsBindGroup trait is a render abstraction and thus had to be moved out of bevy_gpu. Same as above where should I put this?
    4.3 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.

  5. To make reviewing easier I could try to separate the refactor (file renames, type moves, etc.) into multiple smaller PRs before I transfer everything into the bevy_gpu crate. This will create a lot more changes though (especially the use statements will change multiple times). Alternatively, the split can be performed as one large PR (eg. Separate bevy_gpu from bevy_render #7000).

  6. Currently, there is still an open TODO in the render_resource module stating:

// TODO: decide where re-exports should go

We can keep them where they are (gpu_resource), or elevate them into the root bevy_gpu module.
Whatever we choose we should probably do the same for the GPU resources to make importing easier.

use bevy::gpu::Buffer;
vs
use bevy::gpu::gpu_resource::Buffer;
  1. I think we should use the opportunity to hide all wgpu specifics inside the bevy_gpu crate, and only reexport the stuff we have to. bevy_render should no longer depend on both wgpu and encase.

@cart
Copy link
Member

cart commented Jan 4, 2023

But I do not agree that we rename the Device to Gpu.

Yeah this argument makes sense!

The Render/GpuContext is only used during the RenderGraph. I think it should be moved there. Here I am wondering whether the Render prefix is the better name.

Yeah I think calling it RenderContext makes sense

Currently we use multiple different Prefixes for GPU-related stuff: Render, Gpu and Wgpu.
This PR changes Render to Gpu. I believe that we should also rename the Wgpu* types (like WgpuSettings, WgpuLimits, etc.) to Gpu*. I can do this here or as part of a separate PR.

I agree with these renames. Feel free to do them here.

With the type renames complete, the next step would be to rename the render_resource module to gpu_resource.

Agreed!

4.1 The PipelineCache::extract_shaders method depends on the Extract Param of the render module. Thus I had to move it out of bevy_gpu. I am still unsure where to put this code though.

Sounds like bevy_render is the place for it, but we should probably have this conversation in #7000

4.2 The AsBindGroup trait is a render abstraction and thus had to be moved out of bevy_gpu. Same as above where should I put this?

Sounds like bevy_render is the place for it, but we should probably have this conversation in #7000

4.3 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.

This makes sense to me.

// TODO: decide where re-exports should go

I love how use bevy::gpu::Buffer reads. I'm on board for that.

I think we should use the opportunity to hide all wgpu specifics inside the bevy_gpu crate, and only reexport the stuff we have to. bevy_render should no longer depend on both wgpu and encase.

This does seem like it is worth trying, but its hard to say what the actual implications of this are without actually trying it. Happy to consider it, but I can't have a strong opinion until I get my hands dirty / see how it unfolds. Lets wrap this PR up first then move on to #7000 for these questions.

…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}

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`

type aliased GpuCommandEncoder
renamed GpuContext::command_encoder to GpuContext::gpu_command_encoder
@kurtkuehnert
Copy link
Contributor Author

kurtkuehnert commented Jan 5, 2023

Thanks for the feedback. 😄

I have renamed the remaining Wgpu* types to Gpu* and reverted back to RenderContext (and moved it to the render_graph module).

I am still not sure where we should draw the line between types prefixed with Gpu and types without a prefix. In the WebGpu spec every type is prefixed with Gpu and wgpu forgoes them completely.

Thinking more about the refactor as a whole, I came up with another proposal.
If we are sure to follow through with #7000 (or a similar pr), then would the following be an option:

Given you are onboard with the root level type export (use bevy::gpu::Buffer), we could simply do away with all prefixes just like wgpu does. We could export all GPU-related types at the root of the bevy_gpu crate. Additionally, we can add a new prelude that exports common types (e.g. Buffer, RenderPipeline, Device, etc.). This eliminates the need for imports like this:

old
bevy::render::{
  render_resources::*,
  RenderDevice
}

new
bevy::gpu::prelude::*;

And because the types are nicely scoped in the bevy_gpu crate, the context of Device, Queue, Buffer, etc. would be clear. We could even write gpu::Buffer or gpu::Device where desired.

I think this is the most elegant solution by far. Let me know what you think.

I have made a summary of the possible solutions here: #7157

@kurtkuehnert
Copy link
Contributor Author

Closed in favor of #7109.

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-Usability A targeted quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Controversial There is active debate or serious implications around merging this PR
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants