blender
/
blender
136
404
Fork 586
Code Issues 6.2k Pull Requests 761 Projects 19 Wiki Activity

UI: Region polling support #105088

Merged
Julian Eisel merged 39 commits from JulianEisel/blender:temp-region-poll into main 2023-04-05 15:30:46 +02:00
Conversation 0 Commits 39 Files Changed 18 +20 -22
3 changed files with 20 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files
Showing only changes of commit b43b416d1f - Show all commits

6
source/blender/blenkernel/BKE_screen.h
View File

@ -164,10 +164,12 @@ typedef struct ARegionType {
void (*init)(struct wmWindowManager *wm, struct ARegion *region);
/* exit is called when the region is hidden or removed */
void (*exit)(struct wmWindowManager *wm, struct ARegion *region);
/** Optional callback to decide whether the region should be treated as existing given the
/**
JulianEisel marked this conversation as resolved Outdated
Hans Goudey commented 2023-03-03 23:16:36 +01:00
Outdated
Review
Copy Link
  /** 
   * Optional callback to decide whether the region should be treated as existing given the
   * current context. When returning false, the region will be kept in storage, but is not
   * available to the user in any way. Callbacks can assume that context has the owning area and
   * space-data set.
   */
``` /** * Optional callback to decide whether the region should be treated as existing given the * current context. When returning false, the region will be kept in storage, but is not * available to the user in any way. Callbacks can assume that context has the owning area and * space-data set. */
* Optional callback to decide whether the region should be treated as existing given the
* current context. When returning false, the region will be kept in storage, but is not
* available to the user in any way. Callbacks can assume that context has the owning area and
* space-data set. */
* space-data set.
*/
bool (*poll)(const struct bContext *C);
JulianEisel marked this conversation as resolved Outdated
Campbell Barton commented 2023-04-05 12:40:00 +02:00
Outdated
Review
Copy Link

As this function runs a lot I think it would be best to take screen variables directly, e.g. (wm, area, region) arguments and not set the context.

To expand on this, in tests with some basic interaction the region_poll function runs ~19 times on frame change with the default scene, the same on mouse wheel and mouse motion with some modal operators.
Testing some basic interactivity - this function can run 10's of thousands of times in a minute or so
While I don't see this as a problem for simple checks - passing in the necessary arguments ensures slower lookups aren't used. It's also in keeping with other callbacks which take windowing arguments.

Even in this patch the function calls for data-access aren't as directly as they might be: CTX_wm_space_clip(C) calls into CTX_wm_area(..) -> ctx_wm_python_context_get(..), then CTX_py_dict_get(C) & BLI_thread_is_main().

If you anticipate needing access to data besides the area/region in the future, a parameters struct could be used instead of multiple arguments - so the scene or window can be added without having to update the function signature for every poll() function.

As this function runs a _lot_ I think it would be best to take screen variables directly, e.g. `(wm, area, region)` arguments and not set the context. ---- To expand on this, in tests with some basic interaction the `region_poll` function runs ~19 times on frame change with the default scene, the same on mouse wheel and mouse motion with some modal operators. Testing some basic interactivity - this function can run 10's of thousands of times in a minute or so While I don't see this as a problem for simple checks - passing in the necessary arguments ensures slower lookups aren't used. It's also in keeping with other callbacks which take windowing arguments. Even in this patch the function calls for data-access aren't as directly as they might be: `CTX_wm_space_clip(C)` calls into `CTX_wm_area(..)` -> `ctx_wm_python_context_get(..)`, then `CTX_py_dict_get(C)` & `BLI_thread_is_main()`. If you anticipate needing access to data besides the area/region in the future, a parameters struct could be used instead of multiple arguments - so the scene or window can be added without having to update the function signature for every poll() function.
Julian Eisel commented 2023-04-05 13:18:46 +02:00
Review
Copy Link

Doesn't seem like a real issue, for the majority of regions there is no poll function and so region_poll() early exists before any context access. But sure, easy enough of a change.

Doesn't seem like a real issue, for the majority of regions there is no poll function and so `region_poll()` early exists before any context access. But sure, easy enough of a change.
/* draw entirely, view changes should be handled here */
void (*draw)(const struct bContext *C, struct ARegion *region);

34
source/blender/blenloader/intern/versioning_300.cc
View File

@ -2233,29 +2233,25 @@ static void version_ensure_missing_regions(ScrArea *area, SpaceLink *sl)
switch (sl->spacetype) {
case SPACE_FILE: {
ARegion *new_region;
new_region = do_versions_add_region_if_not_found(
regionbase, RGN_TYPE_UI, "versioning: UI region for file", RGN_TYPE_TOOLS);
if (new_region) {
new_region->alignment = RGN_ALIGN_TOP;
new_region->flag |= RGN_FLAG_DYNAMIC_SIZE;
if (ARegion *ui_region = do_versions_add_region_if_not_found(
regionbase, RGN_TYPE_UI, "versioning: UI region for file", RGN_TYPE_TOOLS)) {
ui_region->alignment = RGN_ALIGN_TOP;
ui_region->flag |= RGN_FLAG_DYNAMIC_SIZE;
}
new_region = do_versions_add_region_if_not_found(
regionbase, RGN_TYPE_EXECUTE, "versioning: execute region for file", RGN_TYPE_UI);
if (new_region) {
new_region->alignment = RGN_ALIGN_BOTTOM;
new_region->flag = RGN_FLAG_DYNAMIC_SIZE;
if (ARegion *exec_region = do_versions_add_region_if_not_found(
regionbase, RGN_TYPE_EXECUTE, "versioning: execute region for file", RGN_TYPE_UI)) {
exec_region->alignment = RGN_ALIGN_BOTTOM;
exec_region->flag = RGN_FLAG_DYNAMIC_SIZE;
}
new_region = do_versions_add_region_if_not_found(regionbase,
RGN_TYPE_TOOL_PROPS,
"versioning: tool props region for file",
RGN_TYPE_EXECUTE);
if (new_region) {
new_region->alignment = RGN_ALIGN_RIGHT;
new_region->flag = RGN_FLAG_HIDDEN;
if (ARegion *tool_props_region = do_versions_add_region_if_not_found(
regionbase,
RGN_TYPE_TOOL_PROPS,
"versioning: tool props region for file",
RGN_TYPE_EXECUTE)) {
tool_props_region->alignment = RGN_ALIGN_RIGHT;
tool_props_region->flag = RGN_FLAG_HIDDEN;
}
break;
}

2
source/blender/blenloader/intern/versioning_common.cc
View File

@ -66,7 +66,7 @@ ARegion *do_versions_ensure_region(ListBase *regionbase,
}
}
ARegion *new_region = static_cast<ARegion *>(MEM_callocN(sizeof(ARegion), allocname));
ARegion *new_region = MEM_cnew<ARegion>(allocname);
JulianEisel marked this conversation as resolved Outdated
Hans Goudey commented 2023-03-03 23:12:04 +01:00
Outdated
Review
Copy Link

MEM_callocN -> MEM_cnew

`MEM_callocN` -> `MEM_cnew`
new_region->regiontype = region_type;
BLI_insertlinkafter(regionbase, link_after_region, new_region);
return new_region;