Page MenuHome
Differential D3011

Cycles: Replace use_qbvh boolean flag with an enum-based property
ClosedPublic
Actions

Authored by Sergey Sharybin (sergey) on Jan 19 2018, 11:04 AM.

Details

Reviewers
Brecht Van Lommel (brecht)
Max (maxim_d33)
Commits
rBS2f79d1c0584f: Cycles: Replace use_qbvh boolean flag with an enum-based property
rCd3210d3d294b: Replace use_qbvh boolean flag with an enum-based property
rB2f79d1c0584f: Cycles: Replace use_qbvh boolean flag with an enum-based property
Summary

This was we can introduce other types of BVH, for example, wider ones, without
causing too much mess around boolean flags.

Thoughs:

  • Ideally device info should probably return bitflag of what BVH types it supports.

    It is possible to implement based on simple logic in device/ and mesh.cpp, rest of the changes will stay the same.
  • Not happy with workarounds in util_debug and duplicated enum in kernel. Maybe enbum should be stores in kernel, but then it's kind of weird to include kernel types from utils. Soudns some cyclkic dependency.

Diff Detail

Repository
rB Blender
Branch
cycles_bvh_enum
Build Status
Buildable 1104
Build 1104: arc lint + arc unit

Event Timeline

Sergey Sharybin (sergey) created this revision.Jan 19 2018, 11:04 AM
Harbormaster completed remote builds in B1103: Diff 9852.Jan 19 2018, 11:04 AM
Brecht Van Lommel (brecht) added a comment.Jan 19 2018, 2:39 PM

Maybe call it BVH layout instead of BVH size? Makes a bit more sense if we ever add more variations.

About duplicated enums and dependencies, I think DebugFlags belongs in the render/ folder really. util/ was meant to be for generic types and functions,

intern/cycles/bvh/bvh_params.h
58–64

I would move this out of the BVHParams namespace. If needs to be in the namespace BVHSize should just be Size.

Sergey Sharybin (sergey) added a comment.Jan 19 2018, 3:14 PM

Maybe call it BVH layout instead of BVH size? Makes a bit more sense if we ever add more variations.

That sounds like a better name indeed.

About duplicated enums and dependencies, I think DebugFlags belongs in the render/ folder really. util/ was meant to be for generic types and functions,

DebugFlags() are used by device code, and it seemed to be a bad level include to include render/ from device. If you think it's not that bad level, i don't mind moving debug stuff from util to render at all.

intern/cycles/bvh/bvh_params.h
58–64

For the host code it's kind of optimal to have it a class enum inside of ccl itself. But we aren't forcing C++11 for now, and not sure having BVHSize class with enum inside is a good idea,

Sergey Sharybin (sergey) updated this revision to Diff 9855.Jan 19 2018, 4:44 PM
  • Renamed bvh_size to bvh_layout
  • Removed duplicated enums in bvh.h and kernel_types.h
  • Removed workaround in util_debug.h
  • Made it a bit mask returned by device info, so we should be more future proof now.
  • Made it BVH_LAYOUT_DEFAULT rather than "widest". This way we can add wider BVH implementations but not use them for production files yet.
Harbormaster completed remote builds in B1104: Diff 9855.Jan 19 2018, 4:44 PM
Sergey Sharybin (sergey) added a reviewer: Max (maxim_d33).Jan 19 2018, 4:45 PM
Brecht Van Lommel (brecht) accepted this revision.Jan 19 2018, 8:06 PM

Looks good, though could be simplified a little imo.

intern/cycles/kernel/kernel_types.h
1347

BVHSize -> BVHLayoutMaskBits.

I would just define BVH_LAYOUT_BVH2 and BVH_LAYOUT_BVH4 directly as (1 << 0) and (1 << 1) though, no need for the duplication.

This revision is now accepted and ready to land.Jan 19 2018, 8:06 PM
Sergey Sharybin (sergey) updated this revision to Diff 9865.Jan 22 2018, 4:41 PM

Remove duplicated enum values

Harbormaster completed remote builds in B1110: Diff 9865.Jan 22 2018, 4:41 PM
Sergey Sharybin (sergey) updated this revision to Diff 9866.Jan 22 2018, 4:46 PM

Forgot this in previous update

Brecht Van Lommel (brecht) added a comment.EditedJan 22 2018, 4:47 PM

Ready to commit I think, except that some stuff can be removed still before you do.

intern/cycles/bvh/bvh.cpp
37–44

This can be removed now.

58–66

This can be removed now.

Brecht Van Lommel (brecht) accepted this revision.Jan 22 2018, 4:48 PM

:)

Closed by commit rB2f79d1c0584f: Cycles: Replace use_qbvh boolean flag with an enum-based property (authored by Sergey Sharybin (sergey)). · Explain WhyJan 22 2018, 5:22 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
intern/
cycles/
blender/
addon/
11 lines
2 lines
2 lines
2 lines
bvh/
70 lines
2 lines
44 lines
device/
20 lines
4 lines
5 lines
2 lines
2 lines
2 lines
kernel/
bvh/
33 lines
34 lines
48 lines
26 lines
30 lines
10 lines
render/
14 lines
36 lines
util/
10 lines
20 lines

Diff 9855

View Options

intern/cycles/blender/addon/properties.py

Loading...
View Options

intern/cycles/blender/addon/ui.py

Loading...
View Options

intern/cycles/blender/blender_python.cpp

Loading...
View Options

intern/cycles/blender/blender_sync.cpp

Loading...
View Options

intern/cycles/bvh/bvh.cpp

Loading...
View Options

intern/cycles/bvh/bvh4.cpp

Loading...
View Options

intern/cycles/bvh/bvh_params.h

Loading...
View Options

intern/cycles/device/device.h

Loading...
View Options

intern/cycles/device/device.cpp

Loading...
View Options

intern/cycles/device/device_cpu.cpp

Loading...
View Options

intern/cycles/device/device_cuda.cpp

Loading...
View Options

intern/cycles/device/device_network.cpp

Loading...
View Options

intern/cycles/device/device_opencl.cpp

Loading...
View Options

intern/cycles/kernel/bvh/bvh_local.h

Loading...
View Options

intern/cycles/kernel/bvh/bvh_shadow_all.h

Loading...
View Options

intern/cycles/kernel/bvh/bvh_traversal.h

Loading...
View Options

intern/cycles/kernel/bvh/bvh_volume.h

Loading...
View Options

intern/cycles/kernel/bvh/bvh_volume_all.h

Loading...
View Options

intern/cycles/kernel/kernel_types.h

Loading...
View Options

intern/cycles/render/mesh.cpp

Loading...
View Options

intern/cycles/render/scene.h

Loading...
View Options

intern/cycles/util/util_debug.h

Loading...
View Options

intern/cycles/util/util_debug.cpp

Loading...