-
-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
Rename Render* to Gpu* #6968
Conversation
This is quite an invasive change inside the render module. Most subsequent PRs (to this module) will necessitate a rebase. 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 Let me know what you think about these ideas. |
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. |
I do understand your point. As I see it we have got three options:
I would prefer solution 2 or 3. with 3 being slightly more elegant in my opinion. What do you think? |
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. |
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:
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 |
I totally agree with 1 and 2. But I do not agree that we rename the I am still in favor of this rename, but I will change the prefix to Maybe we should do a poll in the bevy render-dev channel? |
dd55779
to
1faa836
Compare
1faa836
to
3666712
Compare
There was a problem hiding this 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.
It seems that most people are in favor of this change. I still would like to hear your opinions on some decisions related to the entire refactor.
We can keep them where they are ( use bevy::gpu::Buffer;
vs
use bevy::gpu::gpu_resource::Buffer;
|
Yeah this argument makes sense!
Yeah I think calling it
I agree with these renames. Feel free to do them here.
Agreed!
Sounds like
Sounds like
This makes sense to me.
I love how
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
renamed Wgpu* types to Gpu*
b4765e0
to
e2bfa4a
Compare
Thanks for the feedback. 😄 I have renamed the remaining I am still not sure where we should draw the line between types prefixed with Thinking more about the refactor as a whole, I came up with another proposal. Given you are onboard with the root level type export ( old
bevy::render::{
render_resources::*,
RenderDevice
}
new
bevy::gpu::prelude::*; And because the types are nicely scoped in the 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 |
Closed in favor of #7109. |
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
andqueue
, which refer to a GPUDevice or GPUQueue, togpu_device
andgpu_queue
for consistency's sake.additionally, I have type aliased the
CommandEncoder
toGpuCommandEncoder
Changelog
Changed
Renamed the types Render{Device, Queue, Adapter, AdapterInfo, Instance, Context} to GPU{Device, Queue, Adapter, AdapterInfo, Instance, Context}
Renamed
CommandEncoder
toGpuCommandEncoder
The fields of the
RenderContext
have been renamed like so: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: