Fix T79816: UI: Restore scene.statistics() function
ClosedPublic
Actions

Authored by Hans Goudey (HooglyBoogly) on Aug 17 2020, 11:24 PM.

Details

Reviewers

Julian Eisel (Severin)

Maniphest Tasks

T79816: bpy.context.scene.statistics no longer exists in the Python API

Commits

rB8cbfc4c76ed7: Fix T79816: Restore scene.statistics() BPY function
rB83c78529b9ef: Fix T79816: Restore scene.statistics() BPY function

Summary

This RNA function was removed in rBc08d84748804. For understandable reasons really-- getting scene
statistics from a string displayed in the status bar is not exactly the best design. But we have
committed to not changing the RNA API too much for the 2.90 release, so we would like to keep this
functionality.

Generally it's quite easy to restore the function. A few considerations:

In the future this should return a dedicated Statistics structure to python, so it might be best to deprecate this functionality eventually anyway.
There is no need to remove the screen.statusbar_info property-- it really is a property of the status bar, so we can expose it there too.
Because parts of the statistics message can be disabled in the preferences, if we did this naively we would end up with a broken API anyway. To fix this I added an override to the statistics statistics used only in this situation.
Even in 2.83, you have to activate a view layer to fill its Statistics information, the statistics string is only valid after that. This patch does not aim to solve that.

Diff Detail

Repository: rB Blender

Event Timeline

Hans Goudey (HooglyBoogly) created this revision.Aug 17 2020, 11:24 PM

Harbormaster completed remote builds in B9587: Diff 27821.Aug 17 2020, 11:24 PM

Hans Goudey (HooglyBoogly) requested review of this revision.Aug 17 2020, 11:24 PM

Hans Goudey (HooglyBoogly) added a task: T79816: bpy.context.scene.statistics no longer exists in the Python API.Aug 17 2020, 11:25 PM

Ethan Simon (razed) added a subscriber: Ethan Simon (razed).Aug 17 2020, 11:47 PM

As far as functionality goes, this patch works for me. I'll let others do the real code review though since I'm unfamiliar with that area. It also works regardless if the stats are showing in the status bar or not, which is a good thing(tm) :)

My personal addon now looks like the following -- this is probably an ok break?

if bpy.app.version < (2, 90, 0):
    view_layer = ...
    stats = bpy.context.scene.statistics(view_layer)
else:
    stats = bpy.context.scene.statistics()

Oops, as @Jesse Yurkovich (deadpin) points out, I should use view layer as an argument instead of the view layer from the context.

Julian Eisel (Severin) added inline comments.Aug 18 2020, 4:35 PM

source/blender/editors/space_info/info_stats.c
556–558	I'm not entirely sure how BPY handles strings as return values of RNA functions. Should it just wrap the `char *` and not create a copy, the way this is done here is problematic. E.g. calling the screen level function with `U.statusbar_flag = 0` and then calling it on the scene level would cause the result of the former to change, because both share the same buffer. Maybe the buffer gets copied and it's not an issue. If it is, we should add a buffer back to the view layer.

Pass explicit reference for view layer instead of relying on context

Harbormaster completed remote builds in B9624: Diff 27890.Aug 18 2020, 10:27 PM

Hans Goudey (HooglyBoogly) added inline comments.Aug 18 2020, 10:38 PM

source/blender/editors/space_info/info_stats.c
556–558	In practice I haven't found this to be an issue. The buffer is reused but I guess it's just filled properly right after. Adding the buffer back to view layers would be sort of unfortunate in my opinion. This is really a terrible interface for getting these statistics. Even though we're restoring it now, I think we should remove it and provide a better alternative in the future.

Hans Goudey (HooglyBoogly) edited the summary of this revision. (Show Details)Aug 18 2020, 11:28 PM

Hans Goudey (HooglyBoogly) added a project: User Interface.

Julian Eisel (Severin) added inline comments.Aug 19 2020, 12:17 AM

source/blender/editors/space_info/info_stats.c
556–558	I checked the BPY/RNA code, and it seems we always copy the buffer into the Python string. In that case we don't need to store a buffer in DNA at all and just have a static one instead. So we can avoid the 256 bytes for every screen.
source/blender/makesrna/intern/rna_scene.c
934–935	If the view layer is an input argument, you can't just assume that it's from the context's scene. At least it should use the scene it's called the function through (the unused `scene` arg above). Even better would be if there was proper validation if the scene - view-layer combination is valid.

I'd propose the following changes:

Use static string buffer rather than per screen DNA buffer.
Validate the view layer passed to scene.statistics() is actually from the given scene. Give an error if not.
Add assert for valid scene/view-layer combination in lower level BKE function

