Main Workspace Integration
ClosedPublic

Authored by Julian Eisel (Severin) on Jan 6 2017, 12:01 AM.

Details

Summary

Main Workspace Integration

This patch does the main integration of workspaces, which is a concept we agreed on during the 2.8 UI workshop (check the write-up).
Patch/branch based on blender2.8.

Main Changes/Features

  • Introduces the new Workspaces (general description).
  • Store screen-layouts (bScreen) per workspace.
  • Store an active screen-layout per workspace. Changing the workspace will enable this layout.
  • Store active mode in workspace. Changing the workspace will also enter the mode of the new workspace. (Note that we still store the active mode in the object, moving this completely to workspaces is a separate project.)
  • Moved mode switch from 3D View header to Info Editor header.
  • Store active scene in window (not directly workspace related, but overlaps quite a bit).
  • Removed 'Use Global Scene' User Preference option.
  • Compatibility with old files - a new workspace is created for every screen-layout of old files.
  • Default .blend only contains one workspace though ('General'). Idea is that users can add pre-configured workspaces from a menu, rather than having a bunch of default ones of which most will probably never be used.
  • Support appending workspaces.
  • Ctrl+Left and Ctrl+Right now cycle through workspaces instead of screens - not sure if that's what users want.

Also made sure opening files without UI and command-line rendering works fine.

Temporary UI for until new top-bar implementation is done.

Notes:

  • Workspaces are data-blocks.
  • Adding and removing bScreens should be done through ED_workspace_layout API now.
  • A workspace can only be visible in one window. If it could be displayed in multiple ones, all these workspaces would share the same UI data (same editors, same layout, same editor settings, etc) - which doesn't make much sense. Duplicating a window or opening an area in a new window duplicates the active workspace too.
  • The same applies to screen-layouts too. This is nothing new, current Blender already works like this.
  • The mode menu (which is now in the Info Editor header) doesn't display Grease Pencil Edit mode anymore since its availability depends on the active editor. Will be fixed by making Grease Pencil an own object type (as planned).
  • Screen-layouts (bScreen) are IDs and thus stored in a main list-base. Had to add a wrapper WorkSpaceLayout so we can store them in a list-base within workspaces, too. I think on the long run we could completely replace bScreen by workspace structs (by moving screen-layout data to WorkSpaceLayout and everything else to WorkSpace).
  • WorkSpace types should only be accessed through BKE_workspace API, added a compile time check to ensure their DNA headers are only included in DNA and BKE_workspace namespace.
  • For a really few cases, we need the scene within a area/region listener. Previously we could get it from the screen, now we pass it as parameter to the listener. Not so nice to have this for such few cases.
  • Added scene operators SCENE_OT_, was previously done through screen operators.
  • Renamed directories editors/screen/ to editors/workspace/, renamed notifier NC_SCREEN to NC_WORKSPACE (screen is part of workspace now).

Additional changes I had to make:

  • Reworked how we store custom transform orientations in 3D Views. Previously it was enough to store the index of the active orientation in a 3D View as we were always able to find out which scene a 3D View belongs to. This isn't the case now that the scene is stored in the window, because inactive workspaces don't belong to a specified window. When removing a custom orientation we still need to unset it from each 3D view that uses it though, so we need to identify it reliably. Solved by additionally storing the active custom orientation in the 3D view as a pointer, so we can always identify it correctly when removing it.
  • Added support for template_ID to use a custom list to search the IDs in. Needed so workspaces display their own list of screens, not the main list.
  • Added support for data-blocks that are either linkable, appendable or both. Needed so workspaces can be appended but not linked (a linked workspace would mean its entire UI isn't editable).

BPY API Changes

  • Removed Screen.scene, added Window.scene
  • UILayout.template_ID and UILayout.template_ID_preview now additionally take search_data and search_property as parameters to define a custom collection to search the IDs in (optional).
  • Removed UserPreferencesView.use_global_scene
  • Added Context.workspace, Window.workspace and BlendData.workspaces
  • Added bpy.types.WorkSpace containing screen (active screen), screens and object_mode

What's left?

  • Make workspaces part of 'workflow' files -
    https://code.blender.org/wp-content/uploads/2016/12/Workspaces.jpg
  • There are a few open design questions (T50521). We should find the needed answers and implement them.
  • Get the new layer system ready and make workspaces store the active layer.
  • Get the override system ready and support overrides per workspace.
  • Support custom UI setups as part of workspaces.
  • Allow enabling add-ons per workspace.
  • Support custom workspace keymaps.
  • Allow adding a pre-defined workspace from a menu (asset manager?)
  • Workspaces need an icon :)

