[Beginner][Feedback] This path tracer is my first Go program. How would you improve it?

I'm learning Go by writing a physically-based path-tracing renderer. This is the first thing I've written in the language, and I'm learning along the way, so I'd appreciate any feedback more experienced gophers have.

Cheers!

---

评论：

akavel:

At first glance through e.g. camera.go, for now I'd say rather just fairly small stuff:


"Camera simulates a camera" - hmh, personally don't like this much. Doesn't really say anything, not worth the letters in my opinion. Could try maybe dwelling a bit more on the properties it has, what is described about the camera, maybe why? why this and not something else?

as mentioned by someone else, tests would be advisable (and esp. consider table-driven tests)

Focus's godoc is non-understandable to me; I have no idea what the func really does (at first I thought maybe it's something similar to what LookAt does)

Looking at https://godoc.org/github.com/hunterloftis/pbr/pbr:


you don't seem to have a package doc for pbr/pbr. Especially, I don't really know how to start using the package without some intro guide.

personally, I'd rather avoid putting the package in pbr/pbr/, I'd just put it in pbr/

by convention, consts should preferably be named same as normal variables (thus Bias or bias, not BIAS, etc.)

looking at Cli in godoc, I have no idea how to use it and what it does

"Hit describes an intersection" - of what?

"SampleFrame samples a frame" - ok, this much I guessed already from the name, but I totally still don't understand what the function actually does (kinda like the infamous i++; // increment i comment) and how to use it

etc.

edit:


ok, looking somewhat more at the API: I'd highly recommend trying hard to hide as much internal stuff as possible from the public API. E.g. for Sphere, methods Intersect, MaterialAt and NormalAt don't seem of any sensible use to the package user, they seem purely internal details, so they just introduce noise in the API. It would be cleaner and easier to understand if you made them private. Unless they are required for some broader 'Shape' interface or something, but I don't see one here (and anyway, in such case I'd put them (i.e. the Shapes) in some subpackage to group them more explicitly).

Renderer's Rgb, Write, WriteRGB seem partly redundant; also by convention it's good to try fitting some of the stdlib interfaces/func names, e.g. WriteTo, and avoiding clashes (e.g. I'd usually expect Write to have a signature fitting io.Writer; otherwise I'd try inventing a different name); also try using interface like io.Writer instead of taking name of a file - this way you give the user more possibilities.

edit 2:

ok, first heavier issue: never ignore errors. And anyway, you should rather take an io.Reader instead of a file string. I see you may have copied this fragment of code from example code in rgbe package's readme, but that's very not good. Actually, that's the reason why ignoring errors even in examples is often frowned upon.

edit 3:
in Glass etc., describe in godoc what's the meaning of the gloss parameter and what values one can put there. Other than that, it's cool you've added such constructors.

Ok, enough for now, that should give you some ideas for the direction to go with docs & API cleanup.

edit 4: ouch, looking at Scene.Add() - the parameter is of type surface, which is unexported, so user won't know what he/she can actually put there. You should make the surface interface exported. Also, you could make Scene.Add() take variable number of arguments, this should remove some repetitiveness from the code in cmd/ subdir.

hunterloftis:

Thanks for the detailed feedback!

GoDoc stuff: I only wrote anything at all because my editor complains when I don't. I should go through and flesh it out.

Table-driven tests: Thanks for the keyword, I haven't seen this before.

Package docs: I see I can document a package by writing comments above it. Since lots of files compose a package, how do folks tend to choose the file in which to document the package?

pbr/ vs pbr/pbr: Since root dirs get filled with metadata (.gitignore, .gitattributes, readme.md, etc) I like to keep source files separate. The only way I could think of to do this with Go's "package == dirname" convention was to use pbr/pbr.

Const naming: Got it!

Hiding Intersect etc methods: You're right! I wrote those methods before I realized that lowercase is still package-wide (vs file-wide) scope.

Renderer write interface: Good suggestions; I'm not really very familiar with the stdlib interfaces at the moment, but I'll see if I can make Renderer a Writer.

Scene.Add: Great suggestions, thanks!

akavel:

Quick reply:

