Multiview: Cycles Spherical Stereo support
ClosedPublic

Authored by Dalai Felinto (dfelinto) on Apr 6 2015, 6:11 PM.

Diff Detail

Repository
rB Blender
There are a very large number of changes, so older changes are hidden. Show Older Changes
intern/cycles/blender/blender_camera.cpp
114–117

Think it's more clear to pass bcam and access to bcam->use_spherical_stereo. So if shift/matrix will ever depend on more settings you don't need to pass them

122

Same as above.

131

Same.

137

Use RenderSettings::view_format_STEREO_3D instead of 0.

514

That's a bit weird:

  • Cycles sets current view to the render engine, so in theory it knows which view to use without querying the engine
  • View is set from the render loop
  • Camera is sync before render loop

So seems would have some redundant updates.

516

Would it make sense to have some sort of view.type which could be {VIEW_LEFT, VIEW_RIGHT, VIEW_CUSTOM}?

524

That' wrong. Camera is to be tagged for update only if it was modified, which is already checked in blender_camera_sync.

So my guess this code belongs to other place of the file and no force tag is needed.

intern/cycles/render/camera.cpp
77

Camera in volume check is to be adopted to this changes by the looks of it.

This revision now requires changes to proceed.Apr 9 2015, 3:57 PM

Have some mixed feelings about this change.

Even keeping code aside, did i get it right that using stereo 3d implies using spherical mapping? If so,why it's nt a panorama type?

Leaving code aside for now.

Why it's not a panorama type?

This is a method that can be used for Fisheye renders as well, so to have it as its own 'panorama' type would imply on having "equirectangular / equirectangular omnidirectional / fisheye / fisheye omnidirectional" which (as far as UI goes) is not ideal.

Did i get it right that using stereo 3d implies using spherical mapping?

Not really. If someone is mixing real captured fisheye stereo footage with rendered fisheye renders the traditional method is required. If someone is doing 100% fisheye/panorama render than the present method is preferable.

  • Merge remote-tracking branch 'origin/master' into spherical-stereo
  • From review: use RenderSettings::views_format_STEREO_3D
  • From review: remove forced tag, and uneeded camera_sync and code rearrangement

The only missing part is the use of bcam instead of use_spherical_stereo and the 'camera in volume' (whatever that is, I have to test this case)

  • From review: use RenderSettings::views_format_STEREO_3D
  • From review: remove forced tag, and uneeded camera_sync and code rearrangement
  • Merge remote-tracking branch 'origin/master' into spherical-stereo
  • Merge remote-tracking branch 'origin/master' into multiview-spherical-stereo
  • From review: Camera in volume check adapted to those changes

@Sergey Sharybin (sergey), is that what you had in mind?
The camera bounding box in this case is a cylinder of radius interocular_distance/2

  • Merge remote-tracking branch 'origin/master' into spherical-stereo

(just updating the patch against master in case we give up on the idea of lens nodes and go for this patch instead :)

  • Fix building after merge
This revision was automatically updated to reflect the committed changes.

Phabricator is too smart for its own good, re-opening the automatically closed patch (due to commit in the experimental-build branch)

intern/cycles/blender/blender_camera.cpp
137

done

  • Merge remote-tracking branch 'origin/master' into spherical-stereo

(AUDASPACE building fix included, so I can trigger another builder bot build)

This revision was automatically updated to reflect the committed changes.

Once again, re-opening, and now with links for fresh builds:

  • Merge remote-tracking branch 'origin/master' into spherical-stereo
  • this includes the fix for bump map differentials (T45721)
  • Fix for bump maps not working well with spherical-stereo
  • Ultimate fix for differentials (thanks @Brecht Van Lommel (brecht))
  • Merge remote-tracking branch 'origin/master' into spherical-stereo
  • Fix for Left and Right views flipped - reported by Sebastian Koenig

Sharing comments done form debalie review session.

intern/cycles/blender/blender_camera.cpp
116

Guess this is because b_engine.camera_shift_x applies offset for an eye? Worth mentioning this in the comment.

125

Seems phab got confused here with comments..

Still think we can make more clear Py-API for such things, but at least please bother explaining why it's special case here as well ;)

412

Think we can avoid exposing eyes to the underlying camera classes. See comments below in the kernel.

intern/cycles/kernel/kernel_camera.h
290

If eye is STEREO_NONE then panorama_stereo_position will give same position as camera and all this code seems to be same as the case above and we can avoid this duplication?

intern/cycles/kernel/kernel_projection.h
236

Think we don't need such switch here at all.

kernel_data.cam.interocular_distance == 0 equals to STEREO_NONE
kernel_data.cam.interocular_distance < 0 equals to STEREO_LEFT
kernel_data.cam.interocular_distance > 0 equals to STEREO_RIGHT

Think with some good naming of variables and comments around them we can keep kernel simplier and more uniform for all the possible mappings.

intern/cycles/render/camera.cpp
501–525