Diff Detail

Repository
rB Blender
There are a very large number of changes, so older changes are hidden. Show Older Changes
Julian Eisel (Severin) updated this object.
Julian Eisel (Severin) updated this object.

As for the state of this: The branch appears to work rock solid now. I don't plan any more changes so feel free to test/review for merge into blender2.8 :)

The patch is too big for a throughout review (>10k lines). Any part in particular you think needs cross-checking?

Also, I was under the impression that the user's detault workspaces would be saved in a separate file. I think the patch is not covering this.

Could you create a Doc on how to use this to help with testing. Ive built a branch and compiled for others to test but unclear how to utilize it, For example you image above which shows the workspace tabs at the top of the screen, How do i create these setups? Cheers J

  • Fix compile error and warning in release builds
  • Fix and compile time option for workspace object-mode
  • Merge branch 'blender2.8' into workspaces

Sorry guyzz for letting you wait that long! (tm) Real live dared to take all my attention.

The patch is too big for a throughout review (>10k lines). Any part in particular you think needs cross-checking?

I guess for your layer work the most interesting part would be the DNA/RNA stuff which is quite small actually.

Also, I was under the impression that the user's detault workspaces would be saved in a separate file. I think the patch is not covering this.

No, it isn't yet (as mentioned in the description ;). I think we need some more discussion on this to make sure all use cases are covered. Once that's done I'm happy to do the needed changes.


@Bastien Montagne (mont29), there are a couple of things on which I'd love to hear your input on:

  • General integration of the WorkSpace ID type (esp. lib-query and such).
  • Support for appendable-only data-blocks. That'd mainly be the changes from rB18ae49948fad5.
  • Changes done to template_ID to support custom RNA collection to take IDs from, see rB387bc547cb781445.
  • Any ideas on how to implement the predefined workspaces? Plan was to have a '+' tab in the topbar to create a new workspace, spawning a list of predefined workspaces the user can choose from. This way we don't waste space with workspace-tabs that'll never be used, without dropping the ready-to-use UI presets for different tasks. I guess we could store these workspaces in the workflow files, I'm just not sure how to query them for listing in the UI. Could Amber handle this part? Or maybe the preset system could support this?
  • Any thoughts on per workspace add-ons? I guess this would mean having a bAddon list saved in workspaces which would be registered/unregistered when changing workspaces. Think that's doable but I could imagine there would be performance hiccups and such. There's also the case of opening a workspace with an add-on not available. Maybe per workspaces add-ons should be stored as overrides of the UserPref ones internally, so we can handle such cases nicely.

@Bastien Montagne (mont29) & @Dalai Felinto (dfelinto), you both may want to look into the main workspace APIs (/blenkernel/intern/workspace.c, /blenkernel/BKE_workspace.h and /editors/workspace/workspace_edit.c) and into the Python API changes. The patch description should list all changes for the latter one.


Could you create a Doc on how to use this to help with testing. Ive built a branch and compiled for others to test but unclear how to utilize it, For example you image above which shows the workspace tabs at the top of the screen, How do i create these setups? Cheers J

To be honest I don't think this should get some user testing just now. Not because I don't think it's stable (well in fact there are some known issues with the temporary implementation of workspace-level object-mode), but because I don't think it's really usable at all just now. This patch is part of a number of bigger changes that create a big picture change all together, but on it's own it isn't worth much for users.

There's T50521 now for further design discussion. The points regarding workspace sharing and multi-window setups might influence how we'll be saving data to files. So if we want to avoid breaking files created with blender2.8, we should probably hold off with the merge until questions are answered and needed changes done.

@Julian Eisel (Severin) could you please update the patch with your latest changes, and make sure the task description fully cover what is working, what is not, and what was left to be implemented separatly?

Julian Eisel (Severin) updated this revision to Diff 8581.EditedApr 10 2017, 12:13 AM
  • Support storing active render-layer per workspace.
  • Add a workflow file to store custom workspaces as part of user configuration.
  • Allow user to choose a workspace from this workflow file when adding a workspace.
  • Bundle a default workspace configuration with Blender.
  • Improve support for multi-window setups (like proposed in T50521).
  • Only ensure unique layout name within workspace.
  • Basic support for reading 2.8x files in pre-2.8 Blender.
  • Added place holder icon.
  • Of course tons of fixes, cleanups, etc.

Will update the description to reflect latest changes soon. (Would
probably make sense to create a wiki page instead)

Besides the issues mentioned below I think this is ready for merging into blender2.8 branch.


From checking the latest updates I've run into some bugs.

When I apply the patch I get the following error on blender when using userprefs.blend and startup.blend saved from 2.78c after restoring factory settings:

