Details

Reviewers

Julian Eisel (Severin)
Antonio Vazquez (antoniov)

Maniphest Tasks

T77730: ShaderFx Missing Update Notifier

Commits

rBa856de700b42: Fix T77730: ShaderFx Missing Update Notifier

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.Jun 30 2020, 3:45 AM

Hans Goudey (HooglyBoogly) created this revision.

Harbormaster completed remote builds in B8769: Diff 26395.Jun 30 2020, 3:45 AM

Move ND_SHADERFX to NC_OBJECT category

Harbormaster completed remote builds in B8770: Diff 26396.Jun 30 2020, 3:46 AM

Hans Goudey (HooglyBoogly) added a task: T77730: ShaderFx Missing Update Notifier.Jun 30 2020, 3:47 AM

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Jun 30 2020, 3:54 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.Jun 30 2020, 9:39 AM

Remove notifier value collision

Harbormaster completed remote builds in B8790: Diff 26431.Jul 1 2020, 3:30 PM

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.Jul 2 2020, 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`.

Julian Eisel (Severin) added inline comments.Jul 3 2020, 6:08 PM

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.

Antonio Vazquez (antoniov) added inline comments.Jul 3 2020, 8:21 PM

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.

Remove outliner shaderfx redraw
Remove image space area redraw
Remove NC_OBJECT | ND_SHADING redraw cases, use old values

Harbormaster completed remote builds in B8868: Diff 26563.Jul 6 2020, 6:43 PM

Hans Goudey (HooglyBoogly) added inline comments.Jul 6 2020, 6:43 PM

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.

Closed by commit rBa856de700b42: Fix T77730: ShaderFx Missing Update Notifier (authored by Hans Goudey (HooglyBoogly)). · Explain WhyJul 6 2020, 7:17 PM

This revision was automatically updated to reflect the committed changes.

Hans Goudey (HooglyBoogly) added a commit: rBa856de700b42: Fix T77730: ShaderFx Missing Update Notifier.

Fix T77730: ShaderFx Missing Update Notifier
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 26565

source/blender/editors/object/object_shader_fx.c

source/blender/editors/space_buttons/space_buttons.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/makesrna/intern/rna_shader_fx.c

source/blender/windowmanager/WM_types.h

Fix T77730: ShaderFx Missing Update NotifierClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 26565

source/blender/editors/object/object_shader_fx.c

source/blender/editors/space_buttons/space_buttons.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/makesrna/intern/rna_shader_fx.c

source/blender/windowmanager/WM_types.h

Fix T77730: ShaderFx Missing Update Notifier
ClosedPublic
Actions

Revision Contents
Changeset List