Cycles: Adding field-of-view options to the equirectangular panorama camera
ClosedPublic
Actions

Authored by Lukas Stockner (lukasstockner97) on Dec 30 2014, 3:06 PM.

Details

Reviewers

Sergey Sharybin (sergey)

Group Reviewers

Cycles

Commits

rBS4118c1b4e6ae: Cycles: Adding field-of-view options to the equirectangular panorama camera
rC42fdfb4bc5f2: Adding field-of-view options to the equirectangular panorama camera
rB4118c1b4e6ae: Cycles: Adding field-of-view options to the equirectangular panorama camera

Summary

This patch adds the option to set minimum/maximum latitude/longitude values for the equirectangular panorama camera in Cycles, as discussed in T34400.
The separate functions in kernel_projection.h are needed because the regular ones are also used as helper functions for environment map sampling.

Diff Detail

Repository: rB Blender

Event Timeline

Lukas Stockner (lukasstockner97) updated this revision to Diff 3071.Dec 30 2014, 3:06 PM

Lukas Stockner (lukasstockner97) retitled this revision from to Cycles: Adding field-of-view options to the equirectangular panorama camera.

Lukas Stockner (lukasstockner97) updated this object.

Lukas Stockner (lukasstockner97) added a reviewer: Cycles.

Looks good to me, thanks for picking this up. I'd personally use slightly different property names.

intern/cycles/blender/addon/properties.py
585	I'd name these latitude_min, longitude_min, .. since we generally don't use abbreviations like equirect or lat in variable names, and the full thing is indeed a bit long.

I agree with @Brecht Van Lommel (brecht) about naming. Sme qustions tho:

What about adding small comment around the new variables about which units they're measured in? (seems they're different in UI and kernel, which might be a bit confusing)
Is there measurable slowdown of background samlping when using single version of the projection functions?

intern/cycles/kernel/kernel_projection.h
77	Picky: I would just use `_range` as a suffix name (to avoid camera-specific naming since it might be useful in general).

The variables are now renamed to latitude_min etc.
I removed the two separate versions. The primary reason wasn't speed, but having short and nice code, the #define used now should do this as well (and the compiler should be able to optimize it out)
Regarding conversion: There's no real unit change going on, it's just designed to have short kernel code while producing the same result. I made it quite a lot easier and cleaner in this version, but I'm not sure how to explain it in a comment, in the end it's just precomputation of the terms needed for projection.

Some minor note about misunderstanding of how we can de-duplicate projection logic in there. Apart from that i think it's ready to go.

intern/cycles/kernel/kernel_projection.h

You're too fast with this. What i thought is if there's no speed regressions (because compiler could substitude the constant values to the expression) is to have something more like:

ccl_device_inline float2 direction_to_equirectangular(float3 dir)
{
  return direction_to_equirectangular_range(dir,
      make_float4(-M_2_PI_F, M_PI_F, -M_PI_F, M_PI_F))
}

ccl_device float2 direction_to_equirectangular_range(float3 dir, float4 range)
{
  /* ... */
}

Yes, that's definitely better.
By the way, thanks for the quick review!

Thomas Dinges (dingto) added a subscriber: Thomas Dinges (dingto).Jan 4 2015, 1:45 AM

This comment was removed by Thomas Dinges (dingto).