BLI_assert failed: /src/blender/source/blender/blenkernel/intern/workspace.c:262, BKE_workspace_layout_find(), at '!"Couldn't find layout in this workspace. This should not happen!"'

I ran into the same assertion when running blender --factory-startup, then pressing the "+" and selecting "Duplicate Current" as well as switching between workspaces.

source/blender/windowmanager/intern/wm_files.c
588

This is going to cause problems,

  • wm_init_userdef versions preferences, initializes temp dir & auto-script-execute option.
  • wm_file_read_post initializes addons, also many other final steps (load-post callbacks, initializes editors).

This means addons may run before user preferences are properly versioned and initializes, which should be avoided since it could cause strange/undefined behavior.

Besides introducing problems (that can probably be worked around), the intended purpose of wm_file_read_post is to run last, so not really happy to move it before other operations.


What problems happen when the order is restored to the original state? - I think its better to try resolve that problem on it's own.

  • Merge branch 'blender2.8' into workspaces
  • Merge branch '28' into workspaces
  • Minor changes only, use BKE prefix and remove nested header
  • App Templates: optionally use app-template path for workspaces
Campbell Barton (campbellbarton) requested changes to this revision.Apr 25 2017, 8:55 AM
This revision now requires changes to proceed.Apr 25 2017, 8:55 AM
  • Merge branch 'blender2.8' into workspaces
  • Merge branch '28' into workspaces
  • Odd use of BLI_strncpy (just use strcpy)
  • Fix buffer overrun when setting the name
  • Use printf instead of assert
  • Re-order for versioning

@Julian Eisel (Severin) there is a bug in this patch, the problem is caused by temporary windows.

To reproduce:

  • Open 2.7x, maximize any view and safe.
  • Load the file in workspaces branch, and press "Back to Previous".

The problem is that that temporary Screens get their own work workspace,
but switching out of a temporary screen, the newly activated screen doesn't exist in the current workspace.

This can be worked around without too much trouble: see P472
but it would be better not to create this situation in the first place.

I looked into a fix that doesn't create this situation to begin with, see: P473
This isn't ideal since it removes workspaces that are created by temp screens (instead of avoiding to create the in the first place).
But I couldn't see a good way to do this since they are created when we haven't yet linked library pointers.

  • Fix for crash loading a file containing a temp (maximized) screen.
  • Use Main as the first argument
  • Revert minor order change - wm_file_read_post
  • Merge branch '28' into workspaces
  • Clear win->screen pointer
  • Cleanup: formatting
  • Use DNA as a prefix
  • Use DNA as a prefix, for workspace define too
  • Use uppercase for macros
  • Disabling USE_WORKSPACE_MODE works again
  • Use iterator for for do_version_workspaces_after_lib_link
  • Merge branch '28' into workspaces

I've been attempting to prepare this branch for inclusion in 2.8x,
and fixed some issues but run into other bugs which are taking more then a day or so to fix.
Listing issues as follows:

Note that line numbers from crash logs are from: rBd0937aa54b8ff2e00e5a4993c2f9be2c74240b19

  • Duplicate Scene Crashes:
    • Open Blender with --factory-settings
    • Add a new scene
    • Save user-defaults
    • Open Blender again, crash: P475
  • Undo asserts: BLI_assert failed: source/blender/blenloader/intern/readfile.c:7198, blo_lib_link_restore(), at 'win->screen == ((void *)0)'

General concerns:

  • Using the ID-selector for workspaces feels like overloading too much functionality into this type of popup.

    I think it might be better to have a separate popup type for RNA collections, (not attempt to share so much logic).
  • Temp screens and work-spaces is weak (own doing, committed workaround), not blocker but if there was a better way to handle would be nice.

Merging into 2.8x:

I was hoping this was only some minor changes to get it ready for 2.8x branch,
but I keep running into bugs/glitches.

Overall DNA seems stable and not going to break files,
but complexity/bugs introduced mean I don't think it's so reasonable
for active developers to fix these as part of regular development.

@Julian Eisel (Severin), would you be able to check into the crashes and problems with ID-dropdown list mentioned above in some reasonable time-frame? (Say 2 weeks?).
If not - we probably need to assign someone else to work on this task.

Regarding the bugs on undo, reading of old files and such, I can recreate them, but I'm pretty sure they are caused by recent changes. I tested such simple actions regularly and I would have noticed issues like these. Will investigate.


  • Loading old files used to populate the layout popup (rB53d59af364d), now it doesn't. - Is this intended?

I'm not sure what you mean with that, mind explaining some more?

