Page MenuHome

Bring engines to workspace and UI sanitize
ClosedPublic

Authored by Dalai Felinto (dfelinto) on Sep 14 2017, 12:46 PM.

Details

Summary
  • Fix lack of update in edit mode with Cycles
  • Use engine from context/render in more places
  • Fix engine context in Cycles
  • Store engine_name in render struct and use it for render preview
  • Workspace/Engine: Create BKE_render_* calls equivalent to BKE_scene_*
  • Fix UI compatibility to use context.engine
  • Use workspace engine for draw manager "render preview"
  • Expose scene > engine in the UI (render panel)
  • Use workspace -> engine in the opengl render
  • Use workspace -> engine in the viewport
  • Collection Panel: Get mode from workspace
  • Context: Expose "engine" for Python and add BKE_render_engine_get()
  • Properties Editor: Add Workspace and organize context path
  • Workspace: Add (render) engines
  • Workspace: SETLOOPER should get render layer from workspace
  • Pass workspace for space listeners
  • Workspace: "use_scene_settings" flag

Diff Detail

Repository
rB Blender

Event Timeline

Making engines workspace data is surprising to me, was there some design discussion or document about this that I missed?

It seems wrong to me that loading a file without UI could change the render engine. If we think of a render engine as just a draw mode that may be ok, but it's more than that and also defines which panels are visible, what happens when you press F12, preview renders, etc.

If this is meant to make render engines easier to use as a draw mode, then why not have this as a setting per 3D view? We want to support different draw modes in different 3D views in the same workspace, or not? As we discussed I think draw modes and render engines should be different concepts though.

I'd like to see if we could avoid passing workspaces around if its not needed since this is quite a high-level window/screen level data.

We could for example - add engine member to the EvaluationContext and pass this instead since it already has a SceneLayer member.
If it's NULL it can use a fallback (which is nicer than defining a workspace argument, then passing it as NULL in many places IMHO).

No matter how its handled, here ar some functions I think should avoid taking workspace args.

  • ED_view3d_draw_offscreen_imbuf_simple (in general low level drawing utilities). Suggest they take an engine argument instead.
  • SETLOOPER - of ~19 uses the workspace argument is only used once by an RNA update callback.

    Suggest leaving existing macro as-is and add a SETLOOPER_SCENE_LAYER macro which can take a SceneLayer instead of a workspace.
  • ED_preview_icon_render - these should probably use the scene's engine? Assuming they wont be cleared for updating when the engine is changed. This is more library level than window/screen, so I don't think current workspace engine should influence previews.

Making engines workspace data is surprising to me, was there some design discussion or document about this that I missed?

Back in February I made this diagram based on Ton's ideas for workspaces. I think it was around the time we were reviewing workspaces. I don't remember when/if it was posted in a mailing list:

https://wiki.blender.org/index.php/Dev:2.8/Source/Workflow/WorkspacesDiagram

I was under the impression I showed you this once you were at the studio last time, though.

It seems wrong to me that loading a file without UI could change the render engine. If we think of a render engine as just a draw mode that may be ok, but it's more than that and also defines which panels are visible, what happens when you press F12, preview renders, etc.

I agree with you. And that's why the idea is (see diagram above) to store: "render settings" + "active layer" + "engine" in the scene, as well as the workspaces. If you load a file without UI, you can still F12, same for batch rendering.

What is new in this patch, and part of a non-published design, is that Workspaces should also allow users to temporarily see the F12 settings (render settings + active layer + engine). This is the "use_scene_settings" option.

If this is meant to make render engines easier to use as a draw mode, then why not have this as a setting per 3D view? We want to support different draw modes in different 3D views in the same workspace, or not? As we discussed I think draw modes and render engines should be different concepts though.

Draw modes should be a different concept indeed. But it's not directly related to that.

I'd like to see if we could avoid passing workspaces around if its not needed since this is quite a high-level windowing struct.

I see your point, and agree with it. I was just preparing the terrain because we will also need the "workspace" to get RenderData from it.
But it can (should) also be passed as argument/stored in EvaluationContext, of course.

  • SETLOOPER - of ~19 uses the workspace argument is only used once by an RNA update callback.

