Cycles/Render API: Implement "Set Render focus" feature
Needs RevisionPublic
Actions

Authored by Lukas Stockner (lukasstockner97) on Jan 12 2016, 12:45 AM.

Details

Reviewers

Thomas Dinges (dingto)
Sergey Sharybin (sergey)

Summary

This patch adds "Set Render focus" to the RenderEngine API and implements it in Cycles.
Generally, the task of the function is to allow users to specify areas that they want the renderer to focus on
by clicking into the rendering image with Ctrl held down. This is a bit vague on purpose to allow different engines
to implement it as it is suitable for them.

The Cycles implementation reacts to this function by reordering the tiles so that the origin of the Circle and
Hilbert Spiral patterns now is at the clicked position. This allows users to specify which areas should be processed
next for a quicker preview and to allow to check specific off-center areas before the rendering would normally reach them.

The UI side is loosely based on T37220.
ToDo: BI support!

Also includes (would be committed separately, but not worth opening a second diff):

Cycles: Move Hilbert Spiral generation to separate function, deduplicate tile generation

The Hilbert Spiral code is now separated from the tile generation code, which allows to
handle all tile orders in a single loop and therefore deduplicate some code.

Diff Detail

Repository: rB Blender
Branch: click_center

Event Timeline

Lukas Stockner (lukasstockner97) updated this revision to Diff 5800.Jan 12 2016, 12:45 AM

Lukas Stockner (lukasstockner97) retitled this revision from to Cycles/Render API: Implement "Set Render focus" feature.

Lukas Stockner (lukasstockner97) added reviewers: Thomas Dinges (dingto), Sergey Sharybin (sergey).

Lukas Stockner (lukasstockner97) updated this object.

Lukas Stockner (lukasstockner97) added subscribers: cédric lepiller (pitiwazou), Wolfgang Faehnle (mib2berlin).

Here's an example image (stopped after some time):

Also, I just noticed that weird things sometimes happen to the brightness of the image, probably due to some threading conflict when accessing the samples variable that the buffer uses. I'll have a look tomorrow.
Another addition that would make this more useful would be to have a kind of prepass that samples everything at 10% of the samples before fully refining the rest of the image - that might even be a replacement for the progressive refine mode.

Thomas Dinges (dingto) awarded a token.Jan 12 2016, 1:04 AM

Rainer Trummer (aliasguru) awarded a token.Jan 12 2016, 9:03 AM

Initial review.

Also, focus is a bad name, conflicts with camera focus and sounds as re-focus from CV area (was my first idea actually and I thought "WOOOW").

Maybe call it " point of interest" or just a "render center" ?

intern/cycles/blender/addon/__init__.py
92	Don't do this, assume Cycles version matches the addon's one.
intern/cycles/blender/blender_session.h
60	Any reason to not use `float2` here?
intern/cycles/render/session.h
143	Thats where `float2` can be used, matching tile api.
intern/cycles/render/tile.cpp
296	Seems to lack a lock here, causing conflict with tule acquisition.
source/blender/editors/render/render_internal.c
771	Kinda weak. Use modal keymap instead.

This revision now requires changes to proceed.Jan 12 2016, 10:02 AM

Yes, the name might be confusing - maybe calling it something like "interact" and passing the keyboard state (ctrl, shift, alt) would also work, but that conflicts with the image sampling that currently happens when clicking or dragging on the image...

intern/cycles/blender/blender_session.h
60	Not really, I just wasn't sure whether it's fine to use floatx types here.
intern/cycles/render/tile.cpp
296	A lock on the tile_mutex is acquired in the Session code before calling set_center, so that shouldn't be a problem.
source/blender/editors/render/render_internal.c
771	OK, I'll have another look into the UI stuff. TBH, I had enough after 3h of reading WM and event handling code and just coded it like this to get the feature running :)

Interact is actually an interesting idea to make Game Engine API to work, but the idea of passing keyboard state is wrong. _If_ we go that direction, it should be notifier/event based system, but even making it to work properly in a clean way is quite a project on it's own -- you'll have to design a system to pass stuff via Python API easier, avoiding hardcoded values all over the place and so. And last but not least -- you'll need to re-implement keymap handling on the other side of API.

Now, Cycles is a render engine and should stay like that. Meaning, it's only doing what the actual application is asking it to do, in this case leaving all the event and keymap handling to Blender.

Lapineige added a subscriber: Lapineige.Jan 19 2016, 12:37 AM

Lapineige awarded a token.Jan 19 2016, 12:40 AM

Adam T W (Thordwilk) added a subscriber: Adam T W (Thordwilk).Jan 20 2016, 5:48 PM

l would love to have this when working on fur and hair. Just being able to specify those niggling spots on a render instead of having to render a whole chunk of the head would save me a boat load of time when reworking a model.

Valery Siamionau (Saiman) awarded a token.Jan 20 2016, 7:30 PM

Wow. That's what I was waiting for and wrote about Sergey Sharybin. How soon it will be release?

Guys, this a code review area for the developers to collaborate. I understand your excitement, but it also complicates following the actual review process.

Lukas, on the second thought, why its a part of render operator? We can have a dedicated operator for that (similar to set white/black point, for the eaae of copy-paste :). Benefits:

Easier for you not to deal with modal keymap etc
Still has configurable shortcut
Has potentioal of being used before the render to change "focus" (still think we should avoid calling feature like this)
Should avoid your context area tricks you're doing.