Details

Reviewers

Pablo Vazquez (pablovazquez)
Campbell Barton (campbellbarton)

Group Reviewers

User Interface

Commits

rBc8e331f45003: UI - LOCAL View3D overlay stats

Summary

Changes that allow the 3DView Overlay to show local stats when in local view

Once this patch is applied, you should see that the statusbar stats and the overlay stats can vary from each other. The following shows a workspace with three views, with the middle and right sides in local view

This is done by adding an optional stats struct to View3D, to be used only when in localview. This way most views can share one copy of the stats, only need a unique version when necessary.

Diff Detail

Repository

rB Blender

Branch

TEMP-D8883-UPDATE (branched from master)

Build Status

Buildable 15228
Build 15228: arc lint + arc unit

Event Timeline

Harley Acheson (harley) requested review of this revision.Sep 13 2020, 11:52 PM

Harley Acheson (harley) created this revision.

Juan (jc4d) awarded a token.Sep 14 2020, 8:10 AM

Lloyd (digim0nk) awarded a token.Sep 14 2020, 9:08 PM

Harley Acheson (harley) updated this revision to Diff 29087.Sep 19 2020, 7:50 PM

Harley Acheson (harley) edited the summary of this revision. (Show Details)

Harley Acheson (harley) added a reviewer: User Interface.

Harley Acheson (harley) added a project: BF Blender (2.92).

Harley Acheson (harley) retitled this revision from UI WIP - LOCAL View3D overlay stats to UI - LOCAL View3D overlay stats.Sep 19 2020, 8:12 PM

Yuichiro Fujita (chironamo) awarded a token.Sep 20 2020, 11:26 PM

Harley Acheson (harley) mentioned this in T81171: Status Bar - Crucial Updates Appear Offscreen.Sep 30 2020, 1:14 AM

Updating to current state of master.

Yay thanks for working on this! +1

From the UI point of view this works as expected (local stats only in viewports in Local view, status bar shows totals).

This revision is now accepted and ready to land.Oct 19 2020, 1:55 AM

Harley Acheson (harley) added a reviewer: Campbell Barton (campbellbarton).Oct 19 2020, 2:26 AM

Nice functionality, found some issues with internal logic.

This is calculating statistics every-redraw.

Instead this patch should match the behavior of view-layer statistics, where it's updated after ED_info_stats_clear is called.

As there are 16 local view slots, we could store those in the view-layer as well, then use bitscan_forward_i(v3d->local_view_uuid) to get the index, this means we wouldn't have to scan all open 3D views to clear their local-view statistics.

source/blender/editors/space_info/info_stats.c
369	Passing a `v3d` which isn't a local-view will cause a v3d spesific value to be written into `view_layer->stats`. This isn't obvious and could easily cause bugs in the future. Suggest to rename this `v3d_local` here as well as the functions that it's passed to here. Also assert if `v3d->localvd` is NULL to be sure it's used as intended.
377–387	This seems incorrect as local-view can contain some objects not in edit-mode.
409	No need for calloc when the value's immediately overwritten.
source/blender/editors/space_view3d/view3d_view.c
1417	`MEM_SAFE_FREE` already NULL's the value.
source/blender/makesdna/DNA_view3d_types.h
294–295	If this stays in the `View3D` struct, it should be moved to `View3D_Runtime` as we never want to save or load it.

This revision now requires changes to proceed.Oct 19 2020, 7:43 AM

Campbell Barton (campbellbarton) added inline comments.Oct 19 2020, 7:53 AM

source/blender/editors/space_info/info_stats.c
131	Checking `BKE_object_is_visible_in_viewport` means all of the values used in that function must cause the local-views statistics to refresh when they are changed.

updating to current state of master. Review items largely not addressed, just getting it compiling again for now.

Harley Acheson (harley) planned changes to this revision.Apr 18 2021, 11:47 PM

Fixed many things where I understood the issue, but not where I am unsure. Review items fixed:

This is calculating statistics every-redraw...updated after ED_info_stats_clear is called.

Yes, took a bit to see this problem. Now clearing these in ED_info_stats_clear() and only updating when necessary.

No need for calloc when the value's immediately overwritten

Fixed.

MEM_SAFE_FREE already NULL's the value.

Fixed

should be moved to View3D_Runtime as we never want to save or load it.

Yes, done.

just a bit of cleanup.

This looks very close to being ready, although there is currently a bug with the v3d interfering with the global scene statistics.

Duplicate the cube 5 times.
Disable drawing Mesh objects (in the object visibility header popover).
Enable 3D view statistics.

Notice it excludes the mesh objects (good).

Enable the status-bar statistics.

Notice it excludes the mesh objects based on the view-port setting (bug).

Even with this bug fixed, I'm still concerned are that this patch implements quite spesific checks in a way which isn't obvious from reading the code.

We should avoid the possibility of values depending on the view-port writing into the scene's statistics, as this is the kind of bug which can take a while to track down.

In most view-port code the 3D view is passed in as an argument and used when it's available.
Where as in this case using the view-port is an exception that will back-fire when used unintentionally.

Suggest the following.

