Details

Reviewers

Commits

rBc9d821294f08: Cycles: take into account time limit for progress bar

Summary

This change allows the Cycles progress report system to take into conderation
the time limit property. This allows for more accuracte progress
reports for high sample count renders with short time limits.

Diff Detail

Repository

rB Blender

Branch

time_limit_impacts_progress (branched from master)

Build Status

Buildable 23233
Build 23233: arc lint + arc unit

Event Timeline

Alaska (Alaska) requested review of this revision.Aug 2 2022, 6:10 AM

Alaska (Alaska) created this revision.

Harbormaster completed remote builds in B23230: Diff 54285.Aug 2 2022, 6:10 AM

For reference, this change makes it so the percentage on the progress bar is more accurate for time limited renders.

Account for issues with tiles and time limits

Harbormaster completed remote builds in B23233: Diff 54293.Aug 2 2022, 8:27 AM

In D15599#423334, @Alaska (Alaska) wrote:

Account for issues with tiles and time limits

At the moment the time limit is the time limit for rendering per tile. So if you have a time limit of 10s for four tiles, then each tile is given 10s to render, which means the total render time is 40s.
See T92106 for further information?

Since I personally do not have the skills to correct this issue, I have instead opted to take this issue into consideration when reporting the remaining render times until someone fixes the original issue.

Thanks for working on it.

Indeed the progress can be improved, and it should quite straightforward weak to the patch to improve progress report of a single tile.

The multi-tile and time limit needs deeper change. For now it would be acceptable to consider that the time limit is per tile. But ideally the render scheduler will measure time of first few samples of the first tile and estimate what the per-tile time should be, an for all the subsequent tiles will use the same number of tiles as for the first tile. Sounds a bit convoluted, but basically the goals are:

Make Time Limit to be always limiting full frame render time (with some margin of error).
Render same number of samples in all tiles.

But this is a bigger change and should be handled separately from fixing remaining time for a single tile.

intern/cycles/util/progress.h
207	This line is weird to me. I am not sure what is the physical equivalent of mixing pixel-based progress calculation with time-based. I'd imagine that if there is a time limit enabled then we need to use the time based estimate (and clamp to 100% since the actual render time might be higher than the limit due to overshoot in number of samples in scheduler. Also, the time based calculation needs to happen when path tracing starts. As it is currently it seems that synchronization step will use path tracing time limit to calculate progress and remaining time. You can see this when rendering a scene with a substantial synchronisation time.

This revision now requires changes to proceed.Aug 8 2022, 3:02 PM

Fix incorrect progress report at the end of scene initialization
Cap progress percentage to 100%
Fix incorrect remaining time for scenes with long sync times
make format

Harbormaster completed remote builds in B23304: Diff 54530.Aug 9 2022, 9:26 AM

Alaska (Alaska) added inline comments.Aug 9 2022, 9:26 AM

intern/cycles/util/progress.h
207	This line is weird to me. I am not sure what is the physical equivalent of mixing pixel-based progress calculation with time-based. This is how I see it. If the pixel-based progress calculation says that the render will be longer than the time limit, then the time limit based progress report is more accurate, and thus we will use that. But if the pixel-based progress calculations end up suggesting that the render will be shorter than the time limit, then we should use that to report the progress. And this line seems to handle that. I have attached a video show casing this with progress bars throughout a render in a theoretical scene where the time limit is longer than the actual time to render, and adaptive sampling is used, allowing for a render speed up at the end. F13347886 (and clamp to 100% since the actual render time might be higher than the limit due to overshoot in number of samples in scheduler.) Other parts of the code either in Cycles or the Blender UI already takes care of that to ensure the user facing "progress completed" is never greater than 100% However I have made the change to limit it to 100% just in case there is a piece of code on the backend that doesn't work well with a greater than 100% completion. Also, the time based calculation needs to happen when path tracing starts. As it is currently it seems that synchronization step will use path tracing time limit to calculate progress and remaining time. I believe I have address this issue in the latest revision of the patch, however I may of mis-interpreted what you meant by that and may of implemented it poorly.

Brecht Van Lommel (brecht) requested changes to this revision.Aug 10 2022, 3:33 PM

Brecht Van Lommel (brecht) added inline comments.

intern/cycles/session/session.cpp
499	Can you use `const double time_limit = render_scheduler_.get_time_limit() * ((double)tile_manager_.get_num_tiles());` here also and just pass a single time limit. Best to not spread such logic across multiple classes.
intern/cycles/util/progress.h
207	Fix incorrect remaining time for scenes with long sync times Fixing that here seems wrong. Synchronization is not part of this progress % when there is no time limit, so it shouldn't be either when there is a time limit.