Note, seems rB7cb2f43b07500f64c791ea822fada87f57e1dae1 merge broke popup and causes writing past memory, see: P474.

I've committed re-allocation just to avoid the crash: rB9ddb857c7a9b9ea7ae55ef39879497676d6d8e71, which may be removed if the root-cause is fixed.

Is this supposed to be an own bullet point or is the issue related to the layout popup populating?
Sounds like it could be caused by rBf5bc8ad4ce8716.

This is caused by rBf5bc8ad4ce8716, see the concerns I raised there. I'll likely just revert this commit.

General concerns:

  • Using the ID-selector for workspaces feels like overloading too much functionality into this type of popup.

    I think it might be better to have a separate popup type for RNA collections, (not attempt to share so much logic).

Agree, had the same concern. I think ID-selector/templateID code could use a bit of cleanup anyway, even without adding changes from this patch to it.
I initially decided to just ignore the concern for the initial workspace merge and refactor it separately since it would be general cleanup. However, I can also prepare a patch prior to merging workspace support. It would probably split up templateID into separate templates sharing some (low-level?) code.

  • Temp screens and work-spaces is weak (own doing, committed workaround), not blocker but if there was a better way to handle would be nice.

Didn't investigate that yet, but will try to find a nice solution.

@Julian Eisel (Severin), would you be able to check into the crashes and problems with ID-dropdown list mentioned above in some reasonable time-frame? (Say 2 weeks?).

I should be available for that work the next days, yup.

source/blender/windowmanager/intern/wm_files.c
588

The issue I tried to fix with this re-ordering was a crash when loading startup.blend as regular .blend, see rB6bf890786305f.
wm_init_userdef called CTX_data_main with invalid context state.

The important bits were the calls to CTX_wm_window_set in wm_file_read_post. IIRC calling CTX_wm_window_set once more in WM_file_read would've worked too, but I checked if reordering would be fine and it seemed so, so I decided to avoid the extra CTX_wm_window_set call. You're making good points however, will check the alternative(s) again.

Regarding the bugs on undo, reading of old files and such, I can recreate them, but I'm pretty sure they are caused by recent changes. I tested such simple actions regularly and I would have noticed issues like these. Will investigate.


  • Loading old files used to populate the layout popup (rB53d59af364d), now it doesn't. - Is this intended?

I'm not sure what you mean with that, mind explaining some more?

When loading a blend file, the screens are normally converted into layouts. correction to my comment, this works for existing files, but loading factory settings I only get a "General" workspace with a "Default" layout. - Is that working as intended?

Note, seems rB7cb2f43b07500f64c791ea822fada87f57e1dae1 merge broke popup and causes writing past memory, see: P474.

I've committed re-allocation just to avoid the crash: rB9ddb857c7a9b9ea7ae55ef39879497676d6d8e71, which may be removed if the root-cause is fixed.

Is this supposed to be an own bullet point or is the issue related to the layout popup populating?

No, just commenting that a workaround was committed for P474 error that can be revered if this id-template code is improved.

Sounds like it could be caused by rBf5bc8ad4ce8716.

Seems likely.

This is caused by rBf5bc8ad4ce8716, see the concerns I raised there. I'll likely just revert this commit.

Agree this should probably be reverted.

General concerns:

  • Using the ID-selector for workspaces feels like overloading too much functionality into this type of popup.

    I think it might be better to have a separate popup type for RNA collections, (not attempt to share so much logic).

Agree, had the same concern. I think ID-selector/templateID code could use a bit of cleanup anyway, even without adding changes from this patch to it.
I initially decided to just ignore the concern for the initial workspace merge and refactor it separately since it would be general cleanup. However, I can also prepare a patch prior to merging workspace support. It would probably split up templateID into separate templates sharing some (low-level?) code.

While it is a bit out-of-scope for the workspaces branch, any issues here currently aren't real easy to troubleshoot (was looking into red-alert bug).
So would prefer if this area was cleaned up a bit before merge.

  • Temp screens and work-spaces is weak (own doing, committed workaround), not blocker but if there was a better way to handle would be nice.

Didn't investigate that yet, but will try to find a nice solution.

@Julian Eisel (Severin), would you be able to check into the crashes and problems with ID-dropdown list mentioned above in some reasonable time-frame? (Say 2 weeks?).

I should be available for that work the next days, yup.

Great :)

When loading a blend file, the screens are normally converted into layouts. correction to my comment, this works for existing files, but loading factory settings I only get a "General" workspace with a "Default" layout. - Is that working as intended?

