Hello everyone! As a learning exercise I wrote Peter Shirley's Ray Tracing in One Weekend (as we all do) in Rust. I would like to get some constructive criticism regarding the use of best practices and parts of the code that could become more idiomatic (for example I am passing empty/default structs via mutable references to functions which is a very C++ thing to do). I did use cargo clippy to fix some low hanging fruit issues but I am sure that there is room for improvement. Performance is a close second although I haven't attempted to improve it yet. This is just part 1 and hopefully I will do parts 2 and 3 in the future. Thanks!
Indeed, this is not the most straightforward way to return values from functions. Most of the time you can just, well, return the values instead.
// hitable.rs
+fn hit(&self, r: &Ray, t_min: f64, t_max: f64) -> Option<HitRecord>;
-fn hit(&self, r: &Ray, t_min: f64, t_max: f64, rec: &mut HitRecord) -> bool;
// material.rs
+pub struct ScatteredRay {
+ pub ray: Ray,
+ pub attenuation: Color,
+}
fn scatter(
&self,
r_in: &Ray,
rec: &HitRecord,
- attenuation: &mut Color,
- scattered: &mut Ray,
+) -> Option<ScatteredRay>;
-) -> bool;
// main.rs
-let mut rec = HitRecord::default();
if depth == 0 {
return Color::new(0.0, 0.0, 0.0);
}
// If hit something
+if let Some(rec) = world.hit(r, 0.001, INFINITY) {
-if world.hit(r, 0.001, INFINITY, &mut rec) {
- let mut scattered = Ray::default();
- let mut attenuation = Color::default();
+ if let Some(ScatteredRay { ray, attenuation }) = rec
- if rec
.mat_ptr
+ .scatter(r, &rec)
- .scatter(r, &rec, &mut attenuation, &mut scattered)
{
+ return ray_color(&ray, world, depth - 1) * attenuation;
- return ray_color(&scattered, world, depth - 1) * attenuation;
} else {
return Color::new(0.0, 0.0, 0.0);
}
}
4 Likes
I've seen your new topic, but I'm not an expert at optimizing programs. But your codebase has changed a lot, so instead here's some more code quality suggestions.
lambertian.rs
solid_color.rs
xy_rectangle.rs
↓
material/lambertian.rs
texture/solid_color.rs
shape/xy_rectangle.rs
Your code has several traits and many trait implementations. Now, if any one struct was to implement many traits, it might as well be in its own module, but when you have one-to-many relationships like this, you should reflect it into the module (and file system) hierarchy.
fn random_scene() -> (HitableList, Camera, Color, f64)
When you're using the same tuple in many places, it's time to start using named structs. DRY isn't a panacea, but it's a useful heuristic for finding where things could be changed. Your scenes could also be moved into a separate file.
pub fn add<T: Hitable + 'static>(&mut self, hitable: T)
This interface does not compose well with your OBJ import feature. That function creates HitableList anew, and adding a HitableList to another results in an unnecessary layer of indirection. You could change the import function to modify an existing HitableList, or you could follow Vec and implement append/IntoIterator/Extend on HitableList. If you don't need an encapsulation layer minimizing HitableList's API surface, you could even do pub type HitableList = Vec<Arc<dyn Hitable>> and get all Vec methods for free.
fn obj_import_as_triangles(path: &str, material: Arc<dyn Material>) -> HitableList
The import function itself could be moved into a separate module, especially if you plan to support other formats.
AxisAlignedBoundingBox
This name is quite long. In Rust's name convention, unlike in English, acronyms are proper-cased, so you could consider Aabb.
t_min: f64, t_max: f64 / time: (f64, f64)
You should be using one of these consistently. The tuple is better than separate arguments, but in this particular case RangeInclusive<f64> is another possibility.
fn bounding_box(&self, time: (f64, f64)) -> Option<AxisAlignedBoundingBox>
This function ends up returning zero value when there is at least one object, but none of the objects have an AABB.
std::mpsc
This module is soft-deprecated. You don't strictly need to change this as it is fully usable, but crossbeam is widely regarded as better and faster.
No, that was fine. See below.
1 Like
This is not true any more. Rust 1.67 replaced the std::sync::mpsc implementation with crossbeam's, so it will have the same performance. The time to use crossbeam is if you need one of its features such as MPMC.
2 Likes
Thanks for correction. I had believed some API incompatibility made this change difficult, and a quick research could not reveal what had enabled the fix. This is getting off-topic, but is there anything I could read?
I don't recognize what you're referring to, but you might be able to find it in the discussion of PR #93563.
Found it. Had to take docs.rust-lang.org and replace stable to 1.28.0 to find the relevant link, though.
This topic was automatically closed 90 days after the last reply. We invite you to open a new topic if you have further questions or comments.