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

C produces no viewable image, Rust produces a different image from all others. #11

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

Comments

@jzakiya
Copy link
Contributor

jzakiya commented Apr 15, 2021

When I ran the C code version, it produces a bad *.bmp file of 1,000,102 bytes, which doesn't show an image.
Also, while the Rust code produces a viewable image, the image has different shading from all the others.

Here are comparisons with other *.bmp files that produce correct viewable images.

➜  raytracer ls -al *bmp
-rw-rw-r-- 1 jzakiya jzakiya 1000054 Apr 10 01:28 cpp-raytracer.bmp
-rw-rw-r-- 1 jzakiya jzakiya 1000102 Apr 10 01:28 c-raytracer.bmp
-rw-rw-r-- 1 jzakiya jzakiya 1000054 Apr 10 01:27 d-raytracer.bmp
-rw-rw-r-- 1 jzakiya jzakiya 1000054 Mar 25 11:59 julia-ray.bmp
-rw-rw-r-- 1 jzakiya jzakiya 1000054 Mar 24 17:04 nim-raytracer.bmp
-rw-rw-r-- 1 jzakiya jzakiya 1000054 Apr 10 01:28 nim-RayTracer.bmp
-rw-rw-r-- 1 jzakiya jzakiya  750054 Mar 15 12:43 ruby-raytracer-266.bmp
-rw-rw-r-- 1 jzakiya jzakiya  750054 Mar 15 12:35 ruby-raytracer-272.bmp
-rw-rw-r-- 1 jzakiya jzakiya  750054 Mar 15 12:51 ruby-raytracer-jruby9.2.16.0.bmp
-rw-rw-r-- 1 jzakiya jzakiya  750054 Mar 24 16:54 rust-raytracer.bmp
@jzakiya
Copy link
Contributor Author

jzakiya commented Apr 16, 2021

Performing additional diffs between the *.bmp images reveal further differences.

Using cpp-raytracer.bmp as the references for those sized *.bmps indicates the Julia output is not identical to the C++, D, and original Nim version.

For new fast Nim version *.bmp, the spheres are slightly smaller than the original Nim (slower) version, and thus not identical to the C++ and D versions too.

@edin
Copy link
Owner

edin commented Apr 18, 2021

What compiler are you using for C ? I believe that issue is with #pragma pack(push, 1) here if struct is not packed properly then bitmap header is invalid.

@jzakiya
Copy link
Contributor Author

jzakiya commented Apr 19, 2021

I'm using the C file from the site, compiling with:

g++ RayTracer.c -x c -O3 -o raytracer-c.exe

on Linux using

➜  raytracer g++ --version
g++ (PCLinuxOS 10.3.0-1pclos2021) 10.3.0
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@edin
Copy link
Owner

edin commented Apr 20, 2021

I think it should work now Linux and 64 bit compiler as fixed sized types are used now in BITMAPFILEHEADER and BITMAPINFOHEADER structs.

@jzakiya
Copy link
Contributor Author

jzakiya commented Apr 20, 2021

Confirmed the new C version now produces a viewable *.bmp, with the same size as the others.
However, doing a diff on the c-raytracer.bmp and cpp-raytracer.bmp shows they're not identical, but it doesn't seem to be in the image. Maybe some values in the file header are slightly different.

Have you looked at Rust to determine why its image file has different shading?

@edin
Copy link
Owner

edin commented Apr 20, 2021

I've looked at the Rust example and the sphere normal was inverted, it should work now.

@jzakiya
Copy link
Contributor Author

jzakiya commented Apr 20, 2021

Confirmed the new Rust version produces correct *.bmp, with same size as the Ruby version, but their diffs are also not identical, and suspect it's also probably in file header value(s), otherwise I detect no differences in the images.

The only other thing I've detected between the versions I've been able to run, is that the new faster Nim version, which uses the Quake integer squareroot method, produces slightly smaller spheres in the image versus the original Nim version, and all others. So if that's going to "cheat" (as they say) it should produce the same image, so the timings|images are apples-to-apples.

Suggestion:
Can you produce a generic timing wrapper, or internal code for each version, to run the versions multiple times and average them, to get repeatable results to at least 3 stable digits, especially for the fast versions. For the Crystal version, I timed it using 1000 iterations of the rendering code to get repeatable stable values, for Ruby (which is much slower) I did 10 iterations.

On my system (i7-6700HQ, 3.5GHz), Crystal was 84-85ms, C|C++ between 75-78ms, and fast Nim version 42-45ms.

@edin
Copy link
Owner

edin commented Apr 20, 2021

I've started tool in C# in tools folder, it should be able to compare image and produce diff so it's easy to spot difference in color.

For benchmarking maybe we can use something like hyperfine (https://github.com/sharkdp/hyperfine) like it's used in javascript test.

@edin
Copy link
Owner

edin commented Apr 20, 2021

Currently I have really old CPU (FX 8120) it's from 2011, so timing shown in readme are much slower usually 2 to 3 slower compared to latest cpus.

Nim version has some extra optimization, it uses fast square root algoritham (but it may be less accourate) and instead double it uses float for calculations - I've used double data type for most versions as I think that javascript internaly is using double to store numbers.

I've tested crystal in WSL and I am impressed with speed here it takes 195 ms, only 30 ms more than C.

@jzakiya
Copy link
Contributor Author

jzakiya commented Apr 20, 2021

Yes, something like that for timings would be very nice, to get truer consistent values across all implementations.

And it would also be nice to get timings on different newer hardware (i9, AMD Ryzen, ARM, PowerPC, RiscV, etc).

Someone ran the Crystal version on an AMD Ryzen 7 3700x, and got 63ms, vs my 84-85ms on an (2016) i7-6700HQ laptop.

@drhuffman12
Copy link

I have a few systems I can help run the comparison script on when ready.

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