Ah that! Yes, this is intended: Idea was to only have the "General" workspace with the "Default" Layout in the default startup.blend. The other default workspaces ("Animation", "Scripting", "Motion Tracking", ...) could be added via the '+' button (meaning the default workspaces.blend would contain these). That way we don't waste space in the topbar with workspace-tabs that the user will never need, but we still have a number of ready-to-use workspaces bundled with Blender.
When opening pre-2.8 .blends we still convert layouts to workspaces, the only exception is the default startup.blend. (See changes in versioning_defaults.c.)
I should really write such things down in a wiki page... :S

  • Fix unfreed IDProperties, caused by branch merges
  • Fix crash loading pre-2.80 .blends
  • Fix crash when loading .blend with multiple scenes
  • Fix failing assert on undo
  • Fix (harmless) error print with multiple workspaces
  • Fix crash opening startup.blend as regular .blend (again)
  • Fix empty default workspace configuration
  • Better fix for reading fullscreens from old files
  • Revert redundant workaround for buffer overflow

I think all mentioned issues are fixed now. FWIW, most issues were indeed caused by merges or recent changes. I also committed a better fix for the fullscreen window issue, we do all workspace versioning after lib-linking now.
Only thing remaining is cleanup of template_ID which I'll do separately (but so that it's ready before this goes to blender2.8).

Bastien Montagne (mont29) requested changes to this revision.May 12 2017, 7:47 PM

Eeeeh, this is really huge patch, only did a very quick overview of it, so please excuse me in advance if I made some bad misunderstanding here… I focused on areas I know best (readfile and BKE's ID management), and I have three main points of unhappiness here. :/

I) Datablocks pointing to sub-data from other datablocks.

Referring to bScreen custom_orientation pointer to Scene's transform spaces. This is a show-stopper for me, am already very unhappy when I see runtime-only similar stuff (Armature's bone pointer of Object's PoseChannels), but trying to save and load that kind of relationship is a no-go. If you want to reference data across IDs, you do it the shallow way (i.e. reference the other ID itself, and store a name or index to the desired sub-data item). Cross-ID sub-data pointers should be avoided at all cost, and when really totally necessary, they should be strictly runtime only.

II) Linkable vs. Appendable

The least controversial/easiest issue to solve imho. Basically, I don’t think we need or want that distinction at all, mainly because:

  1. It will be a nightmare to merge with current assets work (OK, lame reason, but still… :/ ).
  2. It’s making a piece of code already rather hairy and touchy, even more complicated (kinda related to previous point in fact).
  3. It’s useless, imho. OK, linked workspace would not be editable - but that’s already the case of pretty much all our linked data (only exception are proxies of linked objects). And that’s precisely what static override aims at fixing, too.

Am not happy at the idea of adding more complexity to our append/link code just to prevent people doing something (currently) useless, but also harmless.

III) Private DNA

This is not really a problem with the code, more a philosophical issue. If we want to allow that kind of 'private C++ like' handling of struct in our C core code, think this should at the very least be discussed and approved by whole core developers team. This is a very important change to what we have been doing so far, and I’m personally fully opposed to it. Adding comments in DNA file stating that one should use BKE API to access the data and not modify it directly is totally fine. Enforcing that behavior to the point of adding a dummy public handler, and accessors to get ID or ID name from it, that’s… eeeh… can’t say how much it makes me unhappy. I do not want that in code I’m susceptible to maintain.

This should probably not be discussed here though, think it’s worth a mail to bf-committers - it’s like a giant change to our code-style and philosophy so far in core Blender code!


Sorry for so much unhappiness, again this is just striking me from quick overview of patch. Besides those points (and few minor ones noted in comments below), can’t say I fully grasped the whole beast in just two hours of reading code (and playing a bit with a build of it), but overall looks good to me.

source/blender/blenkernel/intern/idcode.c
53–54

Why switch from easy-to-extend flags to two booleans? What’s wrong with adding mere #define IDTYPE_FLAGS_ISAPPENDABLE (1 << 1) ?

60

i18n, not i19n :p

source/blender/blenkernel/intern/library.c
586

I’d rather really see copying of all IDs supported from here, this is important for future asset management (and generally for powerful and robust data-blocks management). We can consider it a TODO for now, I guess…

979–981

Not much happy with this, don’t see the point of having custom allocators doing exactly the same thing as all others above… Functions like BKE_libblock_alloc_notest() are precisely here to avoid scattering and duplicating that kind of generic helpers all over our BKE code. When specific handling is required it's fine to delegate to relevant BKE code, but here it makes no sense and only adds another level of useless call.

source/blender/blenkernel/intern/workspace.c
336–339

Seriously? Do we want that kind of C++-like crap in our code?

Absolutely everywhere in Blender code we do either (ID *)my_random_datablock or &my_random_datablock->id, why do we start adding that kind of absolutely useless accessors now?

