Restore DebugRenderer #162

zakorgy · 2018-05-03T12:39:35Z

Fixes the third part of #150: restore DebugRenderer.
Added DebugColor and DebugFont shader kinds. These shaders are handled differently when creating programs, because they need index and vertex buffers with relative large sizes (these are the maximum values I got from upstream WebRender), and just one pipeline.
Also replaced draw with draw_indexed to support drawing with these shaders.

At the moment the DebugRenderer draws, but after we reach a certain number of indices/vertices the following vulkan validation layer error shows up:
2018-05-03T12:24:58Z: gfx_backend_vulkan: [MEM] Object: 0x1779beb3980 | Number of currently valid memory objects is not less than the maximum allowed (4096).
I'm not sure if this is caused by the high number of vertices/indices, or I missed something in the implementation.

zakorgy · 2018-05-03T12:56:06Z

Here is an image from the DebugRenderer with the current code:

kvark · 2018-05-03T14:37:30Z

webrender/build.rs

@@ -563,14 +563,14 @@ fn create_vertex_buffer_descriptors(file_name: &str) -> Vec<VertexBufferDesc> {
    } else if file_name.starts_with("debug_color") {
        descriptors = vec![
            VertexBufferDesc {
-                stride: 16, // size of DebugColorVertex 4 * 4
+                stride: 28, // size of DebugColorVertex 3 * 4 + 4 * 4


let's just use mem::size_of directly here?

Unfortunately in the build script we can't access structures from the crate itself.

We can have those structs declared in a separate module, which is naturally included in the crate but manually included in the build.rs script, e.g.

#[path = "src/vertex_types.rs"] mod vertex_types;

kvark · 2018-05-03T14:37:48Z

webrender/src/debug_render.rs

 use debug_font_data;
-use device::{Device, Program, Texture, TextureSlot, VertexDescriptor};
-use device::{TextureFilter, VertexAttribute, VertexAttributeKind};
+use device::{Device, ProgramId, Texture, TextureSlot, /*VertexDescriptor*/};


do we need those commented out entries left?

No, we don't. I will remove them.

kvark · 2018-05-03T14:38:20Z

webrender/src/debug_render.rs

    pub u: f32,
    pub v: f32,
+    s: f32,


why do we need extra data here ? (z, s, t)

Because the debug color vertex shader has the following in parameters:

layout(location = 0) in vec3 aPosition; layout(location = 3) in vec4 aColor; layout(location = 4) in vec4 aColorTexCoord;

aPosition is a 3 element vector but we supply only two value for it x and y.
color is for aColor but contains u8 values so I had to change it to ColorF to match what the shader expects there.
u, v is used for the aColorTexCoord which is also a 4 element vector but we only have two values for it.
For some reason OpenGL knows which value goes where when we uploading these structures to the gpu, but for Vulkan I had to extend DebugFontVertex and DebugFontColor so the tiling of the data is the same as the shader expects.
We could change aColorTexCoord in the upstream debug_color shader to be a vec2 and leave s and t. Because we only use the xy values from it, and other shader doesn't use it.
Unfortunately aPosition comes from prim_shared.glsl so i think we need the z value there.

aPosition is indeed shared with the rest of the shaders, and thus has to have 3 components.

color should still be [u8; 4] on the Rust side and declared as UNORM kind: https://github.com/gfx-rs/gfx/blob/29d30a1254240a1449fc54a7d146b0928642509d/src/hal/src/format.rs#L417

aColorTexCoord should be fixed upstream - servo#2726

dati91

Looks good! Made some comment about it.

Also I just realized that we use the Buffer<B> data_len a bit confusing. We store the len * stride, instead of the actual len. Maybe we can clean up this in another pr.

dati91 · 2018-05-04T12:35:15Z

webrender/src/debug_render.rs

    pub color: ColorU,
 }

 impl DebugColorVertex {
    pub fn new(x: f32, y: f32, color: ColorU) -> DebugColorVertex {
-        DebugColorVertex { x, y, color }
+        DebugColorVertex { x, y, z: 0.0, color, }


nit: remove the last comma.

dati91 · 2018-05-04T12:35:23Z

webrender/src/debug_render.rs

+        // TODO: bind sampler
+        //device.bind_shader_samplers(&font_program, &[("sColor0", DebugSampler::Font)]);
+
+        let pipeline_requirement2 =


nit: pipeline_requirement_color is a bit more descriptive than 2.

dati91 · 2018-05-04T12:45:03Z

webrender/src/device.rs

@@ -1405,6 +1437,12 @@ impl<B: hal::Backend> Program<B> {
            (&self.vertex_buffer.buffer, 0),
            (&self.instance_buffer.buffer.buffer, 0),
        ]));
+        let index_buffer_view = hal::buffer::IndexBufferView {


Can we store this view in the structure? I don't see why should we create it in every draw call.
Also I think You missed this view's deinit, which maybe causing this

2018-05-03T12:24:58Z: gfx_backend_vulkan: [MEM] Object: 0x1779beb3980 | Number of currently valid memory objects is not less than the maximum allowed (4096).

dati91 · 2018-05-04T12:46:36Z

webrender/src/device.rs

@@ -1422,14 +1460,16 @@ impl<B: hal::Backend> Program<B> {
                viewport.rect,
                clear_values,
            );
-            encoder.draw(0 .. 6, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. self.instance_buffer.offset as u32);
+            //encoder.draw(0 .. 6, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. self.instance_buffer.offset as u32);
+            encoder.draw_indexed(0 .. (self.index_buffer.data_len / mem::size_of::<u32>()) as u32, 0, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. cmp::max(self.instance_buffer.offset, 1)as u32);


)as -> ) as

dati91 · 2018-05-04T12:48:50Z

webrender/src/device.rs

@@ -1422,14 +1460,16 @@ impl<B: hal::Backend> Program<B> {
                viewport.rect,
                clear_values,
            );
-            encoder.draw(0 .. 6, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. self.instance_buffer.offset as u32);
+            //encoder.draw(0 .. 6, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. self.instance_buffer.offset as u32);
+            encoder.draw_indexed(0 .. (self.index_buffer.data_len / mem::size_of::<u32>()) as u32, 0, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. cmp::max(self.instance_buffer.offset, 1)as u32);


cmp::max(self.instance_buffer.offset, 1)
You sure we need this max? If we need it, that is very suspicious, maybe worth looking into it.

If we draw with the debug shaders we don't have instances also the instance buffer size and offset is zero, but we have to draw at least once, this requires a 0 .. 1 range for the last parameter. I put the cmp::max here to ensure we have 1 as the upper bound.

dati91 · 2018-05-04T12:49:24Z

webrender/src/device.rs

@@ -1422,14 +1460,16 @@ impl<B: hal::Backend> Program<B> {
                viewport.rect,
                clear_values,
            );
-            encoder.draw(0 .. 6, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. self.instance_buffer.offset as u32);
+            //encoder.draw(0 .. 6, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. self.instance_buffer.offset as u32);


nit: remove the dead code

dati91 · 2018-05-04T12:58:48Z

webrender/src/renderer.rs

@@ -2252,7 +2254,7 @@ impl<B: hal::Backend> Renderer<B> {
                        &mut profile_timers,
                        //&profile_samplers,


I guess enabling the profile_samplers is not that trivial?

Exactly, because profile_samplers uses code which is strongly OpenGL dependent, namely the query modul, which we don't use for the previously mentioned reason. I think this is more relevant to the GpuProfiler part of #150.

dati91 · 2018-05-04T13:04:06Z

webrender/src/device.rs

@@ -1422,14 +1460,16 @@ impl<B: hal::Backend> Program<B> {
                viewport.rect,
                clear_values,
            );
-            encoder.draw(0 .. 6, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. self.instance_buffer.offset as u32);
+            //encoder.draw(0 .. 6, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. self.instance_buffer.offset as u32);
+            encoder.draw_indexed(0 .. (self.index_buffer.data_len / mem::size_of::<u32>()) as u32, 0, (self.instance_buffer.offset - self.instance_buffer.size) as u32 .. cmp::max(self.instance_buffer.offset, 1)as u32);


self.index_buffer.data_len / mem::size_of::<u32>() ->self.index_buffer.data_len / self.index_buffer.data_stride

dati91 · 2018-05-04T13:07:16Z

webrender/src/device.rs

+        assert_ne!(self.bound_program, INVALID_PROGRAM_ID);
+        let program = self.programs.get_mut(&self.bound_program).expect("Program not found.");
+
+        let index_buffer_stride = mem::size_of::<u32>();


You already have this in the index_buffer stored as data_stride.

dati91 · 2018-05-04T13:07:56Z

webrender/src/device.rs

+        assert_ne!(self.bound_program, INVALID_PROGRAM_ID);
+        let program = self.programs.get_mut(&self.bound_program).expect("Program not found.");
+
+        let vertex_buffer_stride = mem::size_of::<T>();


Same as above.

zakorgy · 2018-05-07T12:59:07Z

Looks like removing index buffers from shaders, except for the debug ones, fixes the Vulkan validation layer error I mentioned in the PR description.

zakorgy added the in progress label May 3, 2018

zakorgy requested review from kvark and dati91 May 3, 2018 14:34

kvark reviewed May 3, 2018

View reviewed changes

zakorgy changed the title ~~Restore DebugRenderer [WIP]~~ Restore DebugRenderer May 4, 2018

dati91 reviewed May 4, 2018

View reviewed changes

Zakor Gyula added 7 commits May 7, 2018 14:37

Restore DebugRenderer

29e7df8

Fix warnings

b2fddc7

Decrease aColorTexCoord size

5945724

Place vertex types and instances in a mod

acf5c57

Revert debug vertex colors to use [u8; 4] again

0bb7efe

Fix nits

1cc63b9

Only use indexed drawing for debug shaders

65536a3

zakorgy force-pushed the restore-debug-renderer branch from 9024426 to 65536a3 Compare May 7, 2018 12:52

Remove commented out code

b1ca9ee

zakorgy force-pushed the restore-debug-renderer branch from 5d556fe to b1ca9ee Compare May 7, 2018 13:50

dati91 approved these changes May 8, 2018

View reviewed changes

dati91 merged commit 91552d8 into szeged:master May 8, 2018

dati91 removed the in progress label May 8, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore DebugRenderer #162

Restore DebugRenderer #162

zakorgy commented May 3, 2018

zakorgy commented May 3, 2018

kvark May 3, 2018

zakorgy May 3, 2018

kvark May 3, 2018

kvark May 3, 2018

zakorgy May 3, 2018

kvark May 3, 2018

zakorgy May 3, 2018

kvark May 3, 2018

dati91 left a comment

dati91 May 4, 2018

dati91 May 4, 2018

dati91 May 4, 2018

dati91 May 4, 2018

dati91 May 4, 2018

zakorgy May 4, 2018

dati91 May 4, 2018

dati91 May 4, 2018

zakorgy May 7, 2018

dati91 May 4, 2018

dati91 May 4, 2018

dati91 May 4, 2018

zakorgy commented May 7, 2018

		@@ -2252,7 +2254,7 @@ impl<B: hal::Backend> Renderer<B> {
		&mut profile_timers,
		//&profile_samplers,

Restore DebugRenderer #162

Restore DebugRenderer #162

Conversation

zakorgy commented May 3, 2018

zakorgy commented May 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dati91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zakorgy commented May 7, 2018