Page MenuHome

Overlay Engine: Refactor & Cleanup
ClosedPublic

Authored by Clément Foucault (fclem) on Sun, Nov 24, 2:05 AM.
Tags
None
Tokens
"Mountain of Wealth" token, awarded by duarteframos."100" token, awarded by Frozen_Death_Knight."Love" token, awarded by monio."Love" token, awarded by Kramon."Mountain of Wealth" token, awarded by franMarz."Mountain of Wealth" token, awarded by jbakker."Love" token, awarded by mywa880."Burninate" token, awarded by amonpaike.

Details

Summary

This is the unification of all overlays into one overlay engine as described in T65347.

I went over all the code making it more future proof with less hacks and removing old / not relevent parts.

Goals / Acheivements:

  • Remove internal shader usage (only drw shaders)
  • Remove viewportSize and viewportSizeInv and put them in gloabl ubo
  • Fixed some drawing issues: Missing probe option and Missing Alt+B clipping of some shader
  • Remove old (legacy) shaders dependancy (not using view UBO).
  • Less shader variation (less compilation time at first load and less patching needed for vulkan)
  • removed some geom shaders when I could
  • Remove static e_data (except shaders storage where it is OK)
  • Clear the way to fix some anoying limitations (dithered transparency, background image compositing etc...)
  • Wireframe drawing now uses the same batching capabilities as workbench & eevee (indirect drawing).
  • Reduced complexity, removed ~3000 Lines of code in draw (also removed a lot of unused shader in GPU).
  • Post AA to avoid complexity and cost of MSAA.

Remaining issues:

  • Armature edits, overlay toggles, (... others?) are not refreshing viewport after AA is complete
  • FXAA is not the best for wires, maybe investigate SMAA
  • Maybe do something more temporally stable for AA.
  • Paint overlays are not working with AA.
  • infront objects are difficult to select.
  • the infront wires sometimes goes through they solid counterpart (missing clear maybe?) (toggle overlays on-off when using infront+wireframe overlay in solid shading)

Note: I made some decision to change slightly the appearance of some objects to simplify their drawing. Namely the empty arrows end (which is now hollow/wire) and distance points of the cameras/spots being done by lines.

Diff Detail

Repository
rB Blender

Event Timeline

source/blender/draw/engines/overlay/overlay_antialiasing.c
94

Does nr ever gets 0?

for valid_history is false when sample is 0!, but gets incremented in OVERLAY_antialiasing_end

source/blender/draw/engines/overlay/overlay_engine.c
335

I would say store a flag in pd Check is easy but we could do this later so we can test it more

355

Move to top of file?

Jeroen Bakker (jbakker) requested changes to this revision.Tue, Nov 26, 10:45 AM
  • Viewport render image is incorrect due to incorrect alpha layer
    Shows the alpha render layer
  • TAA Depth fighting in vertex/weight paint (only when TAA is turned on)
  • Viewport Preferences Should be redone.
    • Edit-Mode Smooth Wires does not do a thing
    • Viewport Overlay AA setting in stead of checking Multisampling.

Overall structure is clear. There are some optimizations that could have more documentation and encode/decode functions to make the code more readable.

IMO we should clean up the obvious limitations and commit as early as possible as many projects/patches are waiting on this change and needs to be modified.
Additional support/development still needs to take place ... but we could do that in master, but should be communicated clearly to users.

I still have questions how to implement some future improvements, but we can do that afterwards.

source/blender/draw/engines/overlay/overlay_engine.c
158

name init seems to be incorrect.

189

I don't like the part abbreviation, particle is more clear.

466

Should we for clarity call a function in overlay_antialiasing to keep all the code in a single place.

source/blender/draw/engines/overlay/overlay_engine.h
16

We should go over these year numbers... Seems like a copy paste or rename issue The C file states 2019, the header doesn't

source/blender/draw/engines/overlay/overlay_extra.c
84

Is this a todo or can this be removed?

278

We should explain somewhere that this trick can be done and is not intervening with the vert shader...

282

DRY.

We could add an inline function and pass the DRWPass. Keep current functions for readable code.

351

Should we move this inside the switch statement.

379

