[Bf-blender-cvs] [6219d0d145d] master: Fix (unreported) design flow in how workspace's relation data are read from .blend file.
Bastien Montagne
noreply at git.blender.org
Fri Oct 2 11:49:21 CEST 2020
Commit: 6219d0d145d3f2e6bd30be1c91e952e18db44e74
Author: Bastien Montagne
Date: Wed Sep 30 14:49:42 2020 +0200
Branches: master
https://developer.blender.org/rB6219d0d145d3f2e6bd30be1c91e952e18db44e74
Fix (unreported) design flow in how workspace's relation data are read from .blend file.
Relying on pointer addresses across different data-blocks is extremely
not recommended (and should be strictly forbidden ideally), in
particular in direct_link step of blend file reading.
- It assumes a specific order in reading of data, which is not ensured
in future, and is in any case a very bad, non explicit, hidden
dependency on behaviors of other parts of the codebase.
- It is intrinsically unsafe (as in, it makes writing bad code and making
mistakes easy, see e.g. fix in rB84b3f6e049b35f9).
- It makes advanced handling of data-blocks harder (thinking about
partial undo code e.g., even though in this specific case it was not
an issue as we do not re-read neither windowmanagers nor worspaces
during undo).
New code uses windows' `winid` instead as 'anchor' to find again proper
workspace hook in windows at read time.
As a bonus, it will also cleanup the list of relations from any invalid
ones (afaict it was never done previously).
Differential Revision: https://developer.blender.org/D9073
===================================================================
M source/blender/blenkernel/BKE_workspace.h
M source/blender/blenkernel/intern/workspace.c
M source/blender/blenloader/intern/readfile.c
M source/blender/blenloader/intern/readfile.h
M source/blender/blenloader/intern/versioning_280.c
M source/blender/blenloader/intern/versioning_290.c
M source/blender/editors/screen/workspace_edit.c
M source/blender/makesdna/DNA_workspace_types.h
M source/blender/windowmanager/intern/wm.c
M source/blender/windowmanager/intern/wm_window.c
===================================================================
diff --git a/source/blender/blenkernel/BKE_workspace.h b/source/blender/blenkernel/BKE_workspace.h
index 5ff1ba2c6f5..82a4e5fe08e 100644
--- a/source/blender/blenkernel/BKE_workspace.h
+++ b/source/blender/blenkernel/BKE_workspace.h
@@ -36,7 +36,8 @@ struct bToolRef;
struct WorkSpace *BKE_workspace_add(struct Main *bmain, const char *name);
void BKE_workspace_remove(struct Main *bmain, struct WorkSpace *workspace);
-struct WorkSpaceInstanceHook *BKE_workspace_instance_hook_create(const struct Main *bmain);
+struct WorkSpaceInstanceHook *BKE_workspace_instance_hook_create(const struct Main *bmain,
+ const int winid);
void BKE_workspace_instance_hook_free(const struct Main *bmain,
struct WorkSpaceInstanceHook *hook);
@@ -83,11 +84,13 @@ void BKE_workspace_active_set(struct WorkSpaceInstanceHook *hook,
struct WorkSpaceLayout *BKE_workspace_active_layout_get(const struct WorkSpaceInstanceHook *hook)
GETTER_ATTRS;
void BKE_workspace_active_layout_set(struct WorkSpaceInstanceHook *hook,
+ const int winid,
struct WorkSpace *workspace,
struct WorkSpaceLayout *layout) SETTER_ATTRS;
struct bScreen *BKE_workspace_active_screen_get(const struct WorkSpaceInstanceHook *hook)
GETTER_ATTRS;
void BKE_workspace_active_screen_set(struct WorkSpaceInstanceHook *hook,
+ const int winid,
struct WorkSpace *workspace,
struct bScreen *screen) SETTER_ATTRS;
diff --git a/source/blender/blenkernel/intern/workspace.c b/source/blender/blenkernel/intern/workspace.c
index 324c8db0fe9..775d83278e9 100644
--- a/source/blender/blenkernel/intern/workspace.c
+++ b/source/blender/blenkernel/intern/workspace.c
@@ -125,10 +125,14 @@ static WorkSpaceLayout *workspace_layout_find_exec(const WorkSpace *workspace,
return BLI_findptr(&workspace->layouts, screen, offsetof(WorkSpaceLayout, screen));
}
-static void workspace_relation_add(ListBase *relation_list, void *parent, void *data)
+static void workspace_relation_add(ListBase *relation_list,
+ void *parent,
+ const int parentid,
+ void *data)
{
WorkSpaceDataRelation *relation = MEM_callocN(sizeof(*relation), __func__);
relation->parent = parent;
+ relation->parentid = parentid;
relation->value = data;
/* add to head, if we switch back to it soon we find it faster. */
BLI_addhead(relation_list, relation);
@@ -139,11 +143,15 @@ static void workspace_relation_remove(ListBase *relation_list, WorkSpaceDataRela
MEM_freeN(relation);
}
-static void workspace_relation_ensure_updated(ListBase *relation_list, void *parent, void *data)
+static void workspace_relation_ensure_updated(ListBase *relation_list,
+ void *parent,
+ const int parentid,
+ void *data)
{
- WorkSpaceDataRelation *relation = BLI_findptr(
- relation_list, parent, offsetof(WorkSpaceDataRelation, parent));
+ WorkSpaceDataRelation *relation = BLI_listbase_bytes_find(
+ relation_list, &parentid, sizeof(parentid), offsetof(WorkSpaceDataRelation, parentid));
if (relation != NULL) {
+ relation->parent = parent;
relation->value = data;
/* reinsert at the head of the list, so that more commonly used relations are found faster. */
BLI_remlink(relation_list, relation);
@@ -151,7 +159,7 @@ static void workspace_relation_ensure_updated(ListBase *relation_list, void *par
}
else {
/* no matching relation found, add new one */
- workspace_relation_add(relation_list, parent, data);
+ workspace_relation_add(relation_list, parent, parentid, data);
}
}
@@ -219,13 +227,13 @@ void BKE_workspace_remove(Main *bmain, WorkSpace *workspace)
BKE_id_free(bmain, workspace);
}
-WorkSpaceInstanceHook *BKE_workspace_instance_hook_create(const Main *bmain)
+WorkSpaceInstanceHook *BKE_workspace_instance_hook_create(const Main *bmain, const int winid)
{
WorkSpaceInstanceHook *hook = MEM_callocN(sizeof(WorkSpaceInstanceHook), __func__);
/* set an active screen-layout for each possible window/workspace combination */
for (WorkSpace *workspace = bmain->workspaces.first; workspace; workspace = workspace->id.next) {
- BKE_workspace_active_layout_set(hook, workspace, workspace->layouts.first);
+ BKE_workspace_active_layout_set(hook, winid, workspace, workspace->layouts.first);
}
return hook;
@@ -483,11 +491,12 @@ WorkSpaceLayout *BKE_workspace_active_layout_for_workspace_get(const WorkSpaceIn
* WorkSpaceInstanceHook.act_layout should only be modified directly to update the layout pointer.
*/
void BKE_workspace_active_layout_set(WorkSpaceInstanceHook *hook,
+ const int winid,
WorkSpace *workspace,
WorkSpaceLayout *layout)
{
hook->act_layout = layout;
- workspace_relation_ensure_updated(&workspace->hook_layout_relations, hook, layout);
+ workspace_relation_ensure_updated(&workspace->hook_layout_relations, hook, winid, layout);
}
bScreen *BKE_workspace_active_screen_get(const WorkSpaceInstanceHook *hook)
@@ -495,12 +504,13 @@ bScreen *BKE_workspace_active_screen_get(const WorkSpaceInstanceHook *hook)
return hook->act_layout->screen;
}
void BKE_workspace_active_screen_set(WorkSpaceInstanceHook *hook,
+ const int winid,
WorkSpace *workspace,
bScreen *screen)
{
/* we need to find the WorkspaceLayout that wraps this screen */
WorkSpaceLayout *layout = BKE_workspace_layout_find(hook->active, screen);
- BKE_workspace_active_layout_set(hook, workspace, layout);
+ BKE_workspace_active_layout_set(hook, winid, workspace, layout);
}
const char *BKE_workspace_layout_name_get(const WorkSpaceLayout *layout)
diff --git a/source/blender/blenloader/intern/readfile.c b/source/blender/blenloader/intern/readfile.c
index 1072cd3686e..aa2f103c693 100644
--- a/source/blender/blenloader/intern/readfile.c
+++ b/source/blender/blenloader/intern/readfile.c
@@ -1794,7 +1794,7 @@ static void *newdataadr_no_us(FileData *fd, const void *adr)
}
/* direct datablocks with global linking */
-static void *newglobadr(FileData *fd, const void *adr)
+void *blo_read_get_new_globaldata_address(FileData *fd, const void *adr)
{
return oldnewmap_lookup_and_inc(fd->globmap, adr, true);
}
@@ -2553,6 +2553,21 @@ static void lib_link_workspaces(BlendLibReader *reader, WorkSpace *workspace)
{
ID *id = (ID *)workspace;
+ /* Restore proper 'parent' pointers to relevant data, and clean up unused/invalid entries. */
+ LISTBASE_FOREACH_MUTABLE (WorkSpaceDataRelation *, relation, &workspace->hook_layout_relations) {
+ relation->parent = NULL;
+ LISTBASE_FOREACH (wmWindowManager *, wm, &reader->main->wm) {
+ LISTBASE_FOREACH (wmWindow *, win, &wm->windows) {
+ if (win->winid == relation->parentid) {
+ relation->parent = win->workspace_hook;
+ }
+ }
+ }
+ if (relation->parent == NULL) {
+ BLI_freelinkN(&workspace->hook_layout_relations, relation);
+ }
+ }
+
LISTBASE_FOREACH_MUTABLE (WorkSpaceLayout *, layout, &workspace->layouts) {
BLO_read_id_address(reader, id->lib, &layout->screen);
@@ -2581,12 +2596,8 @@ static void direct_link_workspace(BlendDataReader *reader, WorkSpace *workspace)
BLO_read_list(reader, &workspace->tools);
LISTBASE_FOREACH (WorkSpaceDataRelation *, relation, &workspace->hook_layout_relations) {
- /* data from window - need to access through global oldnew-map */
- /* XXX This is absolutely not acceptable. There is no acceptable reasons to mess with other
- * ID's data in read code, and certainly never, ever in `direct_link_` functions.
- * Kept for now because it seems to work, but it should be refactored. Probably store and use
- * window's `winid`, just like it was already done for screens? */
- relation->parent = newglobadr(reader->fd, relation->parent);
+ /* parent pointer does not belong to workspace data and is therefore restored in lib_link step
+ * of window manager.*/
BLO_read_data_address(reader, &relation->value);
}
@@ -5472,8 +5483,9 @@ static void direct_link_windowmanager(BlendDataReader *reader, wmWindowManager *
WorkSpaceInstanceHook *hook = win->workspace_hook;
BLO_read_data_address(reader, &win->workspace_hook);
- /* we need to restore a pointer to this later when reading workspaces,
- * so store in global oldnew-map. */
+ /* We need to restore a pointer to this later when reading workspaces,
+ * so store in global oldnew-map.
+ * Note that this is only needed for versionning of older .blend files now.. */
oldnewmap_insert(reader->fd->globmap, hook, win->workspace_hook, 0);
direct_link_area_map(reader, &win->global_areas);
@@ -6847,7 +6859,7 @@ static BHead *read_global(BlendFileData *bfd, FileData *fd, BHead *bhead)
/* note, this has to be kept for reading older files... */
static void link_global(FileData *fd, BlendFileData *bfd)
{
- bfd->cur_view_layer = newglobadr(fd, bfd->cur_view_layer);
+ bfd->cur_view_layer = blo_read_get_new_globaldata_address(fd, bfd->cur_view_layer);
bfd->curscreen = newlibadr(fd, NULL, bfd->curscreen);
bfd->curscene = newlibadr(fd, NULL, bfd->curscene);
// this happens in files older than 2.35
diff --git a/source/blender/blenloader/intern/readfile.h b/source/blender/blenloader/intern/readfile.h
index 2ddf96a2d47..d3372a646a9 100644
--- a/source/blender/blenloader/intern/readfile.h
+++ b/source/blender/blenloader/intern/readfile.h
@@ -203,3 +203,7 @@ void do_versions_after_linking_270(struct Main *bmain);
void do_versions_after_linking_280(struct Main *bmain, struct ReportList *reports);
@@ Diff output truncated at 10240 characters. @@
More information about the Bf-blender-cvs
mailing list