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

Updated modified math Rust code version no longer creates correct images. #23

Open
jzakiya opened this issue Apr 30, 2021 · 11 comments
Open

Comments

@jzakiya
Copy link
Contributor

jzakiya commented Apr 30, 2021

The original (corrected) parallel Rust version created correct images when run either sequential|parallel.

The modified version, which changed the Vector math implementation, no longer creates correct images.

@darleybarreto
Copy link
Contributor

That happened after I added the ops stuff?

@jzakiya
Copy link
Contributor Author

jzakiya commented Apr 30, 2021

Yes, the previous vector math version was working fine.
Please check the image after every modification to ensure the image is still correct.

@darleybarreto
Copy link
Contributor

Is this the expected result?
image

@jzakiya
Copy link
Contributor Author

jzakiya commented Apr 30, 2021

Yes. All the different versions should produce an image that looks like that.

@darleybarreto
Copy link
Contributor

Fixed by #24

@darleybarreto
Copy link
Contributor

I'm deeply sorry about that. I only noticed this when I checked the Python version (which is the image above). Before, I assumed that the wrong one was the intended.

@jzakiya
Copy link
Contributor Author

jzakiya commented May 3, 2021

I saw the posted corrected ops version yesterday (Sunday, May 2, 2021) and confirm it now produces the correct image for both non|parallel modes of operation.

(Need to include in run.bat file .\target\release\ray_tracer.exe parallel to run in parallel mode.)

I also confirm it's faster for both modes on my Linux OS, System76 laptop: 69|15 ms.
System: i7-6700HQ (2016); 3.5 GHz, 4C|8T.

However, I want to raise some issues for consideration, that apply not just to this Rust implementation, but apply to writing code in general.

I know Rust code has a "standard" format, and a tool to turn code into it, but like most of these things, these are arbitrary determinations of style. But writing good code, i.e. code that works, is clear and understandable, and concise, I find at times is defeated by presenting code in some "standard" format. After all, writing good code, like any kind of "good" writing,
is just as much a work of art and expression than a mere exercise in syntax formatting.

So I want to raise these issues for consideration and reflection.

One thing I dislike about Rust "standard" formatting is that it wastes so much space using vertical allocation of structural elements that are best understood if written horizontally. It also waste space allocating single lines to end braces }. Python|Nim, et al, of course do away with having to use }|end statements, because they add nothing to the operation of the code and are only there for code parsing purposes.

Thus, there are clearly times when writing coding processes as one line horizontal expressions is better for showing what the process the code is doing than separating it over multiple lines. There are also many cases where multiple line strings of closing }s can be condensed to one line.

Using the Rust implementation, here is an example where I think "standard" Rust formatting lends nothing to code comprehension or clarity. (I know others would vehemently disagree with this). :-)

struct Scene {
    things: Vec<Thing>,
    lights: Vec<Light>,
    camera: Camera,
}

impl Scene {
    fn new() -> Scene {
        Scene {
            things: vec![
                Thing::new_plane(
                    Vector::new(0.0, 1.0, 0.0),
                    0.0,
                    Surface::CheckerboardSurface,
                ),
                Thing::new_sphere(Vector::new(0.0, 1.0, -0.25), 1.0, Surface::ShinySurface),
                Thing::new_sphere(Vector::new(-1.0, 0.5, 1.5), 0.5, Surface::ShinySurface),
            ],
            lights: vec![
                Light {
                    pos: Vector::new(-2.0, 2.5, 0.0),
                    color: Color::new(0.49, 0.07, 0.07),
                },
                Light {
                    pos: Vector::new(1.5, 2.5, 1.5),
                    color: Color::new(0.07, 0.07, 0.49),
                },
                Light {
                    pos: Vector::new(1.5, 2.5, -1.5),
                    color: Color::new(0.07, 0.49, 0.071),
                },
                Light {
                    pos: Vector::new(0.0, 3.5, 0.0),
                    color: Color::new(0.21, 0.21, 0.35),
                },
            ],
            camera: Camera::new(Vector::new(3.0, 2.0, 4.0), Vector::new(-1.0, 0.5, 0.0)),
        }
    }
}

First, a Scene has 3 elements: things, lights, camera.
Here, separating them to 3 lines reduces the cognitive conceptual flow of seeing them as 1 concept. Then within the method, the same reduction in conceptual flow is also created. I think a general reader will find the code below much more conceptually clear and understandable.

It's obvious, a Scene has a vector of things with 3 elements, and lights with 4 elements, and a camera angle. These visually clearly correspond to the struct Scene layout, 1:1.

struct Scene { things: Vec<Thing>, lights: Vec<Light>, camera: Camera }

impl Scene {
    fn new() -> Scene {
        Scene {
            things: vec![
                Thing::new_plane(Vector::new(0.0, 1.0, 0.0), 0.0, Surface::CheckerboardSurface),
                Thing::new_sphere(Vector::new(0.0, 1.0, -0.25), 1.0, Surface::ShinySurface),
                Thing::new_sphere(Vector::new(-1.0, 0.5, 1.5), 0.5, Surface::ShinySurface),
            ],
            lights: vec![
                Light { pos: Vector::new(-2.0, 2.5, 0.0), color: Color::new(0.49, 0.07, 0.07) },
                Light { pos: Vector::new(1.5, 2.5, 1.5),  color: Color::new(0.07, 0.07, 0.49) },
                Light { pos: Vector::new(1.5, 2.5, -1.5), color: Color::new(0.07, 0.49, 0.071)},
                Light { pos: Vector::new(0.0, 3.5, 0.0),  color: Color::new(0.21, 0.21, 0.35) },
            ],
            camera: Camera::new(Vector::new(3.0, 2.0, 4.0), Vector::new(-1.0, 0.5, 0.0)),
    }   }
}