move in else? or if (bb != NULL) {.

as this code needs to be kept in sync with BKE_object_boundbox_get

825

union not needed here

source/blender/draw/engines/overlay/overlay_metaball.c
131

Should we for consistency add a draw_in_front here?

source/blender/draw/engines/overlay/overlay_motion_path.c
60

Should this be moved to a draw_cache_impl_armature or draw_cache_impl_motionpath?
I would suspect that future refactoring is needed in the bMotionPath structure (adding runtime field...)

174

Perhaps a feature change, but should we check for V3D_OVERLAY_HIDE_TEXT?

source/blender/draw/engines/overlay/overlay_outline.c
57

can call shgroup_theme_id_to_outline_id and return the correct one based on the outline_id.

85

can call shgroup_theme_id_to_outline_id and return the correct one based on the outline_id.

source/blender/draw/engines/overlay/overlay_shader.c
755

Define all the theme ids we need in the shader?

source/blender/draw/engines/overlay/shaders/edit_mesh_geom.glsl
67

Seems not optimal?

vec3 edge_ofs = vec3(halfsize * sizeViewportInv.xy, 0.0);

source/blender/draw/engines/overlay/shaders/wireframe_vert.glsl
35

Add defines and DRY

int flags = int(abs(ObjectInfo.w));

source/blender/draw/intern/draw_cache_impl_lattice.c
368–371

We should make a constant for this number and name it the same in the shaders

source/blender/gpu/intern/gpu_shader.c
1275

can be merged?

This revision now requires changes to proceed.Tue, Nov 26, 10:45 AM
Clément Foucault (fclem) edited the summary of this revision. (Show Details)
Clément Foucault (fclem) marked 23 inline comments as done.
  • Cleanup: Fix copyright year
  • Cleanup: Naming conventions, code clarity mentionned in review
  • Fix crash caused by copy pasting!
  • Fix FXAA alpha issue.
  • Fix In Front object selectability
  • Fix infront selectability of solid objects
  • Fix Wireframe issue with infront objects
  • Edit Armature: Tag armature after any operator to invalidate viewport
  • Overlay Engine: Update overlays on ID update
  • Overlay Engine: Make overlay parameters reset TAA
  • Overlay Engine: Fix Armature bone toggle
  • Merge branch 'master' into tmp-overlay-engine
  • Overlay Engine: Fix Paint overlays disapearing because of TAA
  • Overlay Engine: Fix armature shape always transparent
  • Cleanup: Code readability/simplicity
  • Overlay Engine: Wireframe: Fix wireframe overlay on top of infront objects
  • Overlay Engine: Fix crash when enabling pose bone selection overlay
  • Merge branch 'master' into tmp-overlay-engine
Jeroen Bakker (jbakker) requested changes to this revision.Wed, Nov 27, 3:38 PM

The TAA first sample + a spelling. I will start testing this in more depth. But first review seems almost finished.

source/blender/draw/engines/overlay/overlay_antialiasing.c
94

Still not solved. (I checked in tmp-overlay-engine)

Perhaps easiest fix is to set target_sample to 4 and add a dummy sample
const float samples_pos[5][2] = {{0.0f, 0.0f}, {-2, 6}, {6, 2}, {-6, 2}, {2, 6}};

OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 2 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 2 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 2 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 2 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 1 }
OverlayTAA: { nr: 2 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 3 }
OverlayTAA: { nr: 3 }
source/blender/draw/engines/overlay/overlay_motion_path.c
60

TODO is easier to find than T0D0

This revision now requires changes to proceed.Wed, Nov 27, 3:38 PM
  • Overlay Engine: Fix/Improve Temporal AntiAliasing
Jeroen Bakker (jbakker) requested changes to this revision.Thu, Nov 28, 9:00 AM

TAA is fixed. Code wise I think we're there. Some functional limitations I detected:

Face Orientation

Wireframe Responsiveness

When using dense poly meshes the TAA drawing of wireframes on top of solid, material/render is annoying.

Wireframe toggling

I haven't found the exact issue, but toggling the wire frame overlay on/off with bigger scenes just makes the wires draw differently (1 sample?).

Object selection slowdown

Object selection is slower than used to be

This revision now requires changes to proceed.Thu, Nov 28, 9:00 AM
  • Overlay Engine: Fix issue with facing overlay
  • Overlay Engine: Fix loose wire showing after edit mode and not showing in..

with the factory-startup the TAA is disabled. That is good as TAA has some artifacts we still want to solve. I still think we should disable TAA when doing navigation at least.

Things we should do after merging

  • Solve TAA :-)
  • Connect the TAA to a different user pref. Attaching to the Multisampling is not really clear. Connect to the Viewport Anti-Aliasing or a separate setting Overlay Anti-Aliasing. We could check with @JulianKaspar today
  • Remove the Edit-Mode Smooth Wires it isn't used anymore.
  • Remove the Multisampling option? We should check if it is being used.
This revision is now accepted and ready to land.Fri, Nov 29, 9:16 AM
  • Overlay Engine: Add SMAA to replace FXAA
  • Overlay Engine: Fix Sculpt Mask overlay
  • Overlay Engine: Change Antialiasing implementation to a novel method!
  • Merge branch 'master' into tmp-overlay-engine
This revision was automatically updated to reflect the committed changes.