Same for the three ones below…

source/blender/blenloader/intern/readfile.c
2787–2809

You are missing two things here I think?

  1. You do not check ID's tag LIB_TAG_NEED_LINK (nor clear it at the end).
  2. You do not link IDProperties (id->properties).
6060–6073

Eeeeh… this sounds hairy, but am pretty sure you cannot do that!

If I get things right, here you are relinking data from other ID (i.e. v3d->custom_orientation points to data from Scene ID)? But direct_link should never ever expect any other ID data to be available at that time (if data of other ID is in same .blend file, it should work, though still being risky, but if data is from a datablock from another linked .blend file it will fail for sure).

In fact, you should never link to data from other datablock (ideally, never in any case, but at the very least never in .blend file context). E.g. PoseChannels' link to armature bones is already a total PITA to handle in data management code, and you'll notice that in read code we just nullify it (and restore during first armature pose evaluation later).

source/blender/imbuf/intern/thumbs_blend.c
35

Why is this needed???? Or is it a left-over from some merge?

This revision now requires changes to proceed.May 12 2017, 7:47 PM

Regarding having private struct members, I'm also not really a fan of how this patch implements it. Especially when it comes to id/next/prev. Having special looping macros (just for workspaces) seems adding too much overhead.

OTOH keeping some struct member access private can be useful.
Suggest to use something like DNA_DEPRECATED (DNA_PRIVATE_WORKSPACE ?). to mark some members as only accessible for files with private access defined.

I) Datablocks pointing to sub-data from other datablocks.


Referring to bScreen custom_orientation pointer to Scene's transform spaces. This is a show-stopper for me, am already very unhappy when I see runtime-only similar stuff (Armature's bone pointer of Object's PoseChannels), but trying to save and load that kind of relationship is a no-go. If you want to reference data across IDs, you do it the shallow way (i.e. reference the other ID itself, and store a name or index to the desired sub-data item). Cross-ID sub-data pointers should be avoided at all cost, and when really totally necessary, they should be strictly runtime only.

I don't really like this either. Thing is that the custom orientations never should've been stored in the scene but in the screen IMHO... I don't see why you would want to make it scene data (rBd660e293650d doesn't give any info on that either).

Probably the right thing to do would be moving the transform orientations to bScreen now (or directly to WorkSpaceLayout, since it should replace bScreen at some point?). Should be possible to do without compatibility issues.

Why this is needed: So far it was enough to store an index for the active transform orientation (View3D->twmode), but that only worked because the active scene was part of the screen. Now that the scene is stored per window, we don't know which scene will be shown in a screen when it gets activated, so the index isn't enough to identify the transform-orientation, which we need to do when deleting one.

II) Linkable vs. Appendable


The least controversial/easiest issue to solve imho. Basically, I don’t think we need or want that distinction at all, mainly because:

  1. It will be a nightmare to merge with current assets work (OK, lame reason, but still… :/ ).

Don't think this is a lame reason at all. I know the pain :S

  1. It’s making a piece of code already rather hairy and touchy, even more complicated (kinda related to previous point in fact).
  2. It’s useless, imho. OK, linked workspace would not be editable - but that’s already the case of pretty much all our linked data (only exception are proxies of linked objects). And that’s precisely what static override aims at fixing, too.

I think this is a pretty important thing for users, why would they want to link in a UI (which is basically what workspaces define) that is non-editable?
But also, if we allowed linking UI data (without making it local), we would open up a big can of worms technically: The entire UI code assumes its data is local, there are no ID_IS_LINKED_DATABLOCK checks to forbid editing the UI. I tested linked-in workspaces some time ago, and IIRC, it behaved really glitchy, there were errors everywhere.
And then there are things like background images, they would be linked too.

I guess we could try to work around both, the technical and UI issues with overrides or so. However, it sounds like complicating things again, just at a different place. And I don't think the changes I did here add that much complexity to the ID system.

What we could do is telling the user that workspaces can't be linked when trying to do so, and give them the choice to append instead. ("Linked workspaces would not allow editing the UI, so they can only be appended. Do you want to append them instead? [Append] [Cancel]")
I think we could implement that as special exception in WM_OT_link, instead of making it a general ID feature.

III) Private DNA


This is not really a problem with the code, more a philosophical issue. If we want to allow that kind of 'private C++ like' handling of struct in our C core code, think this should at the very least be discussed and approved by whole core developers team. This is a very important change to what we have been doing so far, and I’m personally fully opposed to it. Adding comments in DNA file stating that one should use BKE API to access the data and not modify it directly is totally fine. Enforcing that behavior to the point of adding a dummy public handler, and accessors to get ID or ID name from it, that’s… eeeh… can’t say how much it makes me unhappy. I do not want that in code I’m susceptible to maintain.

