Details

Reviewers

Jeroen Bakker (jbakker)
Julien Kaspar (JulienKaspar)

Commits

rBdba3fb9e091a: Overlay: Flash on Mode Transfer overlay

Summary

This implements T87633

This overlay renders a flash animation on the target object when
transfering the mode to it using the mode transfer operator.
This provides visual feedback when switching between objects without
extra overlays that affect the general color and lighting in the scene.

Differences with the design task:

This uses just a fade out animation instead of a fade in/out animation. The code is ready for fade in/out, but as the rest of the overlays (face sets, masks...) change instantly without animation, having a fade in/out effect gives the impression that the object flashes twice (once for the face sets, twice for the peak alpha of the flash animation).

The rendering uses a flat color without fresnel for now, but this can be improved in the future to make it look more like the shader in the prototype.

Not enabled by default (can be enabled in the overlays panel), maybe the defaults can change for 3.0 to disable fade inactive and enable this instead.

Diff Detail

Repository: rB Blender

Event Timeline

Pablo Dobarro (pablodp606) requested review of this revision.Apr 23 2021, 12:48 AM

Pablo Dobarro (pablodp606) created this revision.

Harbormaster completed remote builds in B14183: Diff 36429.Apr 23 2021, 12:48 AM

Cleanup

Harbormaster completed remote builds in B14184: Diff 36430.Apr 23 2021, 12:50 AM

Pablo Dobarro (pablodp606) edited the summary of this revision. (Show Details)Apr 23 2021, 12:52 AM

Pablo Dobarro (pablodp606) added reviewers: Jeroen Bakker (jbakker), Julien Kaspar (JulienKaspar).

Pablo Dobarro (pablodp606) added projects: Sculpt, Paint & Texture, EEVEE & Viewport.

I tested it for a bit and it seems to work very well.
The only issue I've seen is when using the operator again while the animation is still running it has glitchy effects like the animation being run twice on the same object.

About the differences to the design task, I think it's mostly fine. The overlay is already pleasant enough to not strictly need a fade in.
A fresnel effect would be nice to have but is not strictly needed for now imo.
For the 3.0 release this should be on by default though with the fade inactive geometry overlay off by default.

EDIT: I also still think it makes sense to use this overlay when switching between objects via the outliner. (The left column of icons when in a mode)

I did a quick review. The name flash is not correct. It is what it does. but you should name it based on the intent: switching selection.

Still need to check if you really need the in front pass.
within half a second it could be that the user switches twice. I would not restart the switch for the first selection.
Best to create a shading group for an object that need to be animated and not a global one.

source/blender/draw/engines/overlay/overlay_flash.c
33 ↗	(On Diff #36430)	Empty functions can be removed.
102 ↗	(On Diff #36430)	Don't do this for all objects. Do it once in cache_init and store in private data.

Review update

Harbormaster completed remote builds in B14262: Diff 36572.Apr 27 2021, 6:26 PM

@Jeroen Bakker (jbakker) In that case the name should be something more like overlay_transfer_mode_, right? The operator and overlay don't have to do with selection. In fact, the idea would be to manage the switching mode operation independently from selection when implementing it for weight paint and armatures/meshes.
As this requires a shading group per object, where should those be stored? should they be in OVERLAY_PrivateData in a hash table?

In D11055#283458, @Pablo Dobarro (pablodp606) wrote:

@Jeroen Bakker (jbakker) In that case the name should be something more like overlay_transfer_mode_, right? The operator and overlay don't have to do with selection. In fact, the idea would be to manage the switching mode operation independently from selection when implementing it for weight paint and armatures/meshes.

Would use it at overlay_mode_transfer and use mode transfer everywhere in DNA/RNA and operators. This way search in all files would give you a good indication how it works. As the overlay is in drawing code this is already an indication that it isn't selecting.

As this requires a shading group per object, where should those be stored? should they be in OVERLAY_PrivateData in a hash table?

Should be as easy as creating the shading group during cache_populate. No need for additional data management.

Jeroen Bakker (jbakker) requested changes to this revision.Apr 28 2021, 8:30 AM

This revision now requires changes to proceed.Apr 28 2021, 8:30 AM

Review update

Harbormaster completed remote builds in B14286: Diff 36618.Apr 28 2021, 6:02 PM

Fix formatting

Harbormaster completed remote builds in B14288: Diff 36620.Apr 28 2021, 6:04 PM

I used it a bit more and the issue I had before is basically gone. The only performance related issue left that I noticed is when switching between objects that take a long time, the overlay will be significantly shorter. It's to the point that if the operator is used and it will take a couple seconds, the flash is just a very short blip.
Can this be improved so the overlay animation only starts once the transfer mode operate is done and Blender is no longer frozen so that the flash animation can run whole?

Jeroen Bakker (jbakker) added inline comments.May 14 2021, 4:01 PM

source/blender/draw/engines/overlay/overlay_mode_transfer.c

missing static?

overlay_mode_transfer.c:51:6: warning: no previous prototype for ‘mode_transfer_is_animation_running’ [-Wmissing-prototypes]
   51 | bool mode_transfer_is_animation_running(const float anim_time)
      |      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

missing static?

overlay_mode_transfer.c:56:7: warning: no previous prototype for ‘mode_transfer_alpha_for_animation_time_get’ [-Wmissing-prototypes]
   56 | float mode_transfer_alpha_for_animation_time_get(const float anim_time)
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Fix static warning

Harbormaster completed remote builds in B14565: Diff 37104.May 14 2021, 4:20 PM

Patch looks good.
I'm fine with letting MODE_TRANSFER_FLASH_FADE there for a while but if it will not change we should remove it in bcon3. perhaps add a todo or a task for this so we don't forget.

This revision is now accepted and ready to land.May 17 2021, 9:46 AM

This looks all good to me. It can be improved further in the future if we want.
I presented the current state to Ton and he approved it. So this can land in master now.

Closed by commit rBdba3fb9e091a: Overlay: Flash on Mode Transfer overlay (authored by Pablo Dobarro (pablodp606)). · Explain WhyJun 3 2021, 8:20 PM

This revision was automatically updated to reflect the committed changes.

Pablo Dobarro (pablodp606) added a commit: rBdba3fb9e091a: Overlay: Flash on Mode Transfer overlay.

Overlay: Flash on Mode Transfer overlay
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 37818

release/scripts/startup/bl_ui/space_view3d.py

source/blender/draw/CMakeLists.txt

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

source/blender/draw/engines/overlay/overlay_mode_transfer.c

source/blender/draw/engines/overlay/overlay_private.h

source/blender/editors/object/object_modes.c

source/blender/makesdna/DNA_object_types.h

source/blender/makesdna/DNA_view3d_types.h

source/blender/makesrna/intern/rna_space.c

Overlay: Flash on Mode Transfer overlayClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 37818

release/scripts/startup/bl_ui/space_view3d.py

source/blender/draw/CMakeLists.txt

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

source/blender/draw/engines/overlay/overlay_mode_transfer.c

source/blender/draw/engines/overlay/overlay_private.h

source/blender/editors/object/object_modes.c

source/blender/makesdna/DNA_object_types.h

source/blender/makesdna/DNA_view3d_types.h

source/blender/makesrna/intern/rna_space.c

Overlay: Flash on Mode Transfer overlay
ClosedPublic
Actions

Revision Contents
Changeset List