Code cleanup

[Tyler799]

I did a bit of cleanup like I said I would. I installed the latest clang-format, and ran it automatically and manually using an atom package. Most of the time it automatically moved brackets, but I think it missed a couple. Didn't have any easy way to run against the tests, so we'll see if I missed anything.

There might be some things that look weird in the diff. Feel free to ask me and I'll explain them.

I didn't get to do as much as I wanted, I think some of what I had noticed was from an old 2017 copy and was later fixed.

Here are the main patterns:

1: If (branch) ... if (hit) was a very common pattern. However, hit is the root conditional common to both branches, so I pulled it out.

2: I used std::min and std::max wherever it seemed appropriate. I avoided anywhere I saw other things being done in the conditional.

3: Added a bit of space for readability. If you really prefer I can remove it, but clang-format doesn't complain and it really does make it easier on the eyes.

Also, I was able to simplify GetBoundingBox() and BoundingBox(). There was an odd code block that wasn't necessary at all, as well as a couple extra sets of variables to all do the same thing - find min and max.

If there's anything critical you notice needs to be fixed, let me know and I'll try to remember for anything in future.

Final note:

I've noticed a fairly large amount of duplicated code. I am not sure about the best way to approach it in general, but easily dozens or hundreds of lines could be cut out of here.

Some like:

• if(hit) ... if (node.flag == 0) shows up 3 times. The only difference is the method called in the else condition - I think a function pointer or something could cut those 3 to 1.

• The "set min and max to initial bbox, walk over all vertices of tri, over all xyz, find min/max" is repeated many times with slight variation. Also could cut out lots of duplicate code that all does the same thing.

• Redundant indexing. Compiler should take care of these, but it doesn't hurt to remove it when possible.

[syoyo]

Awesome! Will take a look soon.

Theres is no automatic regression tests and unit tests in NanoRT. Conforming clean-up'ed code does not break something is compile and run some examples and compare resulting image visually.

[Tyler799]

Thanks!

[syoyo]

Changes reveal some warnings when compiling clang with increased compiler warning levels.

For example,

https://github.com/lighttransport/nanort/pull/52/files#diff-6ae3ef545df31bcbb8d6f3814304e805R1493

later idx variable definition is shadowed by this idx definition.

Please use clang and set -Weverything -Werror -Wno-padded -Wno-c++98-compat-pedantic as a mandatory compiler flags.

Optionally, you can set -Wno-system-headers -Wno-documentation -Wno-documentation-unknown-command -Wno-reserved-id-macro

[Tyler799]

Given this is just a header, I do not know how to compile it.

What subprojects/examples would be easiest to set up to build it with?

[Tyler799]

Nevermind, I see now. There is a make file and a dummy .cc file to include the header. I'll work on it tonight after work.

[Tyler799]

Went ahead and fixed that issue, made sure it compiled. Also did some other minor comment and code cleanup.

Hopefully that's good, but let me know if there's anything else that needs to be addressed.

[syoyo]

Thank! merged! (I've added a small modification https://github.com/lighttransport/nanort/commit/0d827e6a25689f7a58c8eeff7645bebfb93f4fd8#diff-6ae3ef545df31bcbb8d6f3814304e805R889)

[Tyler799]

Thank you as well. :)