This should probably not be discussed here though, think it’s worth a mail to bf-committers - it’s like a giant change to our code-style and philosophy so far in core Blender code!

I agree we should discuss this in bf-committers, but let me just say that this is more of an experiment to see if such a code design is beneficial for us.
Obviously keeping certain struct members private and accessing them through abstractions brings some benefits, but since this is C it's an all or nothing situation (all public vs. all private). We could do the DNA_DEPRECATED thing Campbell suggested, but it's not that nice either since I'd add it to almost all struct members.
The workspace design would take a quite big role in the UI code and we have to write it from scratch - so for me this seems like the perfect case to try such a thing. Just to see how it works out in regular dev work.

I agree (and even planned to change) that things like BKE_workspace_id_get, BKE_workspace_next_get, etc are weird. It's fine to just cast them to ID or Link instead, so we can get rid of these accessors.


Sorry for so much unhappiness, again this is just striking me from quick overview of patch.

No worries, that's why we asked you for review after all ;)

source/blender/blenkernel/intern/idcode.c
53–54

Simple practical reason: Lines of idtypes[] initialization below were getting a bit too long. Adding line breaks just made it look a bit crowded and harder to grasp on first view.
Can also just switch back to using flags, don't mind really, you decide ;)

source/blender/blenkernel/intern/library.c
979–981

It's just needed because sizeof(CacheFile) is unknown here. BKE_workspace_alloc has a comment telling to only use this in BKE_libblock_alloc_notest.
I was thinking it would be nice to have a bke_intern.h for such BKE internal functions. Pretty sure there are already several cases where it could be used.

source/blender/blenloader/intern/readfile.c
2787–2809

Yes indeed.

source/blender/imbuf/intern/thumbs_blend.c
35

Getting a compile error without it, but it's in BLO_readfile so can easily avoid adding this here.

Re: Private DNA

We could do the DNA_DEPRECATED thing Campbell suggested, but it's not that nice either since I'd add it to almost all struct members.

Agree its not that nice but it's a tradeoff - some minor awkwardness in putting DNA_PRIVATE_* for private members is by far less hassle then creating an exceptional case for how basic operations are performed throughout the rest of the code base.

Currently its ~4 members - even if its 20 or so... its not a big problem to add specifiers to each.

A less verbose alternative could be to group private members in a struct.

#ifdef DNA_PRIVATE_WORKSPACE
typedef struct WorkSpaceInternal {
    ....
} WorkSpaceInternal;
#endif

Then code in BKE needs to reference workspace->internal.*. While a bit verbose it has the advantage that its obvious when private members are being accessed. (you won't auto-complete a member only to find its private)
Although it has a big disadvantage that changing a struct members private/public state will break the file-format.

All things considered still find the DNA_PRIVATE_* option gives the least friction, since it just enforces the convention instead of hiding data and forcing workarounds for typical usage.

It's a closer match to C++/Java/Rust - where you have control over individual members.

It allows gradual use - so could be added to other DNA types (object, mesh, scene... etc) which we're realistically not going to do if it means having an all-or-nothing approach to private struct members.

I) Transform Spaces: Yes, think we should move them to WorkSpaceLayout then - it’s a tiny bit annoying (often custom orientations are defined from some object in the scene or so), but still much much better than having random cross-ID data pointer in our files!

II) Append/Link can’t we just decide to not make WS linkable at all for now? And bring it back once we have static overrides (we can even define some props to be forcefully always overridden if keeping them linked is a problem - but kind of doubt we need that, by definition static overrides allows to edit linked data as if it was local).

III) Am still not much happy at the idea of private DNA data, even on per-member basis (e.g. often you can safely read data, but need special process when modifying it, etc.). Tend to consider developer knows how to read comments in code, and knows what s.he is doing. But could live with DNA_PRIVATE_* if we really want to go that way - it remains much better than current solution ;) Also, we can tackle that after merging, as long as we do it soon. Should not affect much most of the code.

Points I and II need to be addressed before we merge though, @Julian Eisel (Severin) do you have time to do that in next day or so, or I can handle it too if you prefer?

Re: Private DNA
[...]
It's a closer match to C++/Java/Rust - where you have control over individual members.

It allows gradual use - so could be added to other DNA types (object, mesh, scene... etc) which we're realistically not going to do if it means having an all-or-nothing approach to private struct members.

Esp. the gradual use point is pretty convincing to me. So I would be fine with the DNA_PRIVATE_* idea.