Here's anothrer example where verboseness decreases comprehension and clarity.

struct SurfaceProperties {
    diffuse: Color,
    specular: Color,
    reflect: f32,
    roughness: f32,
}

impl Surface {
    fn get_properties(&self, pos: Vector) -> SurfaceProperties {
        match self {
            Surface::CheckerboardSurface => {
                let (diffuse, reflect) = if (pos.z.floor() + pos.x.floor()) as i32 % 2 != 0 {
                    (COLOR_WHITE, 0.1)
                } else {
                    (COLOR_BLACK, 0.7)
                };

                SurfaceProperties {
                    diffuse,
                    specular: COLOR_WHITE,
                    reflect,
                    roughness: 150.0,
                }
            }
            Surface::ShinySurface => SurfaceProperties {
                diffuse: COLOR_WHITE,
                specular: COLOR_GREY,
                reflect: 0.7,
                roughness: 250.0,
            },
        }
    }
}

First a performance tweak.

All this is doing: (pos.z.floor() + pos.x.floor()) as i32 % 2 != 0
is determining if the computed result is an odd or even integer.
Unfortuantely Rust doesn't have odd?|even? methods like Ruby|Crystal|et al.
To do this, just check if the integer lsb is a 0|1, you don't need to do % 2.
This is faster: (pos.z.floor() + pos.x.floor()) as i32 & 1 != 0
This would be clearer|better though: ((pos.z.floor() + pos.x.floor()) as i32).odd?

Also, it's clearer to separate the conditional computation from its use, e.g:
let odd_test = (pos.z.floor() + pos.x.floor()) as i32 & 1 != 0

then you can just write:
let (diffuse, reflect) = if odd_test { (COLOR_WHITE, 0.1) } else { (COLOR_BLACK, 0.7) };

From a reader's perspective, it's cognitively much clearer and concise on what the process is doing. And from the compiler's perspective, it's all going to be optimized anyway.

Thus for all the reasons stated, the following code is not just shorter, but more importantly, easier to read, follow, and understand for any general reader, reducing 33 loc to now 15 loc.

struct SurfaceProperties { diffuse: Color, specular: Color, reflect: f32, roughness: f32 }

impl Surface {
    fn get_properties(&self, pos: Vector) -> SurfaceProperties {
        match self {
            Surface::CheckerboardSurface => {
                let odd_test = (pos.z.floor() + pos.x.floor()) as i32 & 1 != 0
                let (diffuse, reflect) = if odd_test { (COLOR_WHITE, 0.1) } else { (COLOR_BLACK, 0.7) };

                SurfaceProperties { diffuse, specular: COLOR_WHITE, reflect, roughness: 150.0 }
            }
            Surface::ShinySurface => 
                SurfaceProperties { diffuse: COLOR_WHITE, specular: COLOR_GREY, reflect: 0.7, roughness: 250.0 }
    }   }
}

These are not just issues of style and personal preferences.

It has been scientifically established, the shorter and more concise you can present ideas to people (their brains) the more likely they are to take time to consume the ideas (read, listen, view) and then understand them too.

I've seen in the Rust forums similar questions and issues raised on presenting source code more concisely, and getting rid of syntax code noise that produces a large percentage of unnecessary locs.

Thus, shorter and clearer code will thoroughly be read|understood (and then used) more than longer and less clear code.

Of course, people have written books on these topics. But I just wanted to mention them here, since you significantly changed the format of the prior Rust examples, and I just took your code and reformatted into something I like better, as shown.

@jzakiya
Copy link
Contributor Author

jzakiya commented May 10, 2021

On Thursday, May 6, 2021 Rust 1.5.0 was released (with bug fix release 1.5.1 today, Monday, May 10, 2021).

I noticed compiling with just cargo build --release the times on my system, as reported previously, were 69|15 ms.

Yesterday I noticed the Cargo.toml changed, and a config.toml was added, and compiled again and got: 57|14 ms.

But then I also compiled as below (used for other projects) and got (for avg of 100 iterations): 55|12 ms.:
RUSTFLAGS="-C opt-level=3 -C debuginfo=0 -C target-cpu=native" cargo build --release

So it seems performance is really dependent on compilation options|manner.

FYI, on my system, the fastest Rust compilation make it faster than C|C++, which were the fastest of those I am able to compile (excluding the fast Nim version, which is faster, but doesn't create the exact image as the rest, with both spheres slightly smaller).

This begs the question, should you compare times of different versions based on a "standard" release for each, or based on an "optimum" release for each? Or show differences of the various compilation options for readers to be aware of?

@darleybarreto
Copy link
Contributor

AFAIK opt-level=3 is on by default on release and target-cpu=native basically means non portable code, similar to what a JIT does (e.g. Julia), I have to say I don't know how debuginfo=0 would impact here. But sure, I think people should be aware of the flags like saying somewhere: "Hey, there's might be a flag or option to improve this, we will do our best efforts to have them eventually, if you want, you can PR with those."

@jzakiya
Copy link
Contributor Author

jzakiya commented May 11, 2021

Rust seems to be very "sensitive" to compilation options|manner because for many|most programs many crates are being used, and their implementations affect ultimate total performance. I think this is the major factor here that affects performance. Compiling with 1.52 caused all the crates to be updated and newly compiled, especially for all the crates used with rayon.

I thought it was noteworthy to mention this, because the same source code will produce varied performance solely based on how its compiled. For the unknowing Rust programmer, they may be trying to get more speed by doing source code tweaks, when really they just need to use optimal compiling options for their system.

@edin
Copy link
Owner

edin commented May 25, 2021

I don't have much experience with Rust, but if something can be tuned using various flags than there is no reason for not using it - it's better to keep source code idiomatic as possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants