Page MenuHome

Simplify tool improvements
Needs RevisionPublic

Authored by Pablo Dobarro (pablodp606) on Jul 30 2019, 7:40 PM.

Details

Summary

Requested in T57754

  • Rename simplify to detail
  • Hide the tool when dyntopo is disabled
  • Add the detail size property to the toolbar

Diff Detail

Repository
rB Blender
Branch
simplify-fixes-t57754 (branched from master)
Build Status
Buildable 4221
Build 4221: arc lint + arc unit

Event Timeline

This revision is now accepted and ready to land.Jul 30 2019, 8:33 PM

Thanks for this - it's a small but important fix to make this tool make a little more sense.

Brecht Van Lommel (brecht) requested changes to this revision.Jul 31 2019, 1:13 AM
Brecht Van Lommel (brecht) added inline comments.
release/scripts/startup/bl_ui/space_toolsystem_toolbar.py
52

Suggest to rename filter to exclude_filter, it's a bit more clear what kind of filter this is then.

This should also have a default value: exclude_filter={},

source/blender/editors/sculpt_paint/sculpt.c
5807

This permanently changes the brush from a detail to a draw brush, I don't think that's the right behavior?

I guess what really needs to happen is that toggling dynamic topology causes some kind of tool system refresh, which then leads to the detail brush being unavailable and the tool system switching to the default (draw) brush.

This revision now requires changes to proceed.Jul 31 2019, 1:13 AM
  • Rename filter to exclude_filter
  • Remove wrong brush update from sculpt.c
  • Fix tool switching after disabling dyntopo
Pablo Dobarro (pablodp606) marked 2 inline comments as done.Aug 1 2019, 2:57 PM
Brecht Van Lommel (brecht) requested changes to this revision.Aug 2 2019, 12:55 PM
Brecht Van Lommel (brecht) added inline comments.
source/blender/editors/sculpt_paint/sculpt.c
5806–5807

This isn't quite correct since there can be multiple workspaces. Or the operator could be executed in a context where there is no user interface / workspace / area, and crash.

@Campbell Barton (campbellbarton), do you know how to do this kind of refresh?

This revision now requires changes to proceed.Aug 2 2019, 12:55 PM
source/blender/editors/sculpt_paint/sculpt.c
5806–5807

Currently there isn't a way to handle context changes to sub-modes, this needs to be supported though, think this patch could just not handle this, or - be put on-hold until support is added.

source/blender/editors/sculpt_paint/sculpt.c
5806–5807

I don't know exactly what happens now when you switch dynamic topology with the detail tool, without this code. If there's no weird errors and users just need to switch to another brush it's not that bad and we can leave out this code.

Otherwise we can loop over all the active workspace and areas.

for (wmWindow *win = wm->windows.first; win; win = win->next) {
  WorkSpace *workspace = WM_window_get_active_workspace(win);
  bScreen *screen = WM_window_get_active_screen(screen);

  for (ScrArea *sa = screen->areabase.first; sa; sa = sa->next) {
    if (sa->spacetype == SPACE_VIEW3D) {
      bToolKey tkey;
      WM_toolsystem_key_from_context(view_layer, sa, &tkey);
      WM_toolsystem_ref_set_by_id(C, workspace, &tkey, "builtin_brush.Draw", false);
    }
  }
}
source/blender/editors/sculpt_paint/sculpt.c
5806–5807

The workspace won't have any active tool and the topbar will be empty until you select another tool. It does not crash, but most users are probably going to consider that a bug

source/blender/editors/sculpt_paint/sculpt.c
5806–5807

Right, then changing the tool in all workspaces as in my code snippet seems best?

Regarding

bToolKey tkey;
WM_toolsystem_key_from_context(view_layer, sa, &tkey);
WM_toolsystem_ref_set_by_id(C, workspace, &tkey, "builtin_brush.Draw", false);

It'd be best to check if the tkey->mode matches the current mode before setting the tool - or even that the active objects match.
Otherwise it will try set the tool in a mode which doesn't make sense, while I didn't try this, the best case is it will fail silently.