II) Append/Link can’t we just decide to not make WS linkable at all for now? And bring it back once we have static overrides (we can even define some props to be forcefully always overridden if keeping them linked is a problem - but kind of doubt we need that, by definition static overrides allows to edit linked data as if it was local).

You mean not making it linkable nor appendable? Not sure about that, it would break the whole idea of the worspaces.blend. Even if it's just temporary, would rather avoid it. Also, I think static overrides still need some work until they're ready?
I'd still prefer having special exceptions in WM_OT_link for workspaces. At least for until we know how linking/appending workspaces should work for users (allow linking with static overrides vs. forbid linking vs. explicitly ask to append when trying to link ...).

Points I and II need to be addressed before we merge though, @Julian Eisel (Severin) do you have time to do that in next day or so, or I can handle it too if you prefer?

I should be able to tackle remaining points within the next 1-3 days. We just need to agree on what to do with workspace link/append so I can work on that too ;)

II) Append/Link can’t we just decide to not make WS linkable at all for now? And bring it back once we have static overrides (we can even define some props to be forcefully always overridden if keeping them linked is a problem - but kind of doubt we need that, by definition static overrides allows to edit linked data as if it was local).

You mean not making it linkable nor appendable? Not sure about that, it would break the whole idea of the worspaces.blend. Even if it's just temporary, would rather avoid it. Also, I think static overrides still need some work until they're ready?
I'd still prefer having special exceptions in WM_OT_link for workspaces. At least for until we know how linking/appending workspaces should work for users (allow linking with static overrides vs. forbid linking vs. explicitly ask to append when trying to link ...).

Am also fine with adding exception in WM_OT_link operator, yes - though it’s probably a bit more work than just preventing that ID to be linkable (that’s another reason why I dislike current 'appendable but not linkable' solution - by definition, append is link + make local, so forbidding the later while allowing the former is meeh ;) ), it remains nicely confined and easy to remove once we are ready.

Points I and II need to be addressed before we merge though, @Julian Eisel (Severin) do you have time to do that in next day or so, or I can handle it too if you prefer?

I should be able to tackle remaining points within the next 1-3 days. We just need to agree on what to do with workspace link/append so I can work on that too ;)

Great, think we are all cleared now. :)

Added a patch for moving custom transform orientations from scene to workspace level, D2687.
We were talking about moving it to screen level previously, but realized this would be too granular for users (as in - having to set it up for each screen-layout could become annoying). Having it in workspace seemed to make more sense to me.

I'll remove some changes from this patch regarding transform orientations to reduce diff noise a bit.

  • Use template_search for selecting screen-layout and render-layer
  • Fix crash when renaming workspace render layer
  • Fix crash when appending workspace
  • Remove incorrext XXX mark
  • Fix missing ID handling on lib-linking
  • Fix typo in comment
  • Correct error in version patch
  • Remove linkable vs appendable distinction in data-blocks.
  • Refactor workspace data accessing

So quick update...
All review points should be addressed. The diff is a bit smaller now ;)
After latest patch update, custom transform orientations may behave a bit glitchy but that is kind of intentionally. D2687 should fix these by moving transform orientation from scene to workspace level. It should be applied right after this.

Also, thanks a lot for rB1474cab63e1d @Bastien Montagne (mont29)! There's probably an issue with it after appending workspaces: half of the UI is grayed out then. Seems like it's only linking, not running 'make-local'. Probably an easy fix, will have a look. Just mentioning since this is currently my last TODO remaining for the merge.

source/blender/imbuf/intern/thumbs_blend.c
35

Turns out that even though this can be avoided, it's probably preferable like I did it.
This file includes BLO_readfile.h which now contains ListBase workspaces as struct member. Doing struct ListBase there won't work because size is unknown on compile time. So the options are including BLI_listbase.h in BLO_readfile.h or doing what I've done here (which is what I prefer).

Also, thanks a lot for rB1474cab63e1d @Bastien Montagne (mont29)! There's probably an issue with it after appending workspaces: half of the UI is grayed out then. Seems like it's only linking, not running 'make-local'. Probably an easy fix, will have a look. Just mentioning since this is currently my last TODO remaining for the merge.

Issue was Screen data-blocks not being linkable, hence they were skipped during makelocal process… will commit fix, hopefully it does not break anything else! :P

Besides that, think the whole beast is ready for master Blender2.8!

source/blender/imbuf/intern/thumbs_blend.c
35

Cleaned up a bit, but indeed looks like we have no choice but to link listbase here… no big deal anyway.

This revision is now accepted and ready to land.Jun 1 2017, 1:22 PM
This revision was automatically updated to reflect the committed changes.