Page MenuHome

Simplify `GPU_SHADER_3D_FLAT_SELECT_ID` and `GPU_SHADER_3D_UNIFORM_SELECT_ID` shaders.
ClosedPublic

Authored by Germano Cavalcante (mano-wii) on Feb 13 2019, 6:15 PM.

Details

Summary

This patch allows the drawing of the mesh selection ids to be done on a texture with 32UI format.
This simplifies the shader that previously acted on the backbuffer and had to do an uint to rgba conversion.
The size of framebuber and glViewport is now limited to the size of the threshold.

TODO:
The threshold keeps changing between selecting one element type for another, for better optimization, maintaining the same threshold would avoid having to re-create the framebuffer textures.

Diff Detail

Repository
rB Blender

Event Timeline

Germano Cavalcante (mano-wii) planned changes to this revision.Feb 13 2019, 6:41 PM

Patch stopped working for some reason.

  • Rename functions that erroneously consider reading cache.
  • cleanup: unsigned int -> uint and identation;
  • Fix crash when edit particles.

Generally looks like the right direction, although @Clément Foucault (fclem) knows these details better, running some tests though.


Weight/vertex paint vertex select has an offset.

  • Default cube
  • Subdivide 4 times
  • Enter weight paint mode.
  • VKey
  • BKey to select vertices.
  • Notice it's not working properly (rectangle is offset).

Otherwise this patch seems to work well.

source/blender/editors/space_view3d/view3d_draw_legacy.c
371

doesn't compile on Linux, use min_ii()

420

Not returning anything (shouldn't this show as a compiler warning/error?)

Campbell Barton (campbellbarton) requested changes to this revision.Feb 15 2019, 6:49 AM
This revision now requires changes to proceed.Feb 15 2019, 6:49 AM
  • Fix warnings and cleanup;
  • Make read_nearest work a bit better with rectangles;
  • Use Odd Thresholds;
  • Fix weight/vertex paint vertex selection;
Germano Cavalcante (mano-wii) marked 2 inline comments as done.Feb 15 2019, 4:03 PM
Germano Cavalcante (mano-wii) added inline comments.
source/blender/editors/space_view3d/view3d_draw_legacy.c
420

In fact some warnings appeared.
I have not noticed because in Visual Studio (except for py and glsl) the warnings only appear the first time the file is modified.
I have to edit the settings to ignore warnings of GLSL and Python codes.
I will be more careful next time.

Fix use of uninitialized variables, move rect expanding into validate_object_select_id

Campbell Barton (campbellbarton) requested changes to this revision.Feb 18 2019, 9:26 AM

Checking over this patch, it looks close to being ready, and issue with it is V3D_INVALID_BACKBUF is no longer meaningful because each request is a different size.

Could it be configured to update the entire texture but only read a small region - as it does for existing code?

This revision now requires changes to proceed.Feb 18 2019, 9:26 AM
Germano Cavalcante (mano-wii) marked an inline comment as done.Feb 18 2019, 4:13 PM

Checking over this patch, it looks close to being ready, and issue with it is V3D_INVALID_BACKBUF is no longer meaningful because each request is a different size.

Could it be configured to update the entire texture but only read a small region - as it does for existing code?

I made some local edits, but I'm not sure if keeping a framebuffer with Blender's screen-size cached is advantageous.
It would be less efficient and would also occupy much more space in Vram.

(I can try another solution by removing the use of V3D_INVALID_BACKBUF and maintaining a shared context between functions).

@Germano Cavalcante (mano-wii) keeping a buffer means that for example - when moving the cursor it doesn't need to redraw on every mouse-move that requests the selection result.

Unless there is a good reason we should remove this - it would be nice to finish this patch off without changing behavior of how the result is cached.

@Germano Cavalcante (mano-wii) keeping a buffer means that for example - when moving the cursor it doesn't need to redraw on every mouse-move that requests the selection result.

Yes, I agree with @Campbell Barton (campbellbarton). Keeping 8MB of data (for a 1080p fullscreen viewport) seems reasonable. However I'm not sure whether we should keep it in Video Memory or System Memory.

  • remove unused RegionView3D::gpuoffscreen;
  • move the select framebuffer to the viewport struct.

There are still some things to fix. Like the circle selection.
I'll investigate better later.

  • Cleanup and use dist instead of size;
  • Pass mval as a parameter of ED_view3d_select_id_read_nearest;
  • Move select id utilities to GPU_viewport.
This revision is now accepted and ready to land.Sat, Mar 2, 4:51 PM
  • Cleanup comments;
  • Remove GPU_select_index_set, GPU_select_to_index_array, GPU_select_to_index, GPU_select_to_index_array and GPU_color_depth.
  • GPU_texture: Add a new utility function to set a new texture size.
  • Move the functions to configure the select_id framebuffer back to the DRW_manager.
  • Make sure that the depth_only framebuffer is created before the select_id.
Clément Foucault (fclem) requested changes to this revision.Tue, Mar 5, 4:54 AM
Clément Foucault (fclem) added inline comments.
source/blender/draw/intern/draw_manager.c
2047–2049

What's the point in this function?

If you want to resize a texture just discard and recreate it. Adding new functions to the GPU API for something used only in one place (and that can be done without it) for no real performance gain is not really a good idea. If you doubt about the speed at which the driver allocates textures, resizing the viewport discard all GPU textures from all the TextureLists and it's still very fluid.

Making the GPU API more complex means that we will have more work to do when porting to vulkan/other API.

Also just as a note, I would prefer GPU_texture_new_size() to be named GPU_texture_size_set().

This revision now requires changes to proceed.Tue, Mar 5, 4:54 AM
source/blender/draw/intern/draw_manager.c
2047–2049

Even considering the possibility of changing the rendering API, I recognize that I know well the other APIs to make the decision to create this function (I'll remove).

However, this change was not due to the peformance of the OpenGL functions, but rather to the complexity of the code, since two framebuffers share the same texture.

So if the pointer of a texture is invalidated, it has to be attached in both framebuffers, forcing thus the creation of both even if the second does not need to be used.

This creates a bit of duplicate code and confuses a bit.

But it really is not an essential change.

  • Revert "GPU_texture: Add a new utility function to set a new texture size."
  • Recreate texture instead of resize.
Germano Cavalcante (mano-wii) marked an inline comment as done.Tue, Mar 5, 2:31 PM
Clément Foucault (fclem) requested changes to this revision.Fri, Mar 8, 2:22 PM
Clément Foucault (fclem) added inline comments.
source/blender/draw/intern/draw_manager.c
2526

This is a state change that is done on the main context, not the DRW context. Just remove the line. Any use of glReadPixels should bind the corect buffer to glReadBuffer beforehand.

This revision now requires changes to proceed.Fri, Mar 8, 2:22 PM
  • Remove wrong status change (glReadBuffer).
Germano Cavalcante (mano-wii) marked an inline comment as done.Fri, Mar 8, 2:43 PM
  • Disable RV3D_CLIPPING after use.
This revision is now accepted and ready to land.Fri, Mar 15, 1:45 AM
This revision was automatically updated to reflect the committed changes.