Space before {

Dalai Felinto (dfelinto) marked 2 inline comments as done.
  • Rename panorama_stereo_direction > spherical_stereo_direction
  • Spherical Stereo support for perspective cameras
  • Turning spherical stereo off by default
  • UI: expose spherical-stereo for perspective camera as well
  • From review: style cleanup
  • From review: remove duplicated code
  • From review: remove switch by cleverly defining StereoEye enum
  • Move use_spherical_stereo from cycles to blender (external_engine)
  • Left-over from code duplication
  • Let the Render API to handle the spherical stereo special cases

@Campbell Barton (campbellbarton) could you take a look at those changes in the render API too?

This is a confusing logic. First of all, you define render engine to have bl_use_spherical_stereo, and then in the engine itself you call use_spherical_stereo() which not only checks camera settings but also does render engine verification. This is totally redundant.

The only case where such flag might do difference is in the camera shift/matrix calculation, so enabling spherical stereo doesn't affect other engines. But even then, wouldn't it make more sense to pass a boolean argument to those function about whether you're interested in spherical case or regular one? Could simplify code for the Cycles i think:

bcam->use_spherical = b_engine.use_spherical_stereo();
bcam->matrix = b_engine.camera_matrix(bcam->use_spherical)

Or something like that. Just trying to simplify API and make it more clear.

And i'm still not sure why not to get rid of StereoEye all together by just multiplying distance according to the eye? You can call it interocular_offset. You can calculate this offset in Camera's update_device() and tag camera for the device once the view changed.

source/blender/makesrna/intern/rna_render.c
547

You're calling boolean and function the same way by the looks of it?

  • From review: change render api in regard to spherical stereo
  • From review: pre-calculate interocular offset

Alright, a new update coming. I didn't have time to test it (and won't until Tuesday), so there is a chance left and right eyes are flipped. Apart from that it is now close to what you suggested (I still have stereo_eye around, but it's outside the kernel).

And I'm no longer passing the interocular_distance directly to the kernel, but I'm instead pre-processing it to account for the stereo eye.

intern/cycles/kernel/kernel_camera.h
290

Correct, and this is what I had in a previous patch. I just tried to make the changes less intrusives (and with less impact on general performance for the non-spherical stereo use case). I will remove this, then.

intern/cycles/kernel/kernel_projection.h
236

sure, I will do it

source/blender/makesrna/intern/rna_render.c
547

One is the return parameter, the other the name of the function. Why is that a problem?

  • Fix UI (reported by Sebastian Koenig, thanks)

Hi,
are there any new builds available? The last build I found is 2.74-d33764b from this article: https://code.blender.org/2015/03/1451/ (which is no longer available). The problem with this build is that it outputs very noisy renders compared to standard panorama made in recent 2.76b (official).

Here is an image for comparison: The left is made with the recent 2.76b and on the right is the 2.74-d33764b with stereo panorama enabled.

@Marek Moravec (marek) you can find more recent builds at: http://www.dalaifelinto.com/?p=1009 (see the link on the post). See if the same issue is there with those builds.

@Dalai Felinto (dfelinto) Thank you very much! The recent build renders just fine. I found a bug with border rendering but I guess it is better to fill a separate bug report. Anyway, thank you for your work on this!

  • Merge remote-tracking branch 'origin/master' into spherical-stereo
  • Merge remote-tracking branch 'origin/master' into spherical-stereo
  • Small cleanup

Looks rather good to me now. Some further cleanup is possible, but could happen after the commit as well.

Some notes tho:

  • Seems Spherical Stereo is only used when camera is panoramic. In this case it makes sense to hide the option for perspective camera.
  • Not sure we need bl_use_spherical_stereo, render engnes can simply hide options from the interface and pass false to the render engine when they need to calculate matrix.

Worth poking Rambo to review rna art of the changes.

This revision is now accepted and ready to land.Mar 7 2016, 12:04 PM

Looks good, minor note about comments.

intern/cycles/kernel/kernel_projection.h
229

I think this line is not needed. Comment beneath is sufficient. Also please start comment with uppercase Letter and fullstop at the end. That is inconsistent in Cycles atm, but I want to change this soon.

242

Same as above. Upper case + Fullstop.

@Sergey Sharybin (sergey), spherical stereo is also used when rendering cubemaps, not just panoramas. so it does make sense to have it :)

This comment was removed by Sebastian Koenig (sebastian_k).
source/blender/blenkernel/intern/camera.c
861

Shouldn't this be SCE_VIEWS_FORMAT_STEREO_3D ?

intern/cycles/kernel/kernel_camera.h
118–133

Interesting, so derivatives can't be pre-calculated even for perspective camera when spherical 3D is used?

If so, i think it worth sticking to a single method as it is currently (don't think it's a bottleneck, having single code base is easier for maintenance) and remove derivatived pre-calculation from camera.cpp.

Other notes:

  • Some values are calculated twice (i.e. tP here is in fact Pcamera)
  • Rolling shutter needs to be taken into a care here?

P.S. Or maybe there is a trick to pre-calculate derivated for stereo 3d and perspective camera..

291

This chunk is same as above apart from panorama_to_direction part.

Thinking loud: would it be more clear to move this to an utility function with an argument denoting whether derivatives are calculated for panorama camera?

This revision was automatically updated to reflect the committed changes.