Developer note: I changed where the sample loop is, this way we can update the progress bar every time a sample is finished and in the future even update the result buffer for baking preview.
Details
Details
Diff Detail
Diff Detail
- Branch
- bake-progress-1tile
Event Timeline
Comment Actions
Please read my new comments. Also I did not test it with CUDA (no nvidia here) but it should be working.
| intern/cycles/blender/blender_session.cpp | ||
|---|---|---|
| 558 | This is actually not needed, the default update function includes some redraw routine that is not required, but I think it's fine to use it instead of the update_status_progress. So consider this gone in the final patch, unless someone has any consideration on the matter. | |
| intern/cycles/render/bake.cpp | ||
| 163 | My main concern with this approach (and one I would like to hear from other devs) is if we are adding too much overhead by dispatching a task job per sample. An alternative is to treat the individual tasks as tiles, so when they increment sample it's counted against the overall samples (num_samples * num tasks). That said I think this approach is better in the future when we want to have preview of the baking result back to Blender. For baking it's better to show the preview of the entire image per sample instead of per parts I believe (though again we may need a threshold to not update things every sample). Thoughts? | |
Comment Actions
I'm really unsure about most of the changes you made here. Moving samples cycle from device to session would likely lead to less effective cache utilization. If you'll dispatch individual samples of tiles as separate tasks you'll have even less effective cache usage and will introduce quite reasonable amount of overhead.
Also i have no idea why do you need all those changes and not just probably separate progress_update callback. Cycles is totally capable of reporting progress when using single tile (which as i understood is the main reason why you started to bother with all those changes).
Comment Actions
@Sergey Sharybin (sergey) what cache? I don't think there is any cache going on for the shader tasks (aka baking).
If you'll dispatch individual samples of tiles as separate tasks you'll have even less effective cache usage and will introduce quite reasonable amount of overhead.
But there are and won't be no tiles, those changes are exclusive to shader tasks.
Also i have no idea why do you need all those changes and not just probably separate progress_update callback.
You mean a 'progress_update' routine that takes into account the specificities of shader baking? As I mentioned above, even if we could mimic the tiles code I still think we can benefit from baking per samples (for an upcoming feature 'preview update').
I will start drafting the 'preview update' routine to give me a better picture of that, but I believe there will be more overhead with preview for baking than we have for rendering, thus it may be better to preview update only once per sample (instead of per tile).
Comment Actions
I'm talking about CPU cache.
But there are and won't be no tiles, those changes are exclusive to shader tasks.
I don't really see they're exclusive for shader tasks actually. Moving stuff out from the device also affects rendering. Also not sure why shader tasks are so unique that requires special handling?
What's the "preview update" for baking anyway? Just updating of the image editor when new sample arrives? Bet it's doable the same way as viewport.
Comment Actions
Another attempt, @Sergey Sharybin (sergey) what do you think?
I don't know how to get the number of tasks for GPU (cuda/opencl)
I don't even have a CUDA at hand so I'm actually curious to see what this code is printing (there is a debug printing that goes from 0.0 to 1.0 - for CPU, for CUDA it may go from 0.0 to 9999.0]
Comment Actions
Yet another take (+ rebase from upstream/master)
This one should be cleaner, it works fine in CPU and it should be working in GPU as well.
Comment Actions
Somewhat like the patch much better now. Some minor feedback.
| intern/cycles/device/device.h | ||
|---|---|---|
| 125 | Naming suggestion: get_split_task_count() | |
| intern/cycles/device/device_cuda.cpp | ||
| 821 | Suggestion: Make DeviceTask::update_prgoress to use a pointer to render tile insteado f a reference and call it from here with NULL tile. This way you wouldn't worry about calling update to often. | |
| intern/cycles/device/device_task.h | ||
| 55 ↗ | (On Diff #2192) | Same as above. Or maybe get_subtask_count. |
Comment Actions
Renames based on Sergey's suggestion, and changes to update_progress based on his suggestion as well
I like the update_progress change, specially because it allows in the future for baking to use the same framework as rendering when it comes to update (assuming we find a way to do so).
Comment Actions
Apart from some unfinished debug code (by the looks of it) LGTM.
| source/blender/editors/object/object_bake_api.c | ||
|---|---|---|
| 143 ↗ | (On Diff #2217) | I'd suggest dropping from the current patch. |
Comment Actions
- Revert "tmp2"
- Revert "tmp"
- Merge remote-tracking branch 'upstream/master' into bake-progress-noadd