diff --git a/source/blender/blenkernel/BKE_scene.h b/source/blender/blenkernel/BKE_scene.h
index e3bd57e75e3..3ab923f05f6 100644
--- a/source/blender/blenkernel/BKE_scene.h
+++ b/source/blender/blenkernel/BKE_scene.h
@@ -110,6 +110,8 @@ void BKE_toolsettings_free(struct ToolSettings *toolsettings);
 struct Scene *BKE_scene_duplicate(struct Main *bmain, struct Scene *sce, eSceneCopyMethod type);
 void BKE_scene_groups_relink(struct Scene *sce);
 
+struct Scene *BKE_scene_find_from_view_layer(const struct Main *bmain,
+                                             const struct ViewLayer *layer);
 struct Scene *BKE_scene_find_from_collection(const struct Main *bmain,
                                              const struct Collection *collection);
 
diff --git a/source/blender/blenkernel/intern/scene.c b/source/blender/blenkernel/intern/scene.c
index 6f1cca619ff..1dc51c9ddae 100644
--- a/source/blender/blenkernel/intern/scene.c
+++ b/source/blender/blenkernel/intern/scene.c
@@ -1129,6 +1129,17 @@ int BKE_scene_base_iter_next(
   return iter->phase;
 }
 
+Scene *BKE_scene_find_from_view_layer(const Main *bmain, const ViewLayer *layer)
+{
+  for (Scene *scene = bmain->scenes.first; scene; scene = scene->id.next) {
+    if (BLI_findindex(&scene->view_layers, layer) != -1) {
+      return scene;
+    }
+  }
+
+  return NULL;
+}
+
 Scene *BKE_scene_find_from_collection(const Main *bmain, const Collection *collection)
 {
   for (Scene *scene = bmain->scenes.first; scene; scene = scene->id.next) {
@@ -2246,6 +2257,7 @@ static Depsgraph **scene_get_depsgraph_p(Main *bmain,
 {
   BLI_assert(scene != NULL);
   BLI_assert(view_layer != NULL);
+  BLI_assert(BKE_scene_find_from_view_layer(bmain, view_layer) == scene);
   /* Make sure hash itself exists. */
   if (allocate_ghash_entry) {
     BKE_scene_ensure_depsgraph_hash(scene);
diff --git a/source/blender/editors/include/ED_info.h b/source/blender/editors/include/ED_info.h
index ebb9807d7b9..25d239dd139 100644
--- a/source/blender/editors/include/ED_info.h
+++ b/source/blender/editors/include/ED_info.h
@@ -30,12 +30,9 @@ struct Main;
 
 /* info_stats.c */
 void ED_info_stats_clear(struct ViewLayer *view_layer);
-const char *ED_info_statusbar_string(struct Main *bmain,
-                                     struct bScreen *screen,
-                                     struct bContext *C);
+const char *ED_info_statusbar_string(struct Main *bmain, struct bContext *C);
 
 const char *ED_info_statistics_string(struct Main *bmain,
-                                      struct bScreen *screen,
                                       struct Scene *scene,
                                       struct ViewLayer *view_layer,
                                       char statusbar_flag);
diff --git a/source/blender/editors/space_info/info_stats.c b/source/blender/editors/space_info/info_stats.c
index 7175509a791..22702d84674 100644
--- a/source/blender/editors/space_info/info_stats.c
+++ b/source/blender/editors/space_info/info_stats.c
@@ -547,13 +547,15 @@ static void get_stats_string(
       info + *ofs, len - *ofs, TIP_(" | Objects:%s/%s"), stats_fmt->totobjsel, stats_fmt->totobj);
 }
 
-static const char *info_statusbar_string(
-    Main *bmain, bScreen *screen, Scene *scene, ViewLayer *view_layer, char statusbar_flag)
+static const char *info_statusbar_string(Main *bmain,
+                                         Scene *scene,
+                                         ViewLayer *view_layer,
+                                         char statusbar_flag)
 {
   char formatted_mem[15];
   size_t ofs = 0;
-  char *info = screen->statusbar_info;
-  int len = sizeof(screen->statusbar_info);
+  static char info[256];
+  int len = sizeof(info);
 
   info[0] = '\0';
 
@@ -608,16 +610,17 @@ static const char *info_statusbar_string(
   return info;
 }
 
-const char *ED_info_statusbar_string(Main *bmain, bScreen *screen, bContext *C)
+const char *ED_info_statusbar_string(Main *bmain, bContext *C)
 {
-  return info_statusbar_string(
-      bmain, screen, CTX_data_scene(C), CTX_data_view_layer(C), U.statusbar_flag);
+  return info_statusbar_string(bmain, CTX_data_scene(C), CTX_data_view_layer(C), U.statusbar_flag);
 }
 
-const char *ED_info_statistics_string(
-    Main *bmain, bScreen *screen, Scene *scene, ViewLayer *view_layer, char statusbar_flag)
+const char *ED_info_statistics_string(Main *bmain,
+                                      Scene *scene,
+                                      ViewLayer *view_layer,
+                                      char statusbar_flag)
 {
-  return info_statusbar_string(bmain, screen, scene, view_layer, statusbar_flag);
+  return info_statusbar_string(bmain, scene, view_layer, statusbar_flag);
 }
 
 static void stats_row(int col1,
diff --git a/source/blender/makesdna/DNA_screen_types.h b/source/blender/makesdna/DNA_screen_types.h
index 7ff96c5a908..e8c4d5cde20 100644
--- a/source/blender/makesdna/DNA_screen_types.h
+++ b/source/blender/makesdna/DNA_screen_types.h
@@ -68,8 +68,6 @@ typedef struct bScreen {
   /** User-setting for which editors get redrawn during anim playback. */
   short redraws_flag;
 
-  char statusbar_info[256];
-
   /** Temp screen in a temp window, don't save (like user prefs). */
   char temp;
   /** Temp screen for image render display or fileselect. */
diff --git a/source/blender/makesrna/intern/rna_scene.c b/source/blender/makesrna/intern/rna_scene.c
index b0265910979..8a0797d5900 100644
--- a/source/blender/makesrna/intern/rna_scene.c
+++ b/source/blender/makesrna/intern/rna_scene.c
@@ -924,15 +924,25 @@ static void rna_Scene_volume_update(Main *UNUSED(bmain), Scene *UNUSED(scene), P
   DEG_id_tag_update(&scene->id, ID_RECALC_AUDIO_VOLUME | ID_RECALC_SEQUENCER_STRIPS);
 }
 
-static const char *rna_Scene_statistics_string_get(Scene *UNUSED(scene),
+static const char *rna_Scene_statistics_string_get(Scene *scene,
                                                    Main *bmain,
-                                                   bContext *C,
+                                                   ReportList *reports,
                                                    ViewLayer *view_layer)
 {
-  const char statistics_status_bar_flag = STATUSBAR_SHOW_STATS | STATUSBAR_SHOW_MEMORY |
-                                          STATUSBAR_SHOW_VERSION;
-  return ED_info_statistics_string(
-      bmain, CTX_wm_screen(C), CTX_data_scene(C), view_layer, statistics_status_bar_flag);
+  const eUserpref_StatusBar_Flag statistics_status_bar_flag = STATUSBAR_SHOW_STATS |
+                                                              STATUSBAR_SHOW_MEMORY |
+                                                              STATUSBAR_SHOW_VERSION;
+
+  if (BKE_scene_find_from_view_layer(bmain, view_layer) != scene) {
+    BKE_reportf(reports,
+                RPT_ERROR,
+                "View Layer '%s' not found in scene '%s'",
+                view_layer->name,
+                scene->id.name + 2);
+    return "";
+  }
+
+  return ED_info_statistics_string(bmain, scene, view_layer, statistics_status_bar_flag);
 }
 
 static void rna_Scene_framelen_update(Main *UNUSED(bmain), Scene *scene, PointerRNA *UNUSED(ptr))
@@ -7684,7 +7694,7 @@ void RNA_def_scene(BlenderRNA *brna)
 
   /* Statistics */
   func = RNA_def_function(srna, "statistics", "rna_Scene_statistics_string_get");
-  RNA_def_function_flag(func, FUNC_USE_MAIN | FUNC_USE_CONTEXT);
+  RNA_def_function_flag(func, FUNC_USE_MAIN | FUNC_USE_REPORTS);
   parm = RNA_def_pointer(func, "view_layer", "ViewLayer", "View Layer", "");
   RNA_def_parameter_flags(parm, PROP_NEVER_NULL, PARM_REQUIRED);
   parm = RNA_def_string(func, "statistics", NULL, 0, "Statistics", "");
diff --git a/source/blender/makesrna/intern/rna_screen.c b/source/blender/makesrna/intern/rna_screen.c
index fb2a60db0fd..0bd850332f2 100644
--- a/source/blender/makesrna/intern/rna_screen.c
+++ b/source/blender/makesrna/intern/rna_screen.c
@@ -288,9 +288,11 @@ static void rna_View2D_view_to_region(
   }
 }
 
-static const char *rna_Screen_statusbar_info_get(struct bScreen *screen, Main *bmain, bContext *C)
+static const char *rna_Screen_statusbar_info_get(struct bScreen *UNUSED(screen),
+                                                 Main *bmain,
+                                                 bContext *C)
 {
-  return ED_info_statusbar_string(bmain, screen, C);
+  return ED_info_statusbar_string(bmain, C);
 }
 
 #else