Some of those may need workspace in the future. To be honest the way SETLOOPER is used now (sometimes to loop over all scene objects, other times to loop over all visible objects) makes it hard to refactor into a single new function.

Suggest leaving existing macro as-is and add a `SETLOOPER_SCENE_LAYER` macro which can take a SceneLayer instead of a workspace.

But that makes no sense, unless we change the SETLOOPER macro to loop over all the render layers of the main scene. And if we do it, it makes no sense to iterate over Bases in this case (since bases are part of render layers, not of scene).

  • ED_preview_icon_render - these should probably use the scene's engine? Assuming they wont be cleared for updating when the engine is changed. This is more library level than window/screen, so I don't think current workspace engine should influence previews.

Not really. If your scene (F12) engine is Cycles, but you are currently in a workspace with Eevee you want to see the Eevee material being previewed, as well as the icon. The ED_preview_icon_render is used for the material datablock if I'm not mistaken.

Thanks for the explanation.

I'm still confused by this though, because the tooltip of the workspace engine says "Engine to use for viewport drawing" and it works that way. If the intention is to eventually make the draw mode a setting per 3D view again, then what will this workspace setting do once that is working? Seeing the engine from the F12 settings should also be something you control per 3D view I think.

I'm also not sure about storing "render settings" in workspaces but that's another matter.

I'm still strictly against moving any render settings, including the engine, to workspaces.
IMHO this should be our last resort, definitely not our first choice.

The mentioned wikipage describes why this route was proposed:

This design allows a user to have her own “workspaces.blend” file, re-usable for different projects, with pre-defined render settings for specific workflows.

My suggestion has always been to use the render-layers for this instead. They would contain all the needed render-settings (i.e. render engine) also for viewport renders. And since the workspace already stores an active render-layer, it would also have its render-settings.
On top of that, we could still have display settings, like the "Draw Normals" option or matcap settings.


I've been working on a document that explains more precisely why I'm against having render-settings in workspaces. It also proposes a concrete alternative solution. I hope I can finally share it tomorrow.
We should probably open a design task to discuss things further.

@Campbell Barton (campbellbarton) :
I just re-read your point about the icons. I didn't realize the icon is stored in the image datablock itself. In this case I agree with you. The material preview should still use the workspace though.
Also, I think to split the SETLOOPER macro in two makes sense.

@Brecht Van Lommel (brecht):

I'm still confused by this though, because the tooltip of the workspace engine says "Engine to use for viewport drawing" and it works that way.

Correct.

Seeing the engine from the F12 settings should also be something you control per 3D view I think

The "use_scene_settings" is affecting the layer[*], engine and settings. It's something that affects the entire window, and all the editors. If you add a new node, the poll should take the engine into consideration. If you are looking at visible objects in the dopesheet, it should take the layer into consideration. If you change the material and want to see the material preview in the properties editor, you are using it render settings.

By staying away frow viewport specific settings we end up allowing the whole window to work in a unified way, following the data as determined by the workspace/topbar.

[*] Layers are not actually working because depsgraph still needs to move away from scene.

If the intention is to eventually make the draw mode a setting per 3D view again, then what will this workspace setting do once that is working?

If you are using "use_scene_settings" you are previewing final render (scene) render settings. The viewport "draw modes" (or rather engine presets) should probably be ignored in this case, since they are "drawing" settings, and not final render settings. I think it's more sane to keep this separation.

The mentioned wikipage describes why this route was proposed:

This design allows a user to have her own “workspaces.blend” file, re-usable for different projects, with pre-defined render settings for specific workflows.

My suggestion has always been to use the render-layers for this instead. They would contain all the needed render-settings (i.e. render engine) also for viewport renders. And since the workspace already stores an active render-layer, it would also have its render-settings.
On top of that, we could still have display settings, like the "Draw Normals" option or matcap settings.

I'm pretty sure we had this discussion before. You want to setup a workspace, and re-use it for different projects. If the render settings are exclusively part of a layer, you cannot open a new file, add one of your pre-defined workspaces, pick a different layer, and start working. This is the entire reason why those settings (and the overlay engine settings such as draw normals) fit well in the workspaces. I can elaborate on that in the above diagram, but that's the core of it.

I've been working on a document that explains more precisely why I'm against having render-settings in workspaces. It also proposes a concrete alternative solution. I hope I can finally share it tomorrow.