When the v3d is not a local-view, it must be set to NULL (at the start of ED_info_draw_stats for e.g.). assert this is the case in functions that take the v3d argument.
Rename v3d to v3d_local (while it could be noted in docs, it seems clearer to rename the variable since it's not overly verbose).

source/blender/editors/space_info/info_stats.c
407	This means when in edit-mode and not in local-view: all non-edit-mode objects are included. As far as I can see this should still perform the `BASE_VISIBLE_VIEWLAYER` check.
441	No need for surrounding parenthesis.
447	No need for surrounding parenthesis.
457	The `v3d->localvd` doesn't seem necessary here. While it's not expected, this value could be cleared without checking localvd.
478	The last NULL check seems redundant, under what conditions would a local view not have it's `v3d->runtime.local_stats` value set? (should be noted in comment, otherwise NULL check can be removed).
source/blender/makesdna/DNA_view3d_types.h
261	Should have brief one line comment `Local view statistics, only set when ...`

This revision now requires changes to proceed.Apr 19 2021, 4:14 AM

Thanks @Campbell Barton (campbellbarton)! I’ll try to get to this in this next week or so. Now wishing I didn’t let it sit for six months (all my fault) as some details are now a bit fuzzy.

Benjamin Sauder (kioku) awarded a token.Apr 19 2021, 8:10 AM

Gilberto Rodrigues (gilberto_rodrigues) awarded a token.Apr 19 2021, 4:48 PM

Pratik Borhade (PratikPB2123) added a subscriber: Pratik Borhade (PratikPB2123).Jun 14 2021, 11:30 AM

Updated to current state of master and to address review issues.

An error remains and is flagged here:

stats_update() in blender\editors\space_info\info_stats.c contains the following comment:

/* XXX - Unable to get number of objects in local view while in edit mode. */

The error is that I have been unable to get the number of objects that are in local view while in Edit mode. Like this:

I have a scene containing three cubes, camera and lamp. "Objects 0/5". Good
I select two cubes. "Objects 2/5". Good
"/" into local view. "Objects 2/2". Good
Select one of them. "Objects 1/2". Good
Tab into Edit Mode. "Objects 1/5". BAD

Only pass in the v3d argument to statistics functions when it's set to local-view

Avoid statistics using viewport settings for the global scene statistics,
which won't be valid for other viewports.

Harbormaster completed remote builds in B15193: Diff 38316.Jun 15 2021, 3:56 AM

Fix object count in edit-mode

Harbormaster completed remote builds in B15194: Diff 38317.Jun 15 2021, 3:57 AM

Campbell Barton (campbellbarton) mentioned this in rB2ba804d7b7dc: Screen: clear runtime structures on file-read & data-copy.Jun 15 2021, 4:54 AM

Committed rB2ba804d7b7dc which resolves a crash reloading the file (runtime memory wasn't cleared).

There is a remaining bug, changing selection in object mode doesn't update the number of selected objects.

@Campbell Barton (campbellbarton) - There is a remaining bug, changing selection in object mode doesn't update the number of selected objects.

Yes, I see this but (again) not sure how to fix...

You definitely fixed an error of mine in format_stats(). I was calling stats_update() if we were in local view and localview stats were null OR if the view_layer-> stats were null. It was the second part that was the error in that it caused stats_update() to be called all the time while in local_view. So things worked, but called far too often. But that error was hiding another error.

The only time that ED_info_stats_clear() is called is in wm_event_do_notifiers(). I am assuming that I can clear both viewlayer and local stats. But from there the call to CTX_wm_view3d(C) is always NULL if in local view. Same with CTX_wm_area(C), just to see if I could get to the v3d from there. So not sure how to get to the current v3d while in local view from there to clear the stats.

Fix issue with missing updates
Statistics are freed on 3D view exit & refresh (so stale statistics aren't reused).

Harbormaster completed remote builds in B15228: Diff 38367.Jun 16 2021, 2:04 AM

Correct own comparison error in last update.

Harbormaster completed remote builds in B15229: Diff 38368.Jun 16 2021, 2:09 AM

resolve crash calling the exit function with a NULL 3D view.

Harbormaster completed remote builds in B15232: Diff 38371.Jun 16 2021, 2:39 AM

This revision was not accepted when it landed; it landed in state Needs Review.Jun 16 2021, 4:04 AM

Closed by commit rBc8e331f45003: UI - LOCAL View3D overlay stats (authored by Harley Acheson (harley)). · Explain Why

This revision was automatically updated to reflect the committed changes.

Harley Acheson (harley) added a commit: rBc8e331f45003: UI - LOCAL View3D overlay stats.

Andres Stephens (Draise) awarded a token.Jun 16 2021, 4:48 PM

UI - LOCAL View3D overlay stats
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 38367

source/blender/editors/include/ED_info.h

source/blender/editors/space_info/info_stats.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/editors/space_view3d/view3d_draw.c

source/blender/editors/space_view3d/view3d_view.c

source/blender/makesdna/DNA_view3d_types.h

source/blender/windowmanager/intern/wm_event_system.c

UI - LOCAL View3D overlay statsClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 38367

source/blender/editors/include/ED_info.h

source/blender/editors/space_info/info_stats.c

source/blender/editors/space_view3d/space_view3d.c

source/blender/editors/space_view3d/view3d_draw.c

source/blender/editors/space_view3d/view3d_view.c

source/blender/makesdna/DNA_view3d_types.h

source/blender/windowmanager/intern/wm_event_system.c

UI - LOCAL View3D overlay stats
ClosedPublic
Actions

Revision Contents
Changeset List