Overall, this is quite clean code. Most of what I have to say is tips and tricks, and minor code style things.
It could use more comments; for example, a comment on every Rust struct that matches a WGSL struct mentioning that both places need to be modified if one is.
const VERTICES: [Vertex; 6] = [
- You can skip constructing this vertex buffer and instead calculate the vertices in your vertex shader, from the vertex ID. This is not a big deal either way, but it takes less code since you don't have to create, fill, and bind the buffer.
- When you want a full-screen render pass, it's better to use one large (clipped) triangle instead of two triangles. This is because the GPU needs to do extra work to process the vicinity of the diagonal line where the two triangles meet; a single triangle that just gets clipped to the screen rectangle is cheaper.
pub struct Application<'a> {
window: &'a Window,
surface: Surface<'a>,
You can avoid this lifetime relationship by putting the Window in an Arc and passing a clone of that Arc to create_surface(). This way, the Application struct isn't bound to a borrow from a stack frame, making it instead:
pub struct Application {
window: Arc<Window>,
surface: Surface<'static>,
This doesn't matter for your particular application, but in general, it's useful to avoid lifetimes on long-lived structs. For example, if you wanted to have multiple windows (documents) that the user can create and delete, you would need to do this, because the &'a Window pattern would mean that windows are hard to create and impossible to delete.
use wgpu::{util::{BufferInitDescriptor, DeviceExt}, BindGroup, BindGroupDescriptor, BindGroupEntry, BindGroupLayoutDescriptor, BindGroupLayoutEntry, BindingResource, BindingType, Buffer, …
This is a matter of code style, not objective improvement, but I personally recommend that when you have such a large list of used items, you should not import them individually but qualify the usage sites instead:
pub struct Application<'a> {
...
device: wgpu::Device,
queue: wgpu::Queue,
...
This may be somewhat more tokens in total, but it has several advantages:
- Readers of the code do not need to scroll up to see where a type is from.
- If you are using two libraries with the same one-word type name, say
Device, Queue, Error, etc., they do not conflict.
- You never have to maintain the imports:
- No merge conflicts.
- No unused imports to delete when you delete code.
- Moving code to a different file doesn't require copying and pruning the
use.
let instance = Instance::new(InstanceDescriptor::default());
let surface = instance.create_surface(window)?;
let adapter = instance.request_adapter(
&RequestAdapterOptions {
Consider using the functions from wgpu::util, backend_bits_from_env(), initialize_adapter_from_env_or_default(), and power_preference_from_env(). This gives your application more configurability that may be useful for troubleshooting or working around adapter bugs.
let texture_format = surface_caps.formats
.into_iter()
.find(|format| format.eq(&TextureFormat::Rgba8Unorm))
.context("preferred texture format error")?;
- This is a roundabout way of checking if
formats contains Rgba8Unorm — the variable texture_format will always contain Rgba8Unorm if it doesn't fail. This is a good basis for supporting multiple formats, but you're not actually doing that.
- Code style: Don't call
PartialEq::eq directly; use the == operator.
let light = Light {
position: [-100.0, -250.0, -60.0],
intensity: 1.0
};
let light_buffer = device.create_buffer_init(&BufferInitDescriptor {
label: Some("Spheres buffer"),
- It's inconsistent that the scene data for the light is embedded here, but the scene data for the spheres is above at the top level.
- The
label for the light_buffer is incorrect.
resource: BindingResource::Buffer(BufferBinding {
buffer: &camera_uniform_buffer,
offset: 0,
size: None
})
You can replace this repetitive code with Buffer::as_entire_binding().
pub fn resize(&mut self, new_size: &PhysicalSize<u32>) {
-
PhysicalSize is a small value type, so passing it by reference is unnecessary.
-
This method overwrites all of the fields of self in a way which is identical to calling CameraUniform::new() again, so it doesn't need to exist at all — just call CameraUniform::new() and assign the result. (It would make sense to change new() to accept a PhysicalSize<u32>, since you have that available everywhere.) As a general principle: avoid writing two functions that compute the same formula if you don't have to, because someday you'll update one and forget the other.
light.rs and sphere.rs are so small that there isn't much value in them existing. I would suggest creating instead a scene.rs that contains both types, since they are both parts of the scene.
let oc = ray.position - sphere.center;
let a = dot(ray.direction, ray.direction);
let b = 2.0 * dot(oc, ray.direction);
let c = dot(oc, oc) - sphere.radius * sphere.radius;
Give these variables more descriptive names, and a comment or five explaining what quantities this code is actually calculating. (Mathematics may use short names, but math papers also come with explanatory text, not just formulas. Code needs to be easily comprehended so that it is maintainable.)
.
