Details

Reviewers

Jeroen Bakker (jbakker)
Jacques Lucke (JacquesLucke)

Commits

rB65ad36f5fd71: DRWManager: New implementation.

Summary

This is a new implementation of the draw manager using modern
rendering practices and GPU driven culling.

This only ports features that are not considered deprecated or to be
removed.

The old DRW API is kept working along side this new one, and does not
interfeer with it. However this needed some more hacking inside the
draw_view_lib.glsl. At least the create info are well separated.

The reviewer might start by looking at draw_pass_test.cc to see the
API in usage.

Important files are draw_pass.hh, draw_command.hh,
draw_command_shared.hh.

In a nutshell (for a developper used to old DRW API):

DRWShadingGroups are replaced by Pass<T>::Sub.
Contrary to DRWShadingGroups, all commands recorded inside a pass or sub-pass (even binds / push_constant / uniforms) will be executed in order.
All memory is managed per object (except for Sub-Pass which are managed by their parent pass) and not from draw manager pools. So passes "can" potentially be recorded once and submitted multiple time (but this is not really encouraged for now). The only implicit link is between resource lifetime and ResourceHandles
Sub passes can be any level deep.
IMPORTANT: All state propagate from sub pass to subpass. There is no state stack concept anymore. Ensure the correct render state is set before drawing anything using Pass::state_set().
The drawcalls now needs a ResourceHandle instead of an Object *. This is to remove any implicit dependency between Pass and Manager. This was a huge problem in old implementation since the manager did not know what to pull from the object. Now it is explicitly requested by the engine.
The pases need to be submitted to a draw::Manager instance which can be retrieved using DRW_manager_get() (for now).

Internally:

All object data are stored in contiguous storage buffers. Removing a lot of complexity in the pass submission.
Draw calls are sorted and visibility tested on GPU. Making more modern culling and better instancing usage possible in the future.
Unit Tests have been added for regression testing and avoid most API breakage.
draw::View now contains culling data for all objects in the scene allowing caching for multiple views.
Bounding box and sphere final setup is moved to GPU.
Some global resources locations have been hardcoded to reduce complexity.

What is missing:

~~Workaround for lack of gl_BaseInstanceARB.~~ Done
~~Object Uniform Attributes.~~ Done (Not in this patch)
Workaround for hardware supporting a maximum of 8 SSBO.

Diff Detail

Repository: rB Blender

Event Timeline

Clément Foucault (fclem) requested review of this revision.Aug 31 2022, 12:22 AM

Clément Foucault (fclem) created this revision.

Harbormaster completed remote builds in B23549: Diff 55130.Aug 31 2022, 12:22 AM

Adding @Jacques Lucke (JacquesLucke) as he is interested in the C++ implementation details.

Clément Foucault (fclem) edited the summary of this revision. (Show Details)Aug 31 2022, 12:31 AM

I don't have much understanding of any of this stuff, but I noticed that ObjectBounds::sync has an Object input and uses BKE_object_boundbox_get.
Considering the changes planned in T96968, maybe it would make more sense to retrieve on the geometry level, since this is a new API.
Retrieving local-space bounds on the object level will always be a bit weird because objects can contain so many things.

I don't know how much of this is "new" vs. just a port of the old system. Feel free to just say "not now."

source/blender/draw/intern/draw_pass.hh
196	Standard syntax is `\note` in Blender code
source/blender/draw/intern/draw_resource.hh
105	Recently I've seen a consensus that we should really avoid C style casts in C++ code, and use `static_cast` wherever possible. That makes a lot of sense to me too. It's just so easy to cast away const this way, and not obvious enough.

From draw engine point of view seems fine. (For readers: adding this for review was discussed during the last EEVEE/Viewport meeting to get a sense of what the changes are)
As the changes are almost isolated and changes are expected when the system is actually used (EEVEE-Next) I don't see an issue on landing this.
The API is much nicer from draw engine point of view. Impact on the shader development and debugging in general is something to look into in the upcoming period and fine-tune where needed.

source/blender/draw/intern/draw_defines.h
3	21?
source/blender/draw/intern/draw_pass.hh
380
source/blender/draw/intern/draw_resource.hh
81	In this case I would suggest to store the cryptomatte hash in ob->runtime as it hashing already fixes some common errors (ignore NAN/INF etc) When rendering cryptomatte this doesn't need to happen every sample, so combining it with this solution would benefit performance in other areas as well.
105	I agree. C style hides to many casting issues.
source/blender/draw/intern/draw_shader_shared.h
69	I dislike comments on after definition. especially when they are doc comments.
source/blender/draw/intern/shaders/draw_command_generate_comp.glsl
57	Hmm... atomics are faster when they don't share a cache line. DrawGroup seems to be 48 bytes so it could technically atomic as a change of locking multiple groups. We should check if there are performance gains by adding some additional padding to the DrawGroup or use a different approach.

Only got some code style comments. Can't comment much on the general design of the api.