Looking forward to reading it today. That said, although I'm happy to see a new design, workspace was merged as it is, with very limited usefulness (i.e., screens on steroids) and yet considered "completed". It's been months and no one has picked up on that, or even voiced discontent with workspaces.

Workspaces are a key part of the workflow paradigm, and are yet to live up to that, in my humblest opinion. That's why the design presented in that diagram was thought of back in March. That's why I'm once again having to stop the work in the viewport to give another take at workspace design. Getting even to the point of having to implement things myself, which I was trying to avoid, to be honest.

I don't want to step on anyone's toes, but all those parts of 2.8 need to walk together. Pbr, layers, collections, workspaces, topbar, manipulators, ... they are all part of the same workflow design, and depend on each other.

The "use_scene_settings" is affecting the layer[*], engine and settings. It's something that affects the entire window, and all the editors. If you add a new node, the poll should take the engine into consideration. If you are looking at visible objects in the dopesheet, it should take the layer into consideration. If you change the material and want to see the material preview in the properties editor, you are using it render settings.
By staying away frow viewport specific settings we end up allowing the whole window to work in a unified way, following the data as determined by the workspace/topbar.

If you add a new node, it should always take final render engine into consideration, because adding nodes that do not work in the final render is not what you want, even if the viewport is currently drawn using a different engine. For the dopesheet what matters is the active layer, and I'm fine with having that at the workspace level. The material preview can always be rendered with the final render engine as well.

The final render engine is what unifies things, for Cycles an engine like Clay or Eevee are useful to draw the 3D viewport, but I don't see why they should affect other editors.

If you are using "use_scene_settings" you are previewing final render (scene) render settings. The viewport "draw modes" (or rather engine presets) should probably be ignored in this case, since they are "drawing" settings, and not final render settings. I think it's more sane to keep this separation.

I think it would be great to have a final render displayed in a viewport, I just don't see why it should be in all viewports. It requires you to do modelling, sculpting, painting in one "mode" where you can't see the final render, and tweak materials and lights in another "mode" where you can't have open a secondary viewport to easily navigate the scene to select objects or position lights. These are workflows that are used all the time in 2.7x.

@Brecht Van Lommel (brecht):

The final render engine is what unifies things, for Cycles an engine like Clay or Eevee are useful to draw the 3D viewport, but I don't see why they should affect other editors.

Actually, per the current design, the window is what unifies things. That's why we need a top bar. We want the entire Blender talking about the same data, on the same state.
The window then pick a workspace and a scene. The scene is used to expose layers for the workspace. The workspace is what defines the engine + layer + settings that will be "followed" by the rest of the UI.

As I told @Julian Eisel (Severin) we can change this, of course. But for now we really want to give it a go at this design. To ditch this before we even had it fully working is premature.

I for one don't think users can experience the new design before things like multi-window (T51736), depsgraph ownership (T52765) or topbar are not fully implemented. That's not even mentioning overrides.

Then we can think about tackling the problems based on blender 2.8, but based on previous 2.7 established workflows. Some of it may be solved with shortcuts (to quickly switch engines). Some of it may be solved by making Cycles fully usable even when render preview. Some of it may need per-viewport settings, overrides, ...

I'm pretty sure we had this discussion before. You want to setup a workspace, and re-use it for different projects. If the render settings are exclusively part of a layer, you cannot open a new file, add one of your pre-defined workspaces, pick a different layer, and start working.

That is totally valid and I never said this shouldn't be possible. The question is just how do we make this possible. And this is where we disagree. We agree on the requirements, we disagree on the specifications.
I don't think moving (mostly duplicating) a bunch of render-settings to the workspace is a good way to solve this.

Also, I did explain an alternative solution that would satisfy all requirements to you on IRC. My doc explains it in detail, I don't think this is the right place to propose it.

I've been working on a document that explains more precisely why I'm against having render-settings in workspaces. It also proposes a concrete alternative solution. I hope I can finally share it tomorrow.

Looking forward to reading it today. That said, although I'm happy to see a new design, workspace was merged as it is, with very limited usefulness (i.e., screens on steroids) and yet considered "completed". It's been months and no one has picked up on that, or even voiced discontent with workspaces.

