Cycles: Refactor Progress system to provide better estimates
ClosedPublic
Actions

Authored by Lukas Stockner (lukasstockner97) on Sep 9 2016, 12:03 AM.

Details

Reviewers

Brecht Van Lommel (brecht)
Sergey Sharybin (sergey)

Group Reviewers

Cycles

Commits

rCb0e35ba13204: Refactor Progress system to provide better estimates
rBdd156f743c04: Cycles: Refactor Progress system to provide better estimates
rCc7938e448b75: Refactor Progress system to provide better estimates
rBSa2ebc5268f2b: Cycles: Refactor Progress system to provide better estimates
rBa2ebc5268f2b: Cycles: Refactor Progress system to provide better estimates

Summary

The Progress system in Cycles had two limitations so far:

It just counted tiles, but ignored their size. For example, when rendering a 600x500 image with 512x512 tiles, the right 88x500 tile would count for 50% of the progress, although it only covers 15% of the image.
Scene update time was incorrectly counted as rendering time - therefore, the remaining time started very long and gradually decreased.

This patch fixes both problems:
First of all, the Progress now has a function to ignore time spans, and that is used to ignore scene update time.
The larger change is the tile size: Instead of counting samples per tile, so that the final value is num_samples*num_times, the code now counts every sample for every pixel, so that the final value is num_samples*num_pixels.

Along with that, some unused variables were removed from the Progress and Session classes.

Diff Detail

Repository: rB Blender

Event Timeline

Lukas Stockner (lukasstockner97) updated this revision to Diff 7396.Sep 9 2016, 12:03 AM

Lukas Stockner (lukasstockner97) retitled this revision from to Cycles: Refactor Progress system to provide better estimates.

Lukas Stockner (lukasstockner97) added a reviewer: Cycles.

Lukas Stockner (lukasstockner97) updated this object.

The main thing I'm not sure about is Cycles Standalone - for now I just commented out the samples part so that it compiles.

Here is an example - remaining time plotted against elapsed time in a scene with huge World MIS resolution:

With patch:

Without patch:

Some of the code parts seems much clear and simpler now.

Not sure what is the issue with standalone, but those are to be solved before commit.

intern/cycles/render/bake.cpp
138	While you're on it, can we drop use of `this->` ? Calling local variable same as member is confusing anyway and should be avoid.
intern/cycles/render/tile.cpp
405	Is this code running on every tile update?
409	`divider >> 1` ? :)
intern/cycles/render/tile.h
101	What is a pixel sample? Also, capital. And capital above...

This revision now requires changes to proceed.Sep 9 2016, 9:15 AM

Lukas Stockner (lukasstockner97) mentioned this in rB530673685b50: Experimental build containing updated denoising and various other patches.Sep 9 2016, 9:39 PM

Pretty sure it has something to do with this patch (or the denoising) -- used this build https://developer.blender.org/rBd7f405b726d9f7e4d315453aacb7f1dc83bfecfa on the build bot

Rendering at 120samples, there are 8 tiles... the remaining time stops just before the 7th tile finishes (around the 100sample mark)

Minor comments.

intern/cycles/app/cycles_standalone.cpp
77–85	Might as well remove uncommented code here and in other places.
intern/cycles/blender/blender_session.cpp
932–933	Don't use `long`, this has different sizes on unix and windows. Best to use something like `int64_t` if you need a large int.
intern/cycles/device/device_cuda.cpp
118	`{` on new line.

Addressed review:

this-> isn't used anymore in the bake manager.
The total pixel calculation is now only executed once while resetting the tile manager.
What I mean with "pixel samples" should be clearer from the comment in util_progress.h

The commented code in standalone was the sample stuff where I was unsure how to replace it - now it just prints progress instead.
Replaced long with uint64_t, and also fixed types in TileManager (previously, the total calculation was performed in int, which kind of made the long/uint64_t pointless).
Code style is fixed.

Also, I moved the progress calculation into the Progress class by storing the total pixel sample count in it from the Session.
This deduplicates some code - instead of calculating progress both for rendering and baking in the BlenderSession and now also in Standalone, it's done once in the Progress class (where it belongs imho).

Lukas Stockner (lukasstockner97) mentioned this in rBc319d47f8e7c: Another experimental Cycles build.Oct 3 2016, 4:11 AM

Lukas Stockner (lukasstockner97) mentioned this in rBd96ac3ff3357: Another experimental Cycles build, second version.Oct 3 2016, 6:08 PM

Rebased to current master.

Don't have stopper concerns actually.

Just make sure it work before committing ;)

intern/cycles/device/device_task.cpp
112	Would be cool to avoid having this in DeviceTask. Maybe can pass number of pixels as an argument (if that makes things easier to calculate in the callee). Not so much concerned about about considering this a stopper tho.

This revision now requires changes to proceed.Nov 28 2016, 4:46 PM