source/blender/draw/intern/draw_pass.hh
87	Is it intentional that only r-value-references can be passed into this method? If yes, then it doesn't really make sense that `std::move` isn't used in `blocks_.last()->append_and_get_index`.
96	Assert that index is in valid range.
611	I think it would be better to prefer using `this->` when calling methods of the same class. It's just more explicit.
source/blender/draw/intern/shaders/draw_command_generate_comp.glsl
85	missing newline

Fix incorrect copyright date
Add workaround for hardware without GPU_shader_draw_parameters_support
Merge branch 'master' into tmp-drw-next-commit
Implement workaround to lack of gl_BaseInstance

Harbormaster completed remote builds in B23551: Diff 55139.Aug 31 2022, 12:41 PM

Fix review comments
Move debug offset constants to make it harder to go out of sync.

Harbormaster completed remote builds in B23554: Diff 55149.Aug 31 2022, 4:51 PM

Merge branch 'master' into tmp-drw-next-commit
Fix debug draw shader

Harbormaster completed remote builds in B23575: Diff 55200.Sep 1 2022, 6:46 PM

Clément Foucault (fclem) edited the summary of this revision. (Show Details)Sep 1 2022, 6:48 PM

Jeroen Bakker (jbakker) accepted this revision.Sep 2 2022, 12:11 PM

This revision is now accepted and ready to land.Sep 2 2022, 12:11 PM

Closed by commit rB65ad36f5fd71: DRWManager: New implementation. (authored by Clément Foucault (fclem)). · Explain WhySep 2 2022, 6:45 PM

This revision was automatically updated to reflect the committed changes.

Clément Foucault (fclem) added a commit: rB65ad36f5fd71: DRWManager: New implementation..

Miguel Pozo (pragma37) mentioned this in T101619: Workbench rewrite.Oct 5 2022, 3:51 PM

Paul Larson (GeorgiaPacific) added a subscriber: Paul Larson (GeorgiaPacific).Nov 8 2022, 3:54 AM

Fran Marz (franMarz) awarded a token.Sun, Jan 29, 3:06 PM

DRWManager: New implementation.ClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 55261

release/scripts/addons

release/scripts/startup/bl_ui/space_userpref.py

release/scripts/startup/bl_ui/space_view3d.py

source/blender/draw/CMakeLists.txt

source/blender/draw/intern/DRW_gpu_wrapper.hh

source/blender/draw/intern/DRW_render.h

source/blender/draw/intern/draw_command.hh

source/blender/draw/intern/draw_command.cc

source/blender/draw/intern/draw_command_shared.hh

source/blender/draw/intern/draw_common_shader_shared.h

source/blender/draw/intern/draw_debug.cc

source/blender/draw/intern/draw_defines.h

source/blender/draw/intern/draw_handle.hh

source/blender/draw/intern/draw_manager.h

source/blender/draw/intern/draw_manager.hh

source/blender/draw/intern/draw_manager.c

source/blender/draw/intern/draw_manager.cc

source/blender/draw/intern/draw_pass.hh

source/blender/draw/intern/draw_resource.hh

source/blender/draw/intern/draw_shader.h

source/blender/draw/intern/draw_shader.cc

source/blender/draw/intern/draw_shader_shared.h

source/blender/draw/intern/draw_state.h

source/blender/draw/intern/draw_view.hh

source/blender/draw/intern/draw_view.cc

source/blender/draw/intern/draw_view_data.cc

source/blender/draw/intern/shaders/common_debug_draw_lib.glsl

source/blender/draw/intern/shaders/common_debug_print_lib.glsl

source/blender/draw/intern/shaders/common_intersect_lib.glsl

source/blender/draw/intern/shaders/common_view_lib.glsl

source/blender/draw/intern/shaders/draw_command_generate_comp.glsl

source/blender/draw/intern/shaders/draw_debug_draw_display_vert.glsl

source/blender/draw/intern/shaders/draw_debug_info.hh

source/blender/draw/intern/shaders/draw_debug_print_display_vert.glsl

source/blender/draw/intern/shaders/draw_object_infos_info.hh

source/blender/draw/intern/shaders/draw_resource_finalize_comp.glsl

source/blender/draw/intern/shaders/draw_view_info.hh

source/blender/draw/intern/shaders/draw_visibility_comp.glsl

source/blender/draw/tests/draw_pass_test.cc

source/blender/gpu/CMakeLists.txt

source/blender/gpu/GPU_batch.h

source/blender/gpu/intern/gpu_batch.cc

source/blender/gpu/intern/gpu_shader_create_info.cc

source/blender/gpu/opengl/gl_vertex_array.cc

source/blender/makesdna/DNA_userdef_types.h

source/blender/makesdna/DNA_view3d_types.h

source/blender/makesrna/intern/rna_space.c

source/blender/makesrna/intern/rna_userdef.c

DRWManager: New implementation.
ClosedPublic
Actions

Revision Contents
Changeset List