Avoiding "bad" patterns

68 points Danacus

I often find myself wanting to create collections of different structs that implement some interface or I end up using generics for something that is decided at runtime. For both those things I use a similar solution, but it feels like an antipattern.

Here's an example: let's say I have a 3D scene of objects like cubes and spheres. For each object I create a struct and implement the Intersect trait. Then I want to store all those objects in the scene. Usually you would use trait objects, but I don't really like them and often end up using the enum_dispatch crate.

Another example is when I created a trait to render the scene, but depending on some parameter I want to override some of the functions defined in that trait. So I would end up doing something like this:

impl Render for Renderer<SomeMode> {
    // Change some functionality
}

impl Render for Renderer<AnotherMode> {
    // Change some other functionality
}

But then I need to differentiate between those modes at runtime so I create an enum with all those different modes and use a match expression to figure out which generic parameter to use when creating a Renderer<T>. This is a similar "dispatcher" pattern used by enum_dispatch:

match mode {
    Mode::SomeMode => Renderer::<SomeMode>::new().do_something(),
    Mode::AnotherMode => Renderer::<AnotherMode>::new().do_something(),
}

This can get tedious quickly so I "hack" my way around it with some macro's.

I can't help but feel like I'm using rust wrong by enforcing object-oriented concepts like polymorphism. Are there any tips or guides on how to develop software in a more "rusty" way?

Edit: I'll provide a bit more context. I'm writing a ray tracer from scratch for a university course, so I can't share too much code.

I used the Intersect trait for intersection between a ray and an object. So I implement the trait for different geometric shapes and objects and add some code like this.

#[enum_dispatch]
pub trait Intersect {
    fn intersect(&self, ray: &Ray) -> IntersectResponse<IntersectResult>;
}

#[enum_dispatch(Intersect, Clone, Debug)]
#[derive(Clone, Debug)]
pub enum Object {
    Sphere,
    Cilinder,
    Cube,
    SimpleTriangleMesh,
    IndexedTriangleMesh,
    TransformedObject,
}

And then I store a Vec<Object> in my scene.

For the rendering I need to be able to create different types of "false color images". Some completely replace rendering code with something else, but some run the default rendering code, but draw another result. When I implemented this by adding a false_color_mode parameter to the render function of the scene I needed a lot of matchs and ifs which quickly became very messy. So I came up with this overly complicated monstrosity.

pub trait Render<I: Intersect, S: Shade, L: SampleLight>:
    ObjectsIter<I, S> + LightsIter<L> + IntersectScene
{
     // Default implementation here
}

impl<T, C, M, I, S> ObjectsIter<I, S> for Renderer<'_, C, M, T>
where
    C: Camera,
    M: FCM,
    I: Intersect,
    S: Shade,
    T: Scene + ObjectsIter<I, S>,
{
    fn objects_iter<'b>(
        &'b self,
    ) -> Box<dyn ExactSizeIterator<Item = &'b ShadedObject<I, S>> + 'b> {
        self.scene.objects_iter()
    }
    fn get_object(&self, id: usize) -> &ShadedObject<I, S> {
        self.scene.get_object(id)
    }
}

impl<T, C, M, L> LightsIter<L> for Renderer<'_, C, M, T>
where
    C: Camera,
    M: FCM,
    L: SampleLight,
    T: Scene + LightsIter<L>,
{
    fn lights_iter<'b>(&'b self) -> Box<dyn ExactSizeIterator<Item = &'b L> + 'b> {
        self.scene.lights_iter()
    }
    fn get_light(&self, id: usize) -> &L {
        self.scene.get_light(id)
    }
}

impl<T, C, M> IntersectScene for Renderer<'_, C, M, T>
where
    C: Camera,
    M: FCM,
    T: Scene + IntersectScene,
{
    fn intersect(&self, ray: &Ray) -> IntersectSceneResponse<IntersectResult> {
        self.scene.intersect(ray)
    }
}

impl<C, I, S, T, L> Render<I, S, L> for Renderer<'_, C, DefaultRender, T>
where
    C: Camera,
    I: Intersect,
    S: Shade,
    T: Scene + IntersectScene,
    L: SampleLight,
    Self: ObjectsIter<I, S> + LightsIter<L>,
{
}