I would never say that workspaces are completed, nor would I ever say this. The initial integration - coding the workspaces themselves and make the rest of Blender's UI code work fine with it - that is done. We can now add features for advanced UI setups (hiding panels/buttons, custom toolbars, per workspace add-on overrides, per workspace keymap overrides), make interaction modes (object mode, edit mode, etc) work flawlessly with workspaces and such.

Workspaces are a key part of the workflow paradigm, and are yet to live up to that, in my humblest opinion. That's why the design presented in that diagram was thought of back in March.

Right, workspaces aren't really useful yet, I definitely agree. TBH I feel like your too much focused on viewports however. Workspaces are a high level system, designed for much more than just viewports, but for entire Blender. Workspaces can certainly help improving viewport based workflows. But that doesn't mean we need to make the entire viewport design based on workspaces (we shouldn't IMHO).

Actually, per the current design, the window is what unifies things. That's why we need a top bar. We want the entire Blender talking about the same data, on the same state.
The window then pick a workspace and a scene. The scene is used to expose layers for the workspace. The workspace is what defines the engine + layer + settings that will be "followed" by the rest of the UI.

I understand that's where the new design differs from the one I implemented in 2.6x. But I don't see this as better than per viewport draw modes, or why perhaps doing it per viewport makes some other thing harder. We don't need the engine or render settings to be the same for the entire workspace to ensure we are working on the same data in all editors, only the active layer is needed for that.

Anyway, I've given my input on this topic on multiple occasions and can't seem to change your opinions, so not much point continuing this discussion. Better to wait until this is further along then.

  • Cleanup
  • From review: use eval_context to store engine instead of workspace
  • From review: use RenderEngineType instead of const char *engine_name

I started to address some of the review suggestions regarding passing
Workspace around and expose RenderEngineType instead of the engine name.

I still need to get the SET_LOOPER suggestions addressed.

  • From review: split SETLOOPER usecases
  • Fix doversion for char engine[]
  • From review: Use workspace engine only for buttons window preview

Generally LGTM, only minor issues noted inline.

source/blender/blenkernel/BKE_layer.h
264

Minor concern:

Was wondering why Main is needed to loop over renderable objects (since scenes have access to all the objects).

Since this is only used for clearing tags, we could clear the tags from the scene - also avoids the chance of unnecessary tagging if the blend file happens to have many objects in other scenes which aren't active.

source/blender/blenkernel/BKE_scene.h
184–191

Not sure how picky to be, but these should probably be BKE_scene_ prefixed, since they don't take RenderData.

source/blender/makesrna/intern/rna_workspace.c
127

Could BLI_assert if this isn't found.

source/blender/render/intern/include/render_types.h
204

*picky*, would prefer engine_id, since name is typically for user readable names, id for unique identifiers.

This revision is now accepted and ready to land.Oct 12 2017, 2:36 AM
  • From review: handle rna_workspace_engine_set differently
  • Frow review: Rename engine_name > engine_id
  • From review: Rename BKE_render_* to BKE_viewrender_*
  • From review: Massive changes: own struct for ViewRender data (engine, ...)

@Campbell Barton (campbellbarton) There is one thing I need to test further, but I updated the patch with yours and @Julian Eisel (Severin)'s suggestion. It became a much bigger patch, but we now have a "ViewRender" struct. Feel free to take another look at it.

Thanks for the update, again, generally fine - though pointed out some minor things.

Main question is why allocate ViewRender separately, these small structs are typically better to include unless they are shared or optional and very large.

source/blender/makesdna/DNA_scene_types.h
1708

Any reason to make this a pointer instead of in-lining (ViewRender view_render;) in the struct?

Is there any time we would want the scene to share this with something else? or a reason we would want this to be NULL.

source/blender/makesdna/DNA_space_types.h
502–511

Why is this needed? (from quick look seems its not)

source/blender/makesdna/DNA_workspace_types.h
86

Again, why not include this directly in the struct? (see same comment for Scene)

139

*picky*, there is loose convention to use lowercase e for enum types.

  • From review: No longer need to change mainb type
  • From review: dont use pointer to ViewRender in DNA_*
  • From review: Use "e" prefix for enums

Final touchups - so I can review it online

This revision was automatically updated to reflect the committed changes.