Node previews in the shader editor #110065

Merged
Brecht Van Lommel merged 55 commits from Kdaf/blender:shader-node-previews-GSOC into main 2023-08-08 17:36:14 +02:00
Member

First implementation of node previews in the shader editor.
Using the already present area of previews in the node overlays, most shader nodes (excepted group in/output and material output) can now be previewed.

We take advantage of the RenderResult available as ImBuf images to store a Render for every viewed nested node tree present in a SpaceNode. The computation is initiated at the moment of drawing nodes overlays.

One render is started for the current nodetree, having a ViewLayer associated with each previewed node.
We separate the previewed nodes in two categories: the shader ones and the non-shader ones.

  • for non-shader nodes, we use AOVs(Arbitrary Output Variable) which highly speed up the rendering process by rendering every non-shader nodes at the same time. They are rendered in the first ViewLayer.
  • for shader nodes, we render them each in a different ViewLayer, by rerouting the node to the output of the material in the preview scene.

The preview scene takes the same aspect as the Material preview scene, and the same preview object is used.

At the moment of drawing the node overlay, we take the Render of the viewed node tree and extract the ImBuf of the wanted viewlayer/pass for each previewed node.

What's not supported with this patch:

  • Volume nodes are not supported yet, and will probably not be in the future
  • non-shader node previews dont show up with a background, it will be the case in a future patch
  • there are still some confusing behaviours when toggling node previews (will be fixed in a future patch changing a bit the event structure)
    • when enabling the preview, it doesn't show immediately due to the need of a redraw
    • when disabling all nodes, preview rendering is still running in the background

For future users seeing this commit, there has been an older version of shader node previews back in Blender 2.79 with the internal render engine. There are still some rests in the Blender code used or unused.

First implementation of node previews in the shader editor. Using the already present area of previews in the node overlays, most shader nodes (excepted group in/output and material output) can now be previewed. We take advantage of the `RenderResult` available as `ImBuf` images to store a `Render` for every viewed nested node tree present in a SpaceNode. The computation is initiated at the moment of drawing nodes overlays. One render is started for the current nodetree, having a `ViewLayer` associated with each previewed node. We separate the previewed nodes in two categories: the shader ones and the non-shader ones. - for non-shader nodes, we use AOVs(Arbitrary Output Variable) which highly speed up the rendering process by rendering every non-shader nodes at the same time. They are rendered in the first `ViewLayer`. - for shader nodes, we render them each in a different `ViewLayer`, by rerouting the node to the output of the material in the preview scene. The preview scene takes the same aspect as the Material preview scene, and the same preview object is used. At the moment of drawing the node overlay, we take the `Render` of the viewed node tree and extract the ImBuf of the wanted viewlayer/pass for each previewed node. What's not supported with this patch: - Volume nodes are not supported yet, and will probably not be in the future - non-shader node previews dont show up with a background, it will be the case in a future patch - there are still some confusing behaviours when toggling node previews (will be fixed in a future patch changing a bit the event structure) - - when enabling the preview, it doesn't show immediately due to the need of a redraw - - when disabling all nodes, preview rendering is still running in the background For future users seeing this commit, there has been an older version of shader node previews back in Blender 2.79 with the internal render engine. There are still some rests in the Blender code used or unused.
Colin Marmond added the
Interest
Render & Cycles
label 2023-07-13 16:28:09 +02:00
Colin Marmond added 1 commit 2023-07-13 16:28:16 +02:00
Author
Member

I had an issue with git, so the previous pull request #109120 is closed and replaced by this one.

I had an issue with git, so the previous pull request #109120 is closed and replaced by this one.
Colin Marmond added 2 commits 2023-07-14 11:29:22 +02:00
Colin Marmond added 1 commit 2023-07-14 12:09:56 +02:00
Colin Marmond added 2 commits 2023-07-18 15:06:39 +02:00
Not working, have to adapt for ImBuf
There is still a thing to do about locking the result.
Colin Marmond added 2 commits 2023-07-19 09:52:55 +02:00
Colin Marmond added 4 commits 2023-07-19 12:39:02 +02:00
Colin Marmond added 2 commits 2023-07-19 12:52:21 +02:00
Colin Marmond added 1 commit 2023-07-20 21:41:51 +02:00
Remove the ability to preview group inputs/outputs and material outputs.

Also clean the drawing spaces made for the old design.
Colin Marmond added 2 commits 2023-07-21 15:53:58 +02:00
Colin Marmond added 1 commit 2023-07-21 16:07:38 +02:00
The structure owning each nodetree previews is now called `NestedNodePreviews` and the map of them stored in the spacenode is called `tree_previews_per_context`.
Colin Marmond added 1 commit 2023-07-21 21:05:49 +02:00
Also remove Volume support
Colin Marmond added 1 commit 2023-07-21 21:12:54 +02:00
Colin Marmond changed title from WIP: Node Preview in the shader editor GSOC Project to Node Preview in the shader editor GSOC Project 2023-07-21 21:14:44 +02:00
Colin Marmond requested review from Brecht Van Lommel 2023-07-24 12:06:26 +02:00
Colin Marmond requested review from Hans Goudey 2023-07-24 12:06:27 +02:00
Colin Marmond changed title from Node Preview in the shader editor GSOC Project to Node previews in the shader editor 2023-07-24 12:10:35 +02:00
Hans Goudey requested changes 2023-07-24 21:20:33 +02:00
Hans Goudey left a comment
Member

Maybe it's hard to avoid, but I think the preview toggle in the header of every shade node is unfortunate, adding noise and overlapping text. Putting buttons and toggles and warnings in the node headers just isn't a scalable solution in my opinion. I don't have a better idea myself right now, and this is also just the existing compositor design. But I wonder if it's worth looking into some other options.

I guess the only other thing it would be nice to avoid in theory is the amount of copying and edits in the bNodeTree data structure. For large materials that will probably have a performance impact. I don't have anything constructive to say about that really, just wanted to put it out there. I'm sure Brecht has more useful comments about the overall architecture.