impl<C, I, S, T, L> Render<I, S, L> for Renderer<'_, C, Normals, T>
where
    C: Camera,
    I: Intersect,
    S: Shade,
    T: Scene + IntersectScene,
    L: SampleLight,
    Self: ObjectsIter<I, S> + LightsIter<L>,
{
    fn shade_point<R: Rng>(
        &self,
        _options: &RenderOptions,
        _rng: &mut R,
        _ray_in: &Ray,
        _point: &Point,
        normal: &Vector,
        _brdf: &S,
    ) -> RenderResult {
        RenderResult {
            rgb: RGBSpectrum::new(
                f32::abs(normal.x() as f32),
                f32::abs(normal.y() as f32),
                f32::abs(normal.z() as f32),
            ),
            ..Default::default()
        }
    }
}

impl<C, I, S, T, L> Render<I, S, L> for Renderer<'_, C, Intersections, T>
where
    C: Camera,
    I: Intersect,
    S: Shade,
    T: Scene + IntersectScene,
    L: SampleLight,
    Self: ObjectsIter<I, S> + LightsIter<L>,
{
    fn apply_shading(&self, pixel: &mut Pixel, render_result: RenderResult) {
        pixel.add_assign_rgb_weighted(
            render_result.num_intersections as f32,
            render_result.num_intersections as f32,
            render_result.num_intersections as f32,
            0.,
        );
    }
}

I think the biggest overcomplication is that I tried to accommodate for different scene structs that implement Scene, IntersectScene, ObjectsIter and LightsIter, instead of just using one single Scene struct.

In order to use this code to render a scene I had to create an even bigger monster.

macro_rules! render {
    ($self: ident, $false_color_mode: ident, $options: ident, $camera: ident, $fb: ident, $channel: ident) => {{
        macro_rules! render_inner {
            ($fcm: ident) => {{
                let renderer: Renderer<'_, C, $fcm, _> = Renderer::new($self);
                $fb.for_each_pixel(|x, y, p| {
                    let mut opts_clone = $options.clone();
                    let mut rng =
                        SmallRng::seed_from_u64($options.seed + 10000 * x as u64 + y as u64);
                    renderer.render_pixel(&options, &mut rng, $camera, x, y, p);
                    $channel.notify_progress();
                });
            }};
        }
        match $false_color_mode {
            FalseColorMode::Normals => render_inner!(Normals),
            FalseColorMode::Points => render_inner!(Points),
            FalseColorMode::LightDotProduct => render_inner!(LightDotProduct),
            FalseColorMode::CameraDistance => render_inner!(CameraDistance),
            FalseColorMode::Intersections => render_inner!(Intersections),
            FalseColorMode::IntersectionsTrue => render_inner!(IntersectionsTrue),
            FalseColorMode::Sampling => render_inner!(Sampling),
            FalseColorMode::None => render_inner!(DefaultRender),
        }
    }};
}

pub trait Scene {
    fn render_frame<C: Camera + Send + Sync>(
        &self,
        false_color_mode: FalseColorMode,
        options: RenderOptions,
        camera: &C,
        framebuffer: &mut FrameBuffer,
        progress_channel: ProgressChannel,
    );
}

#[derive(Clone)]
pub struct BasicScene<I: Intersect, S: Shade, L: SampleLight> {
    pub(crate) shapes: Vec<ShadedObject<I, S>>,
    pub(crate) lights: Vec<L>,
}

impl<I, S, L> Scene for BasicScene<I, S, L>
where
    I: Intersect + Send + Sync,
    S: Shade + Send + Sync,
    L: SampleLight + Send + Sync,
{
    fn render_frame<C: Camera + Send + Sync>(
        &self,
        false_color_mode: FalseColorMode,
        options: RenderOptions,
        camera: &C,
        framebuffer: &mut FrameBuffer,
        progress_channel: ProgressChannel,
    ) {
        render!(
            self,
            false_color_mode,
            options,
            camera,
            framebuffer,
            progress_channel
        );
    }
}

So at this point I started to realise that I'm doing something wrong.