Package docs: Personally I usually try to choose some kind of most important file in the package and put it there. But especially if you write a longer intro comment, the convention is to create a separate file named doc.go and put only the (long) package comment there. Search for such files in the stdlib (I suppose the longest is in package fmt, but I'd expect most packages there have it).

Hiding Intersect [...]: Actually, after edit 4, I believe you should rather keep them public, and make the surface interface public instead (i.e. Surface). This would allow other people to add their own surfaces and have them handled correctly by the renderer (as long as you didn't cheat by downcasting in your implementation). Personally, I'd then strongly consider putting them (i.e. Sphere etc., maybe also Surface itself but not necessarily) in a separate package, e.g. pbr/pbr/shape (shorter name than surface; also then shape.Surface does actually make quite much sense).

Renderer vs. io.Write: I meant rather making it an io.WriterTo, i.e.:

func (r *Renderer) WriteTo(w io.Writer) (int64, error)

or even better, make it return an image.Image, so that a user can then decide whether to encode it as a PNG, or maybe as a JPEG, or display on screen, or whatever else. The idea of interfaces in Go feels somewhat like pipes in Linux if used properly.
Consts: one additional question here is do you even actually need/want them exposed in the API, or are they again just a private implementation detail.

faiface:

This looks amazing! Code looks fine at the first glance, good job, man!

hunterloftis:

Thanks! It's been a great, fun way to dive into Go.

plectid:

I assume it's not your first renderer and you've ported it from some other language? The code is clean, very readable and way shorter than I expected for a working path tracer. 
There are some minor nitpicks, such as why do materials return struct values, which you pass later as references, instead of returning pointers in the first place.
Not seeing any tests in a go app is unusual. Other than that, good job, that doesn't look like the first thing you've written in Go.

hunterloftis:

Thanks for checking it out.

why do materials return struct values, which you pass later as references, instead of returning pointers in the first place.

Do you mean Surfaces? I see what you mean there with the repetitive "scene.Add(&pbr.Cube"
Also, +1 to tests... I just want to sort of get the "shape" of it right before I start writing formal tests.
Not my first renderer, but I'm no expert. I play with a lot of graphics stuff in other languages (and now in Go!):

http://www.playfuljs.com/
https://github.com/hunterloftis/pathtracer

plectid:

What the performance difference between js and go? It would be interesting to compare the two in single-threaded and multithreaded environments and if golang performance scales linearly with the number of worker threads.

hunterloftis:

I regret not taking exact measurements when their behavior was 1:1. Roughly, though:

1x JS => 5x Go => 15x Go + Goroutines
https://twitter.com/HunterLoftis/status/875783656765247488

allhatenocattle:

Really neat project!
https://github.com/hunterloftis/pbr/blob/master/pbr/sampler.go#L36
This rand variable is being passed down a couple of calling layers before being used.
A simpler way is to seed the rand package in one file with
func init() {
   rand.Seed(time.Now().UnixNano())
}

and then when you need a random number, just use
rand.Float64(), no need to create *rand.Rand and pass them around.
https://github.com/hunterloftis/pbr/blob/a14583bfcdbf2ce027caea1b517275cb316775ee/pbr/cli.go#L69-L71
Since the only action is incrementing across multiple goroutines, take a look at the sync/atomic package, seems a more natural fit.

hunterloftis:

Will that make the various goroutines block?
Initially I was just using the standard rand, and my concurrent version was actually slower (!) than the single-threaded version. After hours of debugging I discovered that the standard rand uses mutexes to be thread-safe, and since rand.Float64() is one of the most often-called functions in the renderer (called literally billions of times), that mutex was destroying performance. That's when I started instantiating a new rand instance for each Sampler and passing it down.
I would love a method that doesn't involve so much argument-passing, but at the same time the explicitness is nice (and clear). Or for default rand to have smarter concurrent behavior.

allhatenocattle:

Ah, I didn't realize that.  I've never used rand anywhere enough that it caused a material slowdown. TIL.  Cheers.

hunterloftis:

Thanks /u/akavel, /u/plectid, /u/allhatenocattle, /u/Sythe2o0 for your code reviews. I've updated the project based on your feedback:

The top level API is much nicer now.
It has package docs and better godocs.
I've updated the readme instructions with build, test, CLI options.
The Renderer returns an image.Image in Rgb() and Heat() for end users to use however they like.
The Cli is now responsible for writing to pngs
I also replaced some magic numbers with constants.

akavel:

Cool, much better. Still rough around the edges, but you'll learn the finer aspects with time. I do especially like the package doc - brief, clear, and to the point; an awesome intro and first impression, makes a world of a difference in usability and approachability of the package. 
Two more quick notes:

you totally don't need to setup your own hosting for godoc, there's https://godoc.org, it's owned by Google and people are used to just going to https://godoc.org/github.com/hunterloftis/pbr/pbr
regarding your TODO: image.Image is an interface; as a general rule, you'll never really have a practical need to make a pointer to an interface.

As a final tip, I very much recommend studying the stdlib, you will discover many cool idioms.

hunterloftis:

Wow! TIL. I'm really impressed with Go's tooling and ecosystem, had no idea godoc.org auto-generated Github package docs. Just linked there instead.
Thanks for all your suggestions!

Sythe2o0:

The test you do have isn't written in Go, so someone on Windows can't run it without using a non-standard terminal. 

hunterloftis:

That's true; it's just a bash script that runs and times the go program with common options. I should document how to run it without bash though.

Sythe2o0:

Well, there is a standard for writing tests in Go that can time and benchmark things cross-platform, and wouldn't require you to point out how to run the tests at all with the testing package.