Maybe it's hard to avoid, but I think the preview toggle in the header of every shade node is unfortunate, adding noise and overlapping text. Putting buttons and toggles and warnings in the node headers just isn't a scalable solution in my opinion. I don't have a better idea myself right now, and this is also just the existing compositor design. But I wonder if it's worth looking into some other options. I guess the only other thing it would be nice to avoid in theory is the amount of copying and edits in the `bNodeTree` data structure. For large materials that will probably have a performance impact. I don't have anything constructive to say about that really, just wanted to put it out there. I'm sure Brecht has more useful comments about the overall architecture.
@ -718,6 +719,18 @@ class NodeTreeMainUpdater {
blender::bke::node_preview_remove_unused(&ntree);
}
void needs_preview_redraw(bNodeTree &ntree)
Member

"redraw" is a bit vague, since it usually refers to just the UI code. I have a feeling this is checking whether the previews are outdated, not whether they should actually be redrawn in the UI.

"redraw" is a bit vague, since it usually refers to just the UI code. I have a feeling this is checking whether the previews are outdated, not whether they should actually be redrawn in the UI.
Kdaf marked this conversation as resolved
@ -721,0 +724,4 @@
ntree.preview_refresh_state++;
ntree.ensure_topology_cache();
for (bNode *node : ntree.group_nodes()) {
bNodeTree *nested_tree = (bNodeTree *)node->id;
Member
https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast
Kdaf marked this conversation as resolved
@ -0,0 +1,38 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2005 Blender Foundation */
Member

Outdated/incorrect copyright

Outdated/incorrect copyright
Kdaf marked this conversation as resolved
@ -0,0 +10,4 @@
#include "RE_pipeline.h"
#ifdef __cplusplus
extern "C" {
Member

If this isn't included in a C file anymore, no need for extern "C"

If this isn't included in a C file anymore, no need for `extern "C"`
Kdaf marked this conversation as resolved
@ -0,0 +20,4 @@
struct NestedTreePreviews {
Render *previews_render;
int pr_size;
Member

No need to abbreviate "preview" as "pr" here IMO

No need to abbreviate "preview" as "pr" here IMO
Kdaf marked this conversation as resolved
@ -2233,3 +2230,1 @@
BKE_node_instance_hash_lookup(previews, key));
if (preview_compositor) {
preview = preview_compositor->ibuf;
/* Overlay atop the node. */
Member

This indentation is making the diff look more complex than it actually is. Is it possible to revert that change and the moved initialization of bNodeInstanceHash *previews (maybe committing them separately?)

This indentation is making the diff look more complex than it actually is. Is it possible to revert that change and the moved initialization of `bNodeInstanceHash *previews` (maybe committing them separately?)
Author
Member

You mean I should do another patch just for the moving of the initialization of bNodeInstanceHash *previews ?

You mean I should do another patch just for the moving of the initialization of `bNodeInstanceHash *previews` ?
Kdaf marked this conversation as resolved
@ -107,6 +109,10 @@ struct SpaceNode_Runtime {
/** Temporary data for node insert offset (in UI called Auto-offset). */
struct NodeInsertOfsData *iofsd;
/** Use this to store data for the displayed node tree. It has an entry for every distinct
Member
/**
 * Use this to store data for the displayed node tree. It has an entry for every distinct
 * nested nodegroup.
 */ 
``` /** * Use this to store data for the displayed node tree. It has an entry for every distinct * nested nodegroup. */ ```
Kdaf marked this conversation as resolved
@ -0,0 +12,4 @@
*
* We separate the previewed nodes in two categories: the shader ones and the non-shader ones.
* - for non-shader nodes, we use AOVs(Arbitrary Output Variable) which highly speed up the
* rendering process by rendering every non-shader nodes at the same time. They are rendered in the
Member

every non-shader nodes -> every non-shader node

`every non-shader nodes` -> `every non-shader node`
Kdaf marked this conversation as resolved
@ -0,0 +53,4 @@
struct ShaderNodesPreviewJob {
NestedTreePreviews *tree_previews;
Material *mat_orig;
Member

Do you have to store mat_orig, or could you just store mat_copy here?

Do you have to store `mat_orig`, or could you just store `mat_copy` here?
Kdaf marked this conversation as resolved
@ -0,0 +57,4 @@
/* Listbase containing bNodeTreePath * guiding to the viewed nodetree of mat_orig. */
ListBase *treepath;
Scene *scene;
bool *stop;
Member

A comment about what owns these boolean pointers would be helpful. Or maybe it isn't necessary to store them?

A comment about what owns these boolean pointers would be helpful. Or maybe it isn't necessary to store them?
Kdaf marked this conversation as resolved
@ -0,0 +63,4 @@
Material *mat_copy;
bNode *mat_output_copy;
NodeSocketPair mat_displacement_copy;
ListBase treepath_copy;
Member

This treepath_copy could be stored as a Vector to simplify ownership and improve type safety

This `treepath_copy` could be stored as a `Vector` to simplify ownership and improve type safety
Kdaf marked this conversation as resolved
@ -0,0 +122,4 @@
if (auto hash = get_compute_context_hash_for_node_editor(*sn)) {
tree_previews = sn->runtime->tree_previews_per_context.lookup_or_add_cb(*hash, [&]() {
tree_previews = static_cast<NestedTreePreviews *>(
MEM_callocN(sizeof(NestedTreePreviews), __func__));
Member

MEM_new should work here

`MEM_new` should work here
Kdaf marked this conversation as resolved
@ -0,0 +158,4 @@
Main *pr_main,
Material *matcopy)
{
Scene *sce;
Member

sce -> scene, and give the existing scene variable a name that differentiates it

`sce` -> `scene`, and give the existing `scene` variable a name that differentiates it
Kdaf marked this conversation as resolved
@ -0,0 +315,4 @@
bNode *final_node,
bNodeSocket *final_socket)
{
for (bNodeTreePath *path = (bNodeTreePath *)treepath.last(); path->prev != nullptr;
Member

https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast

I'll stop commenting this now, please just check the rest of the patch. Thanks :)

https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast I'll stop commenting this now, please just check the rest of the patch. Thanks :)
Kdaf marked this conversation as resolved
@ -0,0 +356,4 @@
/* Connect the node to the output of the first nodetree from `treepath`. Last element of `treepath`
* should be the path to the node's nodetree */
static void connect_node_to_surface_output(Vector<const bNodeTreePath *> treepath,
Member

Passing Vector by value copies its contents. I'm guessing this should be Span instead.

Passing `Vector` by value copies its contents. I'm guessing this should be `Span` instead.
Kdaf marked this conversation as resolved
@ -0,0 +380,4 @@
/* Connect the nodes to some aov nodes located in the first nodetree from `treepath`. Last element
* of `treepath` should be the path to the nodes nodetree. */
static void connect_nodes_to_aovs(const Vector<const bNodeTreePath *> &treepath,
Member

const Vector & -> const Span

`const Vector &` -> `const Span`
Kdaf marked this conversation as resolved
@ -0,0 +651,4 @@
"Shader Previews",
WM_JOB_EXCL_RENDER,
WM_JOB_TYPE_RENDER_PREVIEW);
ShaderNodesPreviewJob *nt_previews = MEM_new<ShaderNodesPreviewJob>("shader previews");
Member

MEM_new<ShaderNodesPreviewJob>("shader previews"); -> MEM_new<ShaderNodesPreviewJob>(__func__);

Still gives usable text for memory leak warnings but without specifying a new string

`MEM_new<ShaderNodesPreviewJob>("shader previews");` -> `MEM_new<ShaderNodesPreviewJob>(__func__);` Still gives usable text for memory leak warnings but without specifying a new string
Kdaf marked this conversation as resolved
@ -332,1 +333,4 @@
static void node_exit(wmWindowManager *wm, ScrArea *area)
{
SpaceNode *snode = (SpaceNode *)area->spacedata.first;
Member
https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast
Kdaf marked this conversation as resolved
@ -664,1 +664,4 @@
bNestedNodeRef *nested_node_refs;
/* Contains a number increased for each nodetree update.
* The idea here to to store a state var in the preview structures to compare if it differs. */
uint16_t preview_refresh_state;
Member

This counter isn't really a standard way of doing cache invalidation. It looks error prone to me, and not very descriptive. Don't have much context here, but I'd suggest removing the images if they're invalid, or if that isn't possible store a dirty state together with the previews.

This counter isn't really a standard way of doing cache invalidation. It looks error prone to me, and not very descriptive. Don't have much context here, but I'd suggest removing the images if they're invalid, or if that isn't possible store a dirty state together with the previews.
Author
Member

It was hard to find a way to figure out what to use for the dirty state of the previews.
I thought about removing the images, but since the system rely on a RenderResult, we would have to kill the job, make sure we can remove the RenderResult. And the problem is that in many refresh areas, we only have access to the nodetree itself.
The major constraint I had was about how to manage the refresh event.
I'm planning to redo it afterward to do a per node refresh (kind of), and thus do a propagation of the refresh event in the nodes right-connected to the updated nodes.

I know this is far from optimal now, but I dont really know how to do otherwise, since this system will be changed as soon as this patch gets landed.

It was hard to find a way to figure out what to use for the dirty state of the previews. I thought about removing the images, but since the system rely on a RenderResult, we would have to kill the job, make sure we can remove the RenderResult. And the problem is that in many refresh areas, we only have access to the nodetree itself. The major constraint I had was about how to manage the refresh event. I'm planning to redo it afterward to do a per node refresh (kind of), and thus do a propagation of the refresh event in the nodes right-connected to the updated nodes. I know this is far from optimal now, but I dont really know how to do otherwise, since this system will be changed as soon as this patch gets landed.

It seems that once images are implicitly shared, deletion of images no longer needed will become automatic?

It seems that once images are implicitly shared, deletion of images no longer needed will become automatic?
Author
Member

@mod_moder Yes of course, but the problem is not there I guess. It is more about accessing this image from the other functions.
I do not think that is a good idea to modify much functions because this system will be removed asap.
I know it is weird to do a pull request aiming to be modified, but it would never be merged if I waited for another two weeks for completing everything of this project.

@mod_moder Yes of course, but the problem is not there I guess. It is more about accessing this image from the other functions. I do not think that is a good idea to modify much functions because this system will be removed asap. I know it is weird to do a pull request aiming to be modified, but it would never be merged if I waited for another two weeks for completing everything of this project.

I don't have a strong opinion on how best to do the updates. A counter like this seems reasonable absent something more complicated, and to me it seems reasonable as a first step to keep the size of this PR under control.

Can this be moved into runtime (bNodeTreeRuntime)? Also make it a 32 bit integer I think, no real reason to use only 16 and it's conceivable to exceed that.

I don't have a strong opinion on how best to do the updates. A counter like this seems reasonable absent something more complicated, and to me it seems reasonable as a first step to keep the size of this PR under control. Can this be moved into `runtime` (`bNodeTreeRuntime`)? Also make it a 32 bit integer I think, no real reason to use only 16 and it's conceivable to exceed that.
Author
Member

I thought when the counter will reach the highest value, it would loop to 0 again, so 16 bit was enough I thought, but I'll put 32, it's not too much important.

I thought when the counter will reach the highest value, it would loop to 0 again, so 16 bit was enough I thought, but I'll put 32, it's not too much important.

Still a question for me, can't you update this counter in the trees update post callback?

Still a question for me, can't you update this counter in the trees update post callback?
Kdaf marked this conversation as resolved
@ -7640,1 +7640,4 @@
RNA_def_property_clear_flag(prop, PROP_EDITABLE);
RNA_def_property_ui_text(prop,
"Supports Previews",
"Specific to a SpaceNode type to know whether nodes can have previews");
Member

Tooltips generally don't contain have Python/C++ type names, so SpaceNode -> node editor. The text can be simplified a bit though, like "Whether the node editor's type supports displaying node previews`

https://wiki.blender.org/wiki/Human_Interface_Guidelines/Writing_Style

Tooltips generally don't contain have Python/C++ type names, so `SpaceNode` -> `node editor`. The text can be simplified a bit though, like "Whether the node editor's type supports displaying node previews` https://wiki.blender.org/wiki/Human_Interface_Guidelines/Writing_Style
Author
Member

I'm sorry, I dont understand what you mean.
Ok I see

<s>I'm sorry, I dont understand what you mean.</s> Ok I see
Kdaf marked this conversation as resolved
@ -5422,0 +5424,4 @@
RNA_def_property_range(prop, 50, 250);
RNA_def_property_ui_text(prop,
"Node Preview Resolution",
"Resolution used for Shader node previews (should be changed for "
Member

DPI issues

Better not suggest working around a bug in the tooltip ;)

>DPI issues Better not suggest working around a bug in the tooltip ;)
Kdaf marked this conversation as resolved
@ -5422,0 +5425,4 @@
RNA_def_property_ui_text(prop,
"Node Preview Resolution",
"Resolution used for Shader node previews (should be changed for "
"performances and/or DPI issues)");
Member

"performances" shouldn't be plural, it's like "liquid" in that way

"performances" shouldn't be plural, it's like "liquid" in that way
Author
Member

Could I say this ? Resolution used for Shader node previews (should be changed for performance convenience)

Could I say this ? `Resolution used for Shader node previews (should be changed for performance convenience)`
Kdaf marked this conversation as resolved
Colin Marmond added 5 commits 2023-07-25 11:57:42 +02:00
Colin Marmond added 1 commit 2023-07-25 12:11:05 +02:00
Colin Marmond added 1 commit 2023-07-25 15:33:44 +02:00
I thought I already fixed that, strange that I did not committed or merged.
Brecht Van Lommel requested changes 2023-07-26 19:37:06 +02:00
Brecht Van Lommel left a comment
Owner

Overall the implementation is looking great to me now, the naming is clear.

I couldn't get the previews working with factory settings, going to the shading workspace and enabling the preview on the Principled BSDF. And then on exit it hangs on a mutex, which I think is related to the acquire/release code where I left an inline comment.

Opening an existing .blend file with previews enabled did render them though.

Overall the implementation is looking great to me now, the naming is clear. I couldn't get the previews working with factory settings, going to the shading workspace and enabling the preview on the Principled BSDF. And then on exit it hangs on a mutex, which I think is related to the acquire/release code where I left an inline comment. Opening an existing .blend file with previews enabled did render them though.
@ -2236,1 +2250,4 @@
if (preview && !(preview->x && preview->y)) {
preview = nullptr;
}
}

A call to RE_AcquireResultRead must always be paired with a call to RE_ReleaseResult, but right now it's hard to see in the code that this is the case. Can you organize it to make that obvious? Perhaps something like:

if (previews_shader) {
  /* Shader node preview. */
  ImBuf *preview = ED_node_preview_acquire_ibuf(&ntree, previews_shader, &node);
  if (preview) {
    ..
  }
  ED_node_release_preview_ibuf(previews_shader);
}
else {
  /* Other previews. */
  ...
}
A call to `RE_AcquireResultRead` must always be paired with a call to `RE_ReleaseResult`, but right now it's hard to see in the code that this is the case. Can you organize it to make that obvious? Perhaps something like: ``` if (previews_shader) { /* Shader node preview. */ ImBuf *preview = ED_node_preview_acquire_ibuf(&ntree, previews_shader, &node); if (preview) { .. } ED_node_release_preview_ibuf(previews_shader); } else { /* Other previews. */ ... }
Kdaf marked this conversation as resolved
@ -110,0 +113,4 @@
* Use this to store data for the displayed node tree. It has an entry for every distinct
* nested nodegroup.
*/
Map<ComputeContextHash, NestedTreePreviews *> tree_previews_per_context;

Is it possible to use std::unique_ptr<NestedTreePreviews> to automate freeing this?

Is it possible to use `std::unique_ptr<NestedTreePreviews>` to automate freeing this?
Author
Member

I never used that concept, I searched online and I tried to do the best I can. Could you check in the commit if I did it correctly ?

I never used that concept, I searched online and I tried to do the best I can. Could you check in the [commit](https://projects.blender.org/blender/blender/commit/4ccf9af4123118d02b7c3d7218daa5d6e6d718bc) if I did it correctly ?

Looks correct, thanks.

Looks correct, thanks.
Kdaf marked this conversation as resolved
@ -0,0 +464,4 @@
}
Span<bNodeTreePath *> treepath = job_data.treepath_copy;
/* Disconnect shader, volume and displacement from the material output. */

Eevee has a Thickness input that was forgotten here. For future proofing, I'd suggest to loop over all inputs of this node and disconnect them all.

Eevee has a Thickness input that was forgotten here. For future proofing, I'd suggest to loop over all inputs of this node and disconnect them all.
Kdaf marked this conversation as resolved
Colin Marmond added 2 commits 2023-07-27 10:13:48 +02:00
And also dont render previews if the size is 0 (should not be the case, but in a wrong situation it could)
Author
Member

Overall the implementation is looking great to me now, the naming is clear.

I couldn't get the previews working with factory settings, going to the shading workspace and enabling the preview on the Principled BSDF. And then on exit it hangs on a mutex, which I think is related to the acquire/release code where I left an inline comment.

Opening an existing .blend file with previews enabled did render them though.

It was not working at first launch because I did not update the versioning to the latest main (I guess you applied the patch on the latest main). That's now updated.

The hangs on the mutex was due to the preview size being 0, which is now corrected.

> Overall the implementation is looking great to me now, the naming is clear. > > I couldn't get the previews working with factory settings, going to the shading workspace and enabling the preview on the Principled BSDF. And then on exit it hangs on a mutex, which I think is related to the acquire/release code where I left an inline comment. > > Opening an existing .blend file with previews enabled did render them though. It was not working at first launch because I did not update the versioning to the latest main (I guess you applied the patch on the latest main). That's now updated. The hangs on the mutex was due to the preview size being 0, which is now corrected.
Colin Marmond added 2 commits 2023-07-27 11:07:18 +02:00
Colin Marmond added 1 commit 2023-07-27 11:24:17 +02:00
Colin Marmond added 1 commit 2023-07-27 11:36:27 +02:00
Colin Marmond requested review from Brecht Van Lommel 2023-07-27 11:55:56 +02:00
Colin Marmond requested review from Hans Goudey 2023-07-27 11:56:02 +02:00
Brecht Van Lommel requested changes 2023-07-27 14:12:32 +02:00
Brecht Van Lommel left a comment
Owner

I tested the pull request as a branch so that should not have been the problem, but regardless it seems to be fixed now.

The main thing I noticed testing this is that the previews flicker a lot when dragging sliders. For the properties editor preview we ensure the render result remains in place on refreshes. The R_BUTS_PREVIEW does that but it is also used here. I guess there is some other place where the preview image gets freed, can you find a way to keep it until it is replaced by a new one?

I tested the pull request as a branch so that should not have been the problem, but regardless it seems to be fixed now. The main thing I noticed testing this is that the previews flicker a lot when dragging sliders. For the properties editor preview we ensure the render result remains in place on refreshes. The `R_BUTS_PREVIEW` does that but it is also used here. I guess there is some other place where the preview image gets freed, can you find a way to keep it until it is replaced by a new one?
@ -97,0 +99,4 @@
* The idea here to to store a state var in the `NestedTreePreviews` structure to compare if they
* differ.
*/
uint32_t previews_refresh_state;

Add = 0.

Add `= 0`.
Kdaf marked this conversation as resolved
Author
Member

I tested the pull request as a branch so that should not have been the problem, but regardless it seems to be fixed now.

The main thing I noticed testing this is that the previews flicker a lot when dragging sliders. For the properties editor preview we ensure the render result remains in place on refreshes. The R_BUTS_PREVIEW does that but it is also used here. I guess there is some other place where the preview image gets freed, can you find a way to keep it until it is replaced by a new one?

I dont really know what to do, because the RenderResult must currently be freed in order to create a new one with fresh RenderLayers and RenderPasses.
As far as I know there is no function to update a renderresult to the updated scene. Is it something I could add to fit this problem ?

> I tested the pull request as a branch so that should not have been the problem, but regardless it seems to be fixed now. > > The main thing I noticed testing this is that the previews flicker a lot when dragging sliders. For the properties editor preview we ensure the render result remains in place on refreshes. The `R_BUTS_PREVIEW` does that but it is also used here. I guess there is some other place where the preview image gets freed, can you find a way to keep it until it is replaced by a new one? > I dont really know what to do, because the RenderResult must currently be freed in order to create a new one with fresh RenderLayers and RenderPasses. As far as I know there is no function to update a renderresult to the updated scene. Is it something I could add to fit this problem ?
Colin Marmond added 1 commit 2023-07-27 15:55:34 +02:00

I dont really know what to do, because the RenderResult must currently be freed in order to create a new one with fresh RenderLayers and RenderPasses.
As far as I know there is no function to update a renderresult to the updated scene. Is it something I could add to fit this problem ?

Maybe the preview can have its own reference to the image buffer. By calling IMB_refImBuf and then keeping an ImBuf* around, that is freed with IMB_freeImBuf. And then only ever replace that if there is a new image buffer with some contents?

> I dont really know what to do, because the RenderResult must currently be freed in order to create a new one with fresh RenderLayers and RenderPasses. > As far as I know there is no function to update a renderresult to the updated scene. Is it something I could add to fit this problem ? Maybe the preview can have its own reference to the image buffer. By calling `IMB_refImBuf` and then keeping an `ImBuf*` around, that is freed with `IMB_freeImBuf`. And then only ever replace that if there is a new image buffer with some contents?
Colin Marmond added 2 commits 2023-07-27 17:51:03 +02:00
Colin Marmond requested review from Brecht Van Lommel 2023-07-27 17:52:35 +02:00

More issues I found in testing:

  • Changing the preview type (shape) in the material properties does not update the previews.
  • Changing the render engine does not update the previews.
  • Eevee is rendering Principled BSDF black here, while Cycles and other nodes with Eevee work.
  • Workbench render engine does not support previews, so will just leave them unupdated. Can we hide the previews for render engines that do not support them? You can add a BKE_scene_use_previews similar to BKE_scene_use_shading_nodes_custom.
More issues I found in testing: * Changing the preview type (shape) in the material properties does not update the previews. * Changing the render engine does not update the previews. * Eevee is rendering Principled BSDF black here, while Cycles and other nodes with Eevee work. * Workbench render engine does not support previews, so will just leave them unupdated. Can we hide the previews for render engines that do not support them? You can add a `BKE_scene_use_previews` similar to `BKE_scene_use_shading_nodes_custom`.
Brecht Van Lommel requested changes 2023-07-27 18:11:46 +02:00
Brecht Van Lommel left a comment
Owner

Requesting changes.

Requesting changes.
Colin Marmond added 2 commits 2023-07-28 11:45:19 +02:00
Colin Marmond added 1 commit 2023-07-28 11:54:45 +02:00
just to see if git can recover the diff.
Colin Marmond force-pushed shader-node-previews-GSOC from b709502297 to b709502297 2023-07-28 11:57:29 +02:00 Compare

Here's an example .blend that has the problem.

Here's an example .blend that has the problem.
Colin Marmond added 2 commits 2023-07-28 14:11:31 +02:00
Colin Marmond added 1 commit 2023-07-28 16:17:12 +02:00
It fixes the black preview issue with eevee and cycles (persistent mode enabled).

I then added the persistent mode for cycles to allow some optimization of the engine.
Colin Marmond added 3 commits 2023-07-28 17:14:22 +02:00
Colin Marmond requested review from Brecht Van Lommel 2023-07-28 17:15:42 +02:00
Brecht Van Lommel approved these changes 2023-07-28 17:16:23 +02:00
Brecht Van Lommel left a comment
Owner

Looks good from my side now, but maybe @HooglyBoogly still has comments.

Once this is committed, there will need to an issue created with remaining to do items so we can keep track of them.

The release notes should also be updated, with a note about any known limitations that still exist.

Looks good from my side now, but maybe @HooglyBoogly still has comments. Once this is committed, there will need to an issue created with remaining to do items so we can keep track of them. The release notes should also be updated, with a note about any known limitations that still exist.
Author
Member

@brecht , I already did that #110353

I'll do that for the release note.

@brecht , I already did that #110353 I'll do that for the release note.
Member

I think disabling the previews overlay should remove the icon from the header of every node. That at least removes the added noise for cases where people don't plan on using previews.

I found the previews to be quite slow in my basic test. Most of the time they don't seem to show up at all. I attached a video below.

I think disabling the previews overlay should remove the icon from the header of every node. That at least removes the added noise for cases where people don't plan on using previews. I found the previews to be quite slow in my basic test. Most of the time they don't seem to show up at all. I attached a video below.
Hans Goudey requested changes 2023-08-04 15:52:48 +02:00
@ -31,2 +31,4 @@
struct Main *main;
/**
* Data for the preview scene to avoid loading in multiple scenarios.
Member

In such a global place, the preview scene doesn't really have meaning, maybe the comment can be a bit more specific.

In such a global place, `the preview scene` doesn't really have meaning, maybe the comment can be a bit more specific.
Kdaf marked this conversation as resolved
@ -96,1 +96,4 @@
/**
* Contains a number increased for each nodetree update.
* The idea here to to store a state var in the `NestedTreePreviews` structure to compare if they
Member

No need for abbreviations like var in comments. I'd also remove "The idea here to to", that's mainly just extra words that don't really help the explanation.

No need for abbreviations like `var` in comments. I'd also remove "The idea here to to", that's mainly just extra words that don't really help the explanation.
Kdaf marked this conversation as resolved
@ -721,0 +727,4 @@
continue;
}
bNodeTree *nested_tree = reinterpret_cast<bNodeTree *>(node->id);
if (nested_tree) {
Member

can look like this:
if (bNodeTree *nested_tree = reinterpret_cast<bNodeTree *>(node->id)) {

can look like this: `if (bNodeTree *nested_tree = reinterpret_cast<bNodeTree *>(node->id)) {`
Kdaf marked this conversation as resolved
@ -0,0 +12,4 @@
struct bNodeTree;
struct ImBuf;
struct Render;
Member

Since this is new code, we might as well use C++ namespaces properly here and clean up the relevant naming. This whole file should be in the blender::ed::space_node namespace. The ED_ prefix for functions can be removed as well.

Since this is new code, we might as well use C++ namespaces properly here and clean up the relevant naming. This whole file should be in the `blender::ed::space_node` namespace. The `ED_` prefix for functions can be removed as well.
Kdaf marked this conversation as resolved
@ -0,0 +22,4 @@
bool restart_needed;
uint32_t previews_refresh_state;
NestedTreePreviews(const int size)
: previews_render(nullptr),
Member

These defaults can be written like

int preview_size = 0;
These defaults can be written like ``` int preview_size = 0; ```
Kdaf marked this conversation as resolved
@ -0,0 +34,4 @@
if (this->previews_render) {
RE_FreeRender(this->previews_render);
}
this->previews_map.foreach_item([&](const bNode *, ImBuf *ibuf) { IMB_freeImBuf(ibuf); });
Member
    for (ImBuf *ibuf : this->previews_map.values()) {
      IMB_freeImBuf(ibuf);
    }

Rather than iterating over items and having the unused node variable

``` for (ImBuf *ibuf : this->previews_map.values()) { IMB_freeImBuf(ibuf); } ``` Rather than iterating over items and having the unused node variable
Kdaf marked this conversation as resolved
@ -0,0 +38,4 @@
}
};
void ED_spacenode_free_previews(wmWindowManager *wm, SpaceNode *snode);
Member

Use references for pointers that:

  • don't have ownership
  • are expected to be non-null
Use references for pointers that: - don't have ownership - are expected to be non-null
Kdaf marked this conversation as resolved
@ -0,0 +43,4 @@
NestedTreePreviews *tree_previews,
const bNode *node);
void ED_node_release_preview_ibuf(NestedTreePreviews *tree_previews);
NestedTreePreviews *ED_spacenode_get_nested_previews(const bContext *ctx, SpaceNode *sn);
Member

bContext variables are almost always named just C

`bContext` variables are almost always named just `C`
Kdaf marked this conversation as resolved
@ -82,0 +91,4 @@
ePreviewType pr_type,
ePreviewRenderMethod pr_method);
struct World *ED_preview_prepare_world(struct Main *pr_main,
const struct Scene *sce,
Member

sce -> scene

`sce` -> `scene`
Kdaf marked this conversation as resolved
@ -0,0 +129,4 @@
}
NestedTreePreviews *tree_previews = nullptr;
if (auto hash = get_compute_context_hash_for_node_editor(*sn)) {
tree_previews = &*sn->runtime->tree_previews_per_context.lookup_or_add_cb(
Member

.get() is usually clearer than &*

`.get()` is usually clearer than `&*`
Kdaf marked this conversation as resolved
@ -0,0 +146,4 @@
static Material *duplicate_material(const Material *mat)
{
if (mat == nullptr) {
Member

A null check should probably be at a higher level than this function.

A null check should probably be at a higher level than this function.
Kdaf marked this conversation as resolved
@ -0,0 +210,4 @@
ED_preview_set_visibility(pr_main,
scene_preview,
view_layer,
static_cast<ePreviewType>(mat_copy->pr_type),
Member

Use functional style cast for enum and POD types: ePreviewType(mat_copy->pr_type)

Use functional style cast for enum and POD types: `ePreviewType(mat_copy->pr_type)`
Kdaf marked this conversation as resolved
@ -0,0 +479,4 @@
static void preview_render(ShaderNodesPreviewJob &job_data)
{
/* Get the stuff from the builtin preview dbase. */
Scene *sce = preview_prepare_scene(job_data.bmain, job_data.scene, G.pr_main, job_data.mat_copy);
Member

sce -> scene

`sce` -> `scene`
Kdaf marked this conversation as resolved
@ -141,2 +141,4 @@
void *tbh = nullptr;
bool (*prepare_viewlayer)(void *handle, struct ViewLayer *vl, struct Depsgraph *depsgraph);
void *pvh;
Member

Document what these two fields are expected to do. Especially pvh which is a bit cryptic

Document what these two fields are expected to do. Especially `pvh` which is a bit cryptic
Author
Member

@HooglyBoogly Can pvh be renamed to something like prepare_vl_handle to be more readable or should I follow the other types and put something short like pvlh which is obviously cryptic ?

@HooglyBoogly Can `pvh` be renamed to something like `prepare_vl_handle` to be more readable or should I follow the other types and put something short like `pvlh` which is obviously cryptic ?
Member

Personally I don't know why we would ever prefer that sort of cryptic name. Where they're used in existing code, they seem to come from a decade ago or something

Personally I don't know why we would ever prefer that sort of cryptic name. Where they're used in existing code, they seem to come from a decade ago or something
Author
Member

I'll rename the others in another PR after this one so that it benefits everyone's eyes.

I'll rename the others in another PR after this one so that it benefits everyone's eyes.
Kdaf marked this conversation as resolved
Colin Marmond added 3 commits 2023-08-04 17:23:09 +02:00
Colin Marmond added 2 commits 2023-08-04 17:51:31 +02:00
Author
Member

@HooglyBoogly I normally fixed the issue you had with the previews not always showing.

I think it would be better to fix the confusing preview button behaviour in another patch (it also regards the compositor).

@HooglyBoogly I normally fixed the issue you had with the previews not always showing. I think it would be better to fix the confusing preview button behaviour in another patch (it also regards the compositor).
Colin Marmond requested review from Hans Goudey 2023-08-04 17:53:56 +02:00
Hans Goudey reviewed 2023-08-04 18:33:16 +02:00
@ -0,0 +36,4 @@
}
};
void spacenode_free_previews(wmWindowManager &wm, SpaceNode &snode);
Member

Looking good. The spacenode prefix is also unnecessary now because of the namespace.

Looking good. The `spacenode` prefix is also unnecessary now because of the namespace.
Kdaf marked this conversation as resolved
@ -0,0 +41,4 @@
NestedTreePreviews &tree_previews,
const bNode &node);
void node_release_preview_ibuf(NestedTreePreviews &tree_previews);
NestedTreePreviews *spacenode_get_nested_previews(const bContext &C, SpaceNode &sn);
Member

sn -> snode or space_node

Same elsewhere.

`sn` -> `snode` or `space_node` Same elsewhere.
Kdaf marked this conversation as resolved
@ -0,0 +51,4 @@
#include "ED_screen.h"
#include "node_intern.hh"
using namespace blender;
Member

Remove using namespace blender

Remove `using namespace blender`
Kdaf marked this conversation as resolved
@ -0,0 +74,4 @@
NodeSocketPair mat_displacement_copy;
/* TreePath used to locate the nodetree.
* bNodeTreePath elements have some listbase pointers which should not be used. */
blender::Vector<bNodeTreePath *> treepath_copy;
Member

Specifying blender:: manually is now unnecessary inside the namesapce

Specifying `blender::` manually is now unnecessary inside the namesapce
Kdaf marked this conversation as resolved
@ -0,0 +239,4 @@
* \{ */
/* Return the socket used for previewing the node (should probably follow more precise rules). */
static bNodeSocket *node_find_preview_socket(const bNode &node)
Member

C/C++ allow you to break const correctness and retrieve a mutable pointer from the node. Better to return a const bNodeSocket * here :)

C/C++ allow you to break const correctness and retrieve a mutable pointer from the node. Better to return a `const bNodeSocket *` here :)
Author
Member

You mean that it would be better to use a const_cast for every use of this function ?
For consistency maybe I should remove the const in const bNode &node ?

You mean that it would be better to use a const_cast for every use of this function ? For consistency maybe I should remove the const in `const bNode &node` ?
Member

Either that or return a const socket, just don't do the const -> no const change here

Either that or return a const socket, just don't do the `const -> no const` change here
Kdaf marked this conversation as resolved
@ -0,0 +242,4 @@
static bNodeSocket *node_find_preview_socket(const bNode &node)
{
LISTBASE_FOREACH (bNodeSocket *, socket, &node.outputs) {
if (socket->is_visible()) {
Member

Could probably use socket declarations and is_default_link_socket, but that could be adjusted later

Could probably use socket declarations and `is_default_link_socket`, but that could be adjusted later
Author
Member

I've added it to my todo list so that I'll take more time to improve that socket seeking. #110353

I've added it to my todo list so that I'll take more time to improve that socket seeking. #110353
Kdaf marked this conversation as resolved
Colin Marmond added 1 commit 2023-08-04 20:55:14 +02:00
Member

This is still acting funny in some simple tests for me.
Also, I wonder if it would be better to keep showing the old preview rather than clearing it before updating with the new one. That would avoid the flickering, which is fairly distracting.

This is still acting funny in some simple tests for me. Also, I wonder if it would be better to keep showing the old preview rather than clearing it before updating with the new one. That would avoid the flickering, which is fairly distracting.
Author
Member

Brecht already suggested that keeping of the image. It's a bit more complicated: when the new render is started, a new renderresult is created and the passes are allocated. As soon as they are allocated, there is no way to distinguish a rendered preview from a clear allocation.
One thing Brecht mentioned was to trick with lazy allocation which are currently disabled for preview jobs.

The flickering in your case is probably due to the recompilation of the shaders which is a bit long, and there is no (easy) way to avoid that I guess.
I noticed the same slowdowns when playing with the Subsurface Scattering.

Did you experienced not having a preview appearing at all ?

Brecht already suggested that keeping of the image. It's a bit more complicated: when the new render is started, a new renderresult is created and the passes are allocated. As soon as they are allocated, there is no way to distinguish a rendered preview from a clear allocation. One thing Brecht mentioned was to trick with lazy allocation which are currently disabled for preview jobs. The flickering in your case is probably due to the recompilation of the shaders which is a bit long, and there is no (easy) way to avoid that I guess. I noticed the same slowdowns when playing with the Subsurface Scattering. Did you experienced not having a preview appearing at all ?
Member

As soon as they are allocated, there is no way to distinguish a rendered preview from a clear allocation.

Might be missing something, but could you just store an extra variable that tells you whether the preview is finished? I think keeping the old previews around would help a lot with making this look polished and professional.

Did you experienced not having a preview appearing at all ?

Yeah, that's in the first part of the video.

>As soon as they are allocated, there is no way to distinguish a rendered preview from a clear allocation. Might be missing something, but could you just store an extra variable that tells you whether the preview is finished? I think keeping the old previews around would help a lot with making this look polished and professional. >Did you experienced not having a preview appearing at all ? Yeah, that's in the first part of the video.
Author
Member

That's more stuff than I can do this evening, I'll figure it out next week.

That's more stuff than I can do this evening, I'll figure it out next week.
Colin Marmond added 1 commit 2023-08-07 10:06:02 +02:00
Can even benefit to a faster drawing phase.
Colin Marmond added 1 commit 2023-08-07 10:46:47 +02:00
Merge branch 'main' into shader-node-previews-GSOC
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
d6b02d9684

@blender-bot package

@blender-bot package
Member

Package build started. Download here when ready.

Package build started. [Download here](https://builder.blender.org/download/patch/PR110065) when ready.

I will merge this as an experimental feature, so that we can polish it further there. That will make it easier to review further improvements as their own pull requests, and easier to enable later in the release cycle.

I will merge this as an experimental feature, so that we can polish it further there. That will make it easier to review further improvements as their own pull requests, and easier to enable later in the release cycle.
Brecht Van Lommel added 2 commits 2023-08-08 16:48:04 +02:00
Merge branch 'main' into HEAD
All checks were successful
buildbot/vexp-code-patch-coordinator Build done.
1900454118

@blender-bot build

@blender-bot build
Brecht Van Lommel merged commit b8eb7d18e9 into main 2023-08-08 17:36:14 +02:00
Brecht Van Lommel deleted branch shader-node-previews-GSOC 2023-08-08 17:36:16 +02:00
Member

Sorry if I'm late to this review and maybe I'm missing something, but wouldn't it be simpler (for shader nodes) to use preview_render_type from materials instead of a new enum?

The 3D option already reacts to changes in the material preview_render_type.

Test it by changing:
row.prop(overlay, "preview_shape", expand=True)
to:
row.prop(context.material, "preview_render_type", text="")


Another flaw with this Flat/3D toggle is that if you choose 3D and set the material preview to Flat (square icon), then the shader nodes preview Flat and 3D look the same:
flat_vs_3d.gif

Sorry if I'm late to this review and maybe I'm missing something, but wouldn't it be simpler (for shader nodes) to use `preview_render_type` from materials instead of a new enum? The `3D` option already reacts to changes in the material `preview_render_type`. <video src="/attachments/3ed4f311-6ad3-4fd2-9674-27f52e9b12a0" title="2023-08-18_16.32.27.mp4" controls></video> Test it by changing: `row.prop(overlay, "preview_shape", expand=True)` to: `row.prop(context.material, "preview_render_type", text="")` ---- Another flaw with this Flat/3D toggle is that if you choose 3D and set the material preview to *Flat* (square icon), then the shader nodes preview **Flat** and **3D** look the same: ![flat_vs_3d.gif](/attachments/2c100e0f-9d21-46d1-9b16-fbd24e248862)

@pablovazquez This change was made in #110958. There is also more context for this on devtalk:
https://devtalk.blender.org/t/gsoc-2023-add-a-shader-preview-node-feedback/28334

The reason is that the material preview shape also affects the asset preview. And you might want to see flat previews in the node editor, but then you have to remember to switch it back to fix your asset preview. And that's easy to forget.

I think we can have both in the popover. A toggle between flat/3D, and then when it is set to 3D you can choose the shape. The duplicate flat preview type is still not great then, maybe a different name will help.

An additional issue is that the flat preview is mostly useful for textures, but not as much for BSDFs. Maybe that setting should only not affect non-shader sockets. But that may be hard to communicate or understand. Not sure yet what the best solution is here.

@pablovazquez This change was made in #110958. There is also more context for this on devtalk: https://devtalk.blender.org/t/gsoc-2023-add-a-shader-preview-node-feedback/28334 The reason is that the material preview shape also affects the asset preview. And you might want to see flat previews in the node editor, but then you have to remember to switch it back to fix your asset preview. And that's easy to forget. I think we can have both in the popover. A toggle between flat/3D, and then when it is set to 3D you can choose the shape. The duplicate flat preview type is still not great then, maybe a different name will help. An additional issue is that the flat preview is mostly useful for textures, but not as much for BSDFs. Maybe that setting should only not affect non-shader sockets. But that may be hard to communicate or understand. Not sure yet what the best solution is here.
Sign in to join this conversation.
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset System
Interest
Audio
Interest
Automated Testing
Interest
Blender Asset Bundle
Interest
BlendFile
Interest
Collada
Interest
Compatibility
Interest
Compositing
Interest
Core
Interest
Cycles
Interest
Dependency Graph
Interest
Development Management
Interest
EEVEE
Interest
EEVEE & Viewport
Interest
Freestyle
Interest
Geometry Nodes
Interest
Grease Pencil
Interest
ID Management
Interest
Images & Movies
Interest
Import Export
Interest
Line Art
Interest
Masking
Interest
Metal
Interest
Modeling
Interest
Modifiers
Interest
Motion Tracking
Interest
Nodes & Physics
Interest
OpenGL
Interest
Overlay
Interest
Overrides
Interest
Performance
Interest
Physics
Interest
Pipeline, Assets & IO
Interest
Platforms, Builds & Tests
Interest
Python API
Interest
Render & Cycles
Interest
Render Pipeline
Interest
Sculpt, Paint & Texture
Interest
Text Editor
Interest
Translations
Interest
Triaging
Interest
Undo
Interest
USD
Interest
User Interface
Interest
UV Editing
Interest
VFX & Video
Interest
Video Sequencer
Interest
Virtual Reality
Interest
Vulkan
Interest
Wayland
Interest
Workbench
Interest: X11
Legacy
Asset Browser Project
Legacy
Blender 2.8 Project
Legacy
Milestone 1: Basic, Local Asset Browser
Legacy
OpenGL Error
Meta
Good First Issue
Meta
Papercut
Meta
Retrospective
Meta
Security
Module
Animation & Rigging
Module
Core
Module
Development Management
Module
EEVEE & Viewport
Module
Grease Pencil
Module
Modeling
Module
Nodes & Physics
Module
Pipeline, Assets & IO
Module
Platforms, Builds & Tests
Module
Python API
Module
Render & Cycles
Module
Sculpt, Paint & Texture
Module
Triaging
Module
User Interface
Module
VFX & Video
Platform
FreeBSD
Platform
Linux
Platform
macOS
Platform
Windows
Priority
High
Priority
Low
Priority
Normal
Priority
Unbreak Now!
Status
Archived
Status
Confirmed
Status
Duplicate
Status
Needs Info from Developers
Status
Needs Information from User
Status
Needs Triage
Status
Resolved
Type
Bug
Type
Design
Type
Known Issue
Type
Patch
Type
Report
Type
To Do
No Milestone
No project
No Assignees
7 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#110065
No description provided.