search

mmp / pbrt-v3

Source code for pbrt, the renderer described in the third edition of "Physically Based Rendering: From Theory To Implementation", by Matt Pharr, Wenzel Jakob, and Greg Humphreys.
http://pbrt.org
BSD 2-Clause "Simplified" License
4.88k stars 1.19k forks source link

Better Parsing Structure #139

Closed klein-j closed 6 years ago

klein-j commented 7 years ago

This is a suggestion for future code refactoring.

The parsing code should be completely separated from the scene rendering. Currently, the rendering is started in pbrtWorldEnd, which is not exactly nice. This becomes apparent when the internal state of the profiler must be reset, which is already described in the comments as 'kind of ugly'.

I ran into this issue, when I tried to add additional command line arguments to override certain parameters in the scene file (which is quite useful for automatic rendering of different parameters). Right now this is not nicely possible, as the scene is build while the file is being parsed. pbrt already does something similar and stores a filename in a global variable to use it for scene construction, which is also not exactly nice.

I would propose to separate the phases more cleanly. At first, the file would be parsed, than the internal representation can be changed (according to command line parameters or so), then the scene would be built and finally rendered. I believe this would make the whole program structure more clear (I was very surprised, when I found the actual rendering call in pbrtWorldEnd, when I started to work with the code for the first time) and easier to extend. I realize, that this might require a few more temporary variables and thus slightly impact performance. however, I don't think that scene parsing is the main bottleneck of rendering, and furthermore it should be possible to just C++11-move everything big (e.g. triangle meshes), so that there would be no real overhead due to this restructuring.

As I would like to see this happen, I could maybe help out with this refactoring, but it probably would depend on my free time. However, I first wanted to see, if others agree, and think that this should be done.

klein-j commented 7 years ago

Also adding to this: Currently, if a filename is provided in the input file and the command line, the input file has the higher priority. I find this somewhat irritating, as I see command line arguments as more volatile and easier to change that the scene file. Or, in other words an "Oh no, the file specifies the wrong output name I better fix this with a command line parameter" makes more sense to me as an "Oh no, the command line is wrong. I better fix this by editing the scene file". Ideally, I would like to see a system, where (to the extend where it makes sense) all parameters in a scene file could be overwritten by the command line.

mmp commented 6 years ago

That's a good suggestion (and I agree with the point that starting rendering in pbrtWorldEnd() is a bit "surprising".) It's too big a change for pbrt-v3, but we'll seriously consider it for pbrt-v4.

If you could be more specific about what sort of things you'd like to be able to do in terms of overriding parameters, that would be helpful, just so we can make sure that whatever we end up doing works for that, that'd be a good test.

klein-j commented 6 years ago

If you could be more specific about what sort of things you'd like to be able to do in terms of overriding parameters, that would be helpful, just so we can make sure that whatever we end up doing works for that, that'd be a good test.

As a first step, I am thinking about all global scene parameters, e.g. the content of the RenderOptions struct. Maybe creating cameras, integrators and so on could be delayed in a way that during file parsing only the type and parameter list is read, and then partially or completely overwritten during command line parsing. After that, the objects would be created with the updated parameters. (There could be an option to create a "new" object, where each parameter that is not specified in the command line is set to the default value again, i.e. clearing the parameter list before adding the new parameters from the command line).

As a second step, it might also be interesting to even change objects in the scene hierarchy or the hierarchy itself with command line parameters. E.g. rendering the same scene a hundred times where only the position of one object changes. However this would require some sort of sophisticated object addressing, possibly by specifying a route through the scene tree or by adding a 'unique identifier' parameter to objects.

While it is true, that it might seem like a overkill to add command line arguments for some variables (e.g. up/vector of camera), it is certainly extremely useful for others (e.g. SampleNumber / Resolution / OutputFile). These parameters already exist, but in a sort of hacky way. I think a general system that would allow changing everything could be more flexible and robust while at the same time not make the parsing structure any more complex, just more general (thus allowing every parameter to change would actually be easier, than just adding support for the most common, as everything is handled the same way).

mmp commented 6 years ago

Thanks; these are good ideas. Noted for the 4th edition. :-)