Page MenuHome

Fix T77730: ShaderFx Missing Update Notifier
ClosedPublic

Authored by Hans Goudey (HooglyBoogly) on Tue, Jun 30, 3:45 AM.

Details

Summary

I have the delete shortcut ready for shader effects like the other stack UIs,
but because they're missing an update notification it doesn't work properly.

Theoretically this should be done whenever a context for the properties editor
is added.

The main thing I wanted to check on is whether I defined ND_SHADERFX in the correct place in WM_types.h.
But also, @Antonio Vazquez (antoniov). I checked viewport cycles and EEVEE rendering and it all works, so I'm
assuming I have the update in all the necessary places.

Diff Detail

Repository
rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) requested review of this revision.Tue, Jun 30, 3:45 AM
Hans Goudey (HooglyBoogly) created this revision.
  • Move ND_SHADERFX to NC_OBJECT category
Antonio Vazquez (antoniov) requested changes to this revision.EditedTue, Jun 30, 9:39 AM

The code looks good to me, but I could not compile due the duplicated case error in space_buttons.c line 444

source/blender/windowmanager/WM_types.h
364

Compiling in Windows get an error because this is already used.

case ND_SHADING:
       case ND_SHADING_DRAW:
       case ND_SHADING_LINKS:  <<<<<<<<<<<<<<<< This one
       case ND_SHADING_PREVIEW:
         /* currently works by redraws... if preview is set, it (re)starts job */
         sbuts->preview = 1;
         break;

Error: C2196 case value '2097152' already used

This revision now requires changes to proceed.Tue, Jun 30, 9:39 AM
  • Remove notifier value collision

All compiler issues have gone. I have tested and all looks right. Now if you have several panels with the same FX, all are updated.

Now we need that @Julian Eisel (Severin) review the UI code to verify all is right.

This revision is now accepted and ready to land.Thu, Jul 2, 4:35 PM

Minor changes requested. Overall this looks fine (didn't test though).

source/blender/editors/space_buttons/space_buttons.c
416–418

Wouldn't just adding this be an alternative fix?
Did you try that?

I think it's fine to decouple notifers though. Allows more fine-tuned updates and it's easier to reason about things. So adding a SHADERFX notifier type is fine with me too. But if this simple fix is possible, you should at least mention why you didn't choose it in the commit message.

427

Function call is duplicated.

source/blender/windowmanager/WM_types.h
370–371

Grepping the code, turns out there are no usages of NC_OBJECT | ND_SHADING(_FOO).

So you can actually just keep these at their old values and remove the cases in buttons_area_listener() for NC_OBJECT.

source/blender/editors/space_image/space_image.c
414 ↗(On Diff #26431)

Also, are you sure the info editor needs to be refreshed and redrawn when shaderfx settings change? It seems this is only needed to update the ghost-edges drawn to show the modifier deformation:

Maybe a question for @Antonio Vazquez (antoniov).
I'd prefer not adding the notifier listener if we're unsure to prevent unnecessary updates. We can add it back if it causes issues.

source/blender/editors/space_image/space_image.c
414 ↗(On Diff #26431)

I don't need panel refresh, but need a viewport refresh..if this done, it's enough. Not sure what you mean here.

Hans Goudey (HooglyBoogly) marked 5 inline comments as done.
  • Remove outliner shaderfx redraw
  • Remove image space area redraw
  • Remove NC_OBJECT | ND_SHADING redraw cases, use old values
source/blender/editors/space_buttons/space_buttons.c
416–418

Yeah, I think it makes more sense to keep them decoupled. We can avoid some updates like that, and more that I didn't catch that you mentioned below. I'll note it in the commit.