Details

Reviewers

Brecht Van Lommel (brecht)
Thomas Dinges (dingto)
Sergey Sharybin (sergey)
Lukas Stockner (lukasstockner97)

Commits

rC04a0c534a8ee: distance culling for objects
rB59af4562dde6: Cycles: distance culling for objects.
rBSe8641d44740e: Cycles: distance culling for objects.
rCa2dc5246b763: distance culling for objects
rBe8641d44740e: Cycles: distance culling for objects.

Summary

Hi all,

So I have created a patch to cull off objects based off of distance from camera. Most of my code has been based off the camera culling part.

How is this useful? well, we have some production scenes at the moment, but we can't use camera culling on a lot of objects due to reflections... with camera culling & distance culling combined, we have a minimum distance that the objects are visible in (which is defined in the simplify panel).

It is also useful for small objects to be culled off when far from camera as well.

If there are any questions just let me know and I will try to address them as soon as possible. I apologize in advance if my code isn't up to scratch, I am quite new to the cycles kernel.

Carlo

Diff Detail

Repository: rC Cycles

Event Timeline

Carlo Andreacchio (candreacchio) updated this revision to Diff 7731.Nov 5 2016, 12:50 PM

Carlo Andreacchio (candreacchio) retitled this revision from to Cycles - Distance Culling.

Carlo Andreacchio (candreacchio) updated this object.

Carlo Andreacchio (candreacchio) added reviewers: Brecht Van Lommel (brecht), Sergey Sharybin (sergey), Thomas Dinges (dingto), Lukas Stockner (lukasstockner97).

Carlo Andreacchio (candreacchio) set the repository for this revision to rC Cycles.

Hi, thanks for submitting this.

It seems to work for me. The only real possible thing I see is that adding every kind of culling to sync_object's arguments could start to look unclean at some point.

mathieu menuet (bliblubli) awarded a token.Nov 5 2016, 6:06 PM

The code is getting a bit messy with all these options. I wouldn't mind have a BlenderCulling helper class that contains the properties and utility functions for culling.

intern/cycles/blender/blender_object.cpp
291–296	For clarity and speed I suggest: float3 camera_position = transform_get_column(scene->camera->matrix, 3); float3 position = transform_get_column(tfm, 3); return (len_squared(camera_position - position) > margin * margin); However, wouldn't it be better to cull based on the distance from the camera position to the object bounding box, rather than the object center? Just looking at the center will fail for example with a large ground plane with a center far away from the camera position. Maybe even better would be the distance from the object bounding box to the camera frustum, though that math is more complicated.
345–356	For performance reasons we should only call the `object_*_clip()` functions if culling is actually enabled. So perhaps: bool camera_culled = use_camera_cull && object_boundbox_clip(scene, b_ob, tfm, camera_cull_margin); bool distance_culled = use_distance_cull && object_distance_clip(scene, tfm, distance_cull_margin); if ((camera_culled && distance_culled) \|\| (camera_culled && !use_distance_cull) \|\| (distance_culled && !use_camera_cull)) { return NULL; }

This revision now requires changes to proceed.Nov 5 2016, 9:35 PM

Thanks for the review Brecht,

I have updated the code to now measure the distance to the bounding box, I forgot about that usecase.

Currently I am unsure if I am manipulating the bounding box correctly to account for object scale (nor am I sure if I am using the right function to get the scale parameters of the object itself). Any direction on this area would be much appreciated.

I am also not taking into account the camera frustum for the distance calculations. That may be a bit too advanced for my coding skills

As always, any feedback would be much appreciated!

Carlo

It would be possible to get rid of one of the options by disabling distance culling when the cull distance is set to zero, as it's already done for some options.

In D2332#54147, @Lukas Stockner (lukasstockner97) wrote:

It would be possible to get rid of one of the options by disabling distance culling when the cull distance is set to zero, as it's already done for some options.

I wouldn't mind keeping it just for symmetry with the other option, but don't have a strong opinion either way.

intern/cycles/blender/addon/properties.py
1025–1029	I'm wondering if we need this per-object option, wouldn't it be enough to have a single option to control if the object should be culled by either camera frustum or camera distance?
intern/cycles/blender/blender_object.cpp
302–304	This should be the same as in `object_boundbox_clip`, and not just use the object scale but the entire transform: float3 p = make_float3(boundbox[3 * i + 0], boundbox[3 * i + 1], boundbox[3 * i + 2]); p = transform_point(&tfm, p);
309	Nitpick: use underscore `closest_point` style for variable names.

This revision now requires changes to proceed.Nov 6 2016, 1:52 AM

Addressed Lukas note - When distance_cull_margin == 0 it disables distance culling
Addressed Brechts notes - bounding box points are now transformed correctly, styling changes as well
Brought back code which was removed initially ( camera_cull_margin was not being fetched from the scene)

Right now I have left it as two separate controls, as I feel like this would allow the artists / TDs to decide which to use and when... I can remove this if needed but in my opinion it would be more ideal to have this control.

If there are any other changes please let me know and I will address them swiftly

Carlo

intern/cycles/blender/addon/properties.py
1025–1029	I was thinking about this when I was making the patch... Thinking about real world projects, there are some objects which I would want to cull away when they are further than 50m or so, whether in camera view or not (small objects which would take up less then a pixel at that range). I can change this if needed, but it would be nice on really massive scenes to have this flexibility.

Looks good to me now. I might refactor the code a bit afterwards to put this into a helper class.

Maybe @Sergey Sharybin (sergey) has some feedback still since he added the original culling feature, if not I can commit this.

intern/cycles/blender/blender_object.cpp
603	Nitpick: `){` should be `) {`.

This revision is now accepted and ready to land.Nov 6 2016, 1:59 PM

Styling Notes, let me know if there's anything else I can do!

Just wondering if there is anything else needed from myself to get this into the master branch?

No, I was just waiting to see if there was any feedback from Sergey, but if there is none I'll commit this soon.

Generally fine, but see the comment.

intern/cycles/blender/blender_object.cpp
348	This logic is a bit unclear to me. It'll be less clear if/when we add more culling optimizations. Wouldn't simple `if(camera_culled \|\| distance_culled)` be more predictable and easy to follow?

Hi Sergey,

Thanks for the comment... My reasoning behind this is so that when both options are enabled, that the object remains active for a minimum distance regardless if it is within the camera's view. This way it will still impact the scene with indirect lighting & reflections, but as soon as it drops away outside the certain distance & is not in the camera's field of view, it gets culled out.

We tried recently to increase the number of types of objects we cull out of our scenes, and the biggest problem was popping reflections & indirect lighting. This would fix the problem entirely.

There are also objects which I would definitely like to cull away based off distance alone, regardless of if they are in camera view or not (small objects quite a distance away taking up less then a pixel), which is why I have left it as two separate controls.

I am unsure if there is a cleaner method to this if statement, or if this would be fine with the direction of cycles/blender, any advice would be appreciated.

Closed by commit rBe8641d44740e: Cycles: distance culling for objects. (authored by Carlo Andreacchio (candreacchio), committed by Brecht Van Lommel (brecht)). · Explain WhyNov 20 2016, 3:48 PM

This revision was automatically updated to reflect the committed changes.

Cycles - Distance Culling
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 7744

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_object.cpp

intern/cycles/blender/blender_sync.h

Cycles - Distance CullingClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 7744

intern/cycles/blender/addon/properties.py

intern/cycles/blender/addon/ui.py

intern/cycles/blender/blender_object.cpp

intern/cycles/blender/blender_sync.h

Cycles - Distance Culling
ClosedPublic
Actions

Revision Contents
Changeset List