Recalc selection count at the end of the circle selection.
Details
Diff Detail
- Repository
- rB Blender
- Branch
- T95165 (branched from master)
- Build Status
Buildable 21953 Build 21953: arc lint + arc unit
Event Timeline
As far as I was aware the value of BMesh.totfacese was expected to be valid, there are many other parts of the code which check this value.
Checking rBea4309925f1d2d2a224bd1dce12269a58ade9b62, it looks like circle select isn't updating selection totals, couldn't this be made to update the selection number once the operator has completed?
Guys, I think T95165: Regression: Custom Normals tools dont update immediately (when started with Auto Smooth OFF) is already resolved, no (will check when)?
Can you still reproduce this? What is this going to fix instead?
I miss the paper trail :-)
According to us there could be similar issues where operators use the totsel fields, but aren't set after edit mode circle selection.
From looking into this patch it has a some drawbacks.
- All uses of the circle gesture will run an extra time on completion (while that's not necessarily such a problem, it means there may be cases where the exec function needs to check is_finished only to avoid running an extra time).
- It's possible (although unlikely) view3d_circle_select_finished doesn't run. Since the event system may cancel a running modal operator via the cancel callback.
From looking into this there are couple of ways this could be handled without having to further extend operator callbacks.
- Wrap WM_gesture_circle_modal & WM_gesture_circle_cancel with functions that run a callback when modal exits or cancel is called.
- Store data in the wmGesture.user_data which is guaranteed to run it's free callback, see wmGenericUserDataFreeFn. The allocated data could be a simple struct, it can store a boolean thats set once selection has changed (so flushing doesn't run unnecessarily), possible other data could be stored there for reuse later.
Both options seem acceptable but I'd prefer rely on the free callback as it seems less likely the callback fails to run (if that happens we'll get a memory leak too).
| source/blender/editors/space_view3d/view3d_select.c | ||
|---|---|---|
| 4614–4615 | This is indeed a bit odd, checking on it em_setup_viewcontext is expected to be used to set the vc.em. Most likely em_setup_viewcontext could be removed and ED_view3d_viewcontext_init could set the em member but that can be done as part of a separate commit. | |
I prefer the freefn approach. Mostly as I am not familiar with the impact that the other approaches have. (Adding a separate callback seems to be the clearest, but as this is ui code I expect a broader discussion is needed).
Seems like the user_data is already in use (sometimes). So would need to fallback to the function wrapping approach. Adding it to the same user_data would add a lot of ugly code and exceptions.
| source/blender/editors/space_view3d/view3d_select.c | ||
|---|---|---|
| 4635 | to be safe check: result & OPERATOR_FINISHED | |
Has been landed into blender-v3.2-release manually due to arcanist issues on mac.
rB0375720e2889: Fix T95165: Custom Normals tools dont update immediately.
Think this is not a fix for T95165: Regression: Custom Normals tools dont update immediately (when started with Auto Smooth OFF) (that report did not even use circle select)
That was apparently already fixed by rB0f89bcdbebf5: Fix depsgraphs sharing IDs via evaluated edit mesh
Will also check if the bisect for cause of T95165 was possibly wrong? (appologies in that case).
Should the commit message/title be changed?
OK, so T95165 was originally caused by rB51568030e9c8: Depsgraph: remove redundant mesh data duplication in edit-mode