blender
/
blender
136
406
Fork 589
Code Issues 6.2k Pull Requests 750 Projects 19 Wiki Activity

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
This commit is contained in:
Bastien Montagne 2020-09-30 14:49:42 +02:00
parent d74d35e39e
commit 6219d0d145
10 changed files with 91 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
source/blender/blenkernel/BKE_workspace.h
View File

@ -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;

28
source/blender/blenkernel/intern/workspace.c
View File

@ -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)

32
source/blender/blenloader/intern/readfile.c
View File

@ -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

4
source/blender/blenloader/intern/readfile.h
View File

@ -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);
void do_versions_after_linking_290(struct Main *bmain, struct ReportList *reports);
void do_versions_after_linking_cycles(struct Main *bmain);
/* This is rather unfortunate to have to expose this here, but better use that nasty hack in
* do_version than readfile itself. */
void *blo_read_get_new_globaldata_address(struct FileData *fd, const void *adr);

6
source/blender/blenloader/intern/versioning_280.c
View File

@ -241,7 +241,7 @@ static void do_version_workspaces_after_lib_link(Main *bmain)
if (screen->temp) {
/* We do not generate a new workspace for those screens...
* still need to set some data in win. */
win->workspace_hook = BKE_workspace_instance_hook_create(bmain);
win->workspace_hook = BKE_workspace_instance_hook_create(bmain, win->winid);
win->scene = screen->scene;
/* Deprecated from now on! */
win->screen = NULL;
@ -254,10 +254,10 @@ static void do_version_workspaces_after_lib_link(Main *bmain)
WorkSpaceLayout *layout = BKE_workspace_layout_find(workspace, win->screen);
BLI_assert(layout != NULL);
win->workspace_hook = BKE_workspace_instance_hook_create(bmain);
win->workspace_hook = BKE_workspace_instance_hook_create(bmain, win->winid);
BKE_workspace_active_set(win->workspace_hook, workspace);
BKE_workspace_active_layout_set(win->workspace_hook, workspace, layout);
BKE_workspace_active_layout_set(win->workspace_hook, win->winid, workspace, layout);
/* Move scene and view layer to window. */
Scene *scene = screen->scene;

27
source/blender/blenloader/intern/versioning_290.c
View File

@ -41,6 +41,7 @@
#include "DNA_rigidbody_types.h"
#include "DNA_screen_types.h"
#include "DNA_shader_fx_types.h"
#include "DNA_workspace_types.h"
#include "BKE_collection.h"
#include "BKE_colortools.h"
@ -779,5 +780,31 @@ void blo_do_versions_290(FileData *fd, Library *UNUSED(lib), Main *bmain)
*/
{
/* Keep this block, even when empty. */
if (!DNA_struct_elem_find(fd->filesdna, "WorkSpaceDataRelation", "int", "parentid")) {
LISTBASE_FOREACH (WorkSpace *, workspace, &bmain->workspaces) {
LISTBASE_FOREACH_MUTABLE (
WorkSpaceDataRelation *, relation, &workspace->hook_layout_relations) {
relation->parent = blo_read_get_new_globaldata_address(fd, relation->parent);
BLI_assert(relation->parentid == 0);
if (relation->parent != NULL) {
LISTBASE_FOREACH (wmWindowManager *, wm, &bmain->wm) {
wmWindow *win = BLI_findptr(
&wm->windows, relation->parent, offsetof(wmWindow, workspace_hook));
if (win != NULL) {
relation->parentid = win->winid;
break;
}
}
if (relation->parentid == 0) {
BLI_assert(
!"Found a valid parent for workspace data relation, but no valid parent id.");
}
}
if (relation->parentid == 0) {
BLI_freelinkN(&workspace->hook_layout_relations, relation);
}
}
}
}
}
}

2
source/blender/editors/screen/workspace_edit.c
View File

@ -141,7 +141,7 @@ bool ED_workspace_change(WorkSpace *workspace_new, bContext *C, wmWindowManager
return false;
}
BKE_workspace_active_layout_set(win->workspace_hook, workspace_new, layout_new);
BKE_workspace_active_layout_set(win->workspace_hook, win->winid, workspace_new, layout_new);
BKE_workspace_active_set(win->workspace_hook, workspace_new);
/* update screen *after* changing workspace - which also causes the

7
source/blender/makesdna/DNA_workspace_types.h
View File

@ -160,10 +160,15 @@ typedef struct WorkSpaceDataRelation {
struct WorkSpaceDataRelation *next, *prev;
/** The data used to identify the relation
* (e.g. to find screen-layout (= value) from/for a hook). */
* (e.g. to find screen-layout (= value) from/for a hook).
* Note: Now runtime only. */
void *parent;
/** The value for this parent-data/workspace relation. */
void *value;
/** Reference to the actual parent window, wmWindow->winid. Used in read/write code. */
int parentid;
char _pad_0[4];
} WorkSpaceDataRelation;
/**

2
source/blender/windowmanager/intern/wm.c
View File

@ -395,7 +395,7 @@ void wm_add_default(Main *bmain, bContext *C)
win->scene = CTX_data_scene(C);
STRNCPY(win->view_layer_name, CTX_data_view_layer(C)->name);
BKE_workspace_active_set(win->workspace_hook, workspace);
BKE_workspace_active_layout_set(win->workspace_hook, workspace, layout);
BKE_workspace_active_layout_set(win->workspace_hook, win->winid, workspace, layout);
screen->winid = win->winid;
wm->winactive = win;

8
source/blender/windowmanager/intern/wm_window.c
View File

@ -297,7 +297,7 @@ wmWindow *wm_window_new(const Main *bmain, wmWindowManager *wm, wmWindow *parent
/* Dialogs may have a child window as parent. Otherwise, a child must not be a parent too. */
win->parent = (!dialog && parent && parent->parent) ? parent->parent : parent;
win->stereo3d_format = MEM_callocN(sizeof(Stereo3dFormat), "Stereo 3D Format (window)");
win->workspace_hook = BKE_workspace_instance_hook_create(bmain);
win->workspace_hook = BKE_workspace_instance_hook_create(bmain, win->winid);
return win;
}
@ -327,7 +327,7 @@ wmWindow *wm_window_copy(Main *bmain,
layout_new = duplicate_layout ?
ED_workspace_layout_duplicate(bmain, workspace, layout_old, win_dst) :
layout_old;
BKE_workspace_active_layout_set(win_dst->workspace_hook, workspace, layout_new);
BKE_workspace_active_layout_set(win_dst->workspace_hook, win_dst->winid, workspace, layout_new);
*win_dst->stereo3d_format = *win_src->stereo3d_format;
@ -2420,7 +2420,7 @@ WorkSpaceLayout *WM_window_get_active_layout(const wmWindow *win)
}
void WM_window_set_active_layout(wmWindow *win, WorkSpace *workspace, WorkSpaceLayout *layout)
{
BKE_workspace_active_layout_set(win->workspace_hook, workspace, layout);
BKE_workspace_active_layout_set(win->workspace_hook, win->winid, workspace, layout);
}
/**
@ -2434,7 +2434,7 @@ bScreen *WM_window_get_active_screen(const wmWindow *win)
}
void WM_window_set_active_screen(wmWindow *win, WorkSpace *workspace, bScreen *screen)
{
BKE_workspace_active_screen_set(win->workspace_hook, workspace, screen);
BKE_workspace_active_screen_set(win->workspace_hook, win->winid, workspace, screen);
}
bool WM_window_is_temp_screen(const wmWindow *win)