Node previews in the shader editor #110065
No reviewers
Labels
No Label
Interest
Alembic
Interest
Animation & Rigging
Interest
Asset Browser
Interest
Asset Browser Project Overview
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
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
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#110065
Loading…
Reference in New Issue
No description provided.
Delete Branch "Kdaf/blender:shader-node-previews-GSOC"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
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 asImBuf
images to store aRender
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.
ViewLayer
.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:
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.
I had an issue with git, so the previous pull request #109120 is closed and replaced by this one.
WIP: Node Preview in the shader editor GSOC Projectto Node Preview in the shader editor GSOC ProjectNode Preview in the shader editor GSOC Projectto Node previews in the shader editorMaybe 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)
"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.
@ -721,0 +724,4 @@
ntree.preview_refresh_state++;
ntree.ensure_topology_cache();
for (bNode *node : ntree.group_nodes()) {
bNodeTree *nested_tree = (bNodeTree *)node->id;
https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast
@ -0,0 +1,38 @@
/* SPDX-License-Identifier: GPL-2.0-or-later
* Copyright 2005 Blender Foundation */
Outdated/incorrect copyright
@ -0,0 +10,4 @@
#include "RE_pipeline.h"
#ifdef __cplusplus
extern "C" {
If this isn't included in a C file anymore, no need for
extern "C"
@ -0,0 +20,4 @@
struct NestedTreePreviews {
Render *previews_render;
int pr_size;
No need to abbreviate "preview" as "pr" here IMO
@ -2233,3 +2230,1 @@
BKE_node_instance_hash_lookup(previews, key));
if (preview_compositor) {
preview = preview_compositor->ibuf;
/* Overlay atop the node. */
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?)You mean I should do another patch just for the moving of the initialization of
bNodeInstanceHash *previews
?@ -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
@ -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
every non-shader nodes
->every non-shader node
@ -0,0 +53,4 @@
struct ShaderNodesPreviewJob {
NestedTreePreviews *tree_previews;
Material *mat_orig;
Do you have to store
mat_orig
, or could you just storemat_copy
here?@ -0,0 +57,4 @@
/* Listbase containing bNodeTreePath * guiding to the viewed nodetree of mat_orig. */
ListBase *treepath;
Scene *scene;
bool *stop;
A comment about what owns these boolean pointers would be helpful. Or maybe it isn't necessary to store them?
@ -0,0 +63,4 @@
Material *mat_copy;
bNode *mat_output_copy;
NodeSocketPair mat_displacement_copy;
ListBase treepath_copy;
This
treepath_copy
could be stored as aVector
to simplify ownership and improve type safety@ -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__));
MEM_new
should work here@ -0,0 +158,4 @@
Main *pr_main,
Material *matcopy)
{
Scene *sce;
sce
->scene
, and give the existingscene
variable a name that differentiates it@ -0,0 +315,4 @@
bNode *final_node,
bNodeSocket *final_socket)
{
for (bNodeTreePath *path = (bNodeTreePath *)treepath.last(); path->prev != nullptr;
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 :)
@ -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,
Passing
Vector
by value copies its contents. I'm guessing this should beSpan
instead.@ -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,
const Vector &
->const Span
@ -0,0 +651,4 @@
"Shader Previews",
WM_JOB_EXCL_RENDER,
WM_JOB_TYPE_RENDER_PREVIEW);
ShaderNodesPreviewJob *nt_previews = MEM_new<ShaderNodesPreviewJob>("shader previews");
MEM_new<ShaderNodesPreviewJob>("shader previews");
->MEM_new<ShaderNodesPreviewJob>(__func__);
Still gives usable text for memory leak warnings but without specifying a new string
@ -332,1 +333,4 @@
static void node_exit(wmWindowManager *wm, ScrArea *area)
{
SpaceNode *snode = (SpaceNode *)area->spacedata.first;
https://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast
@ -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;
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.
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?
@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 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?
@ -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");
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
I'm sorry, I dont understand what you mean.Ok I see
@ -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 "
Better not suggest working around a bug in the tooltip ;)
@ -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)");
"performances" shouldn't be plural, it's like "liquid" in that way
Could I say this ?
Resolution used for Shader node previews (should be changed for performance convenience)
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 toRE_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:@ -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?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 ?
Looks correct, thanks.
@ -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.
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.
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
.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 anImBuf*
around, that is freed withIMB_freeImBuf
. And then only ever replace that if there is a new image buffer with some contents?More issues I found in testing:
BKE_scene_use_previews
similar toBKE_scene_use_shading_nodes_custom
.Requesting changes.
b709502297
tob709502297
Here's an example .blend that has the problem.
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.
@brecht , I already did that #110353
I'll do that for the release note.
Colin Marmond referenced this pull request2023-08-02 18:00:04 +02:00
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.
@ -31,2 +31,4 @@
struct Main *main;
/**
* Data for the preview scene to avoid loading in multiple scenarios.
In such a global place,
the preview scene
doesn't really have meaning, maybe the comment can be a bit more specific.@ -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
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.@ -721,0 +727,4 @@
continue;
}
bNodeTree *nested_tree = reinterpret_cast<bNodeTree *>(node->id);
if (nested_tree) {
can look like this:
if (bNodeTree *nested_tree = reinterpret_cast<bNodeTree *>(node->id)) {
@ -0,0 +12,4 @@
struct bNodeTree;
struct ImBuf;
struct Render;
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. TheED_
prefix for functions can be removed as well.@ -0,0 +22,4 @@
bool restart_needed;
uint32_t previews_refresh_state;
NestedTreePreviews(const int size)
: previews_render(nullptr),
These defaults can be written like
@ -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); });
Rather than iterating over items and having the unused node variable
@ -0,0 +38,4 @@
}
};
void ED_spacenode_free_previews(wmWindowManager *wm, SpaceNode *snode);
Use references for pointers that:
@ -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);
bContext
variables are almost always named justC
@ -82,0 +91,4 @@
ePreviewType pr_type,
ePreviewRenderMethod pr_method);
struct World *ED_preview_prepare_world(struct Main *pr_main,
const struct Scene *sce,
sce
->scene
@ -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(
.get()
is usually clearer than&*
@ -0,0 +146,4 @@
static Material *duplicate_material(const Material *mat)
{
if (mat == nullptr) {
A null check should probably be at a higher level than this function.
@ -0,0 +210,4 @@
ED_preview_set_visibility(pr_main,
scene_preview,
view_layer,
static_cast<ePreviewType>(mat_copy->pr_type),
Use functional style cast for enum and POD types:
ePreviewType(mat_copy->pr_type)
@ -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);
sce
->scene
@ -141,2 +141,4 @@
void *tbh = nullptr;
bool (*prepare_viewlayer)(void *handle, struct ViewLayer *vl, struct Depsgraph *depsgraph);
void *pvh;
Document what these two fields are expected to do. Especially
pvh
which is a bit cryptic@HooglyBoogly Can
pvh
be renamed to something likeprepare_vl_handle
to be more readable or should I follow the other types and put something short likepvlh
which is obviously cryptic ?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
I'll rename the others in another PR after this one so that it benefits everyone's eyes.
@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).
@ -0,0 +36,4 @@
}
};
void spacenode_free_previews(wmWindowManager &wm, SpaceNode &snode);
Looking good. The
spacenode
prefix is also unnecessary now because of the namespace.@ -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);
sn
->snode
orspace_node
Same elsewhere.
@ -0,0 +51,4 @@
#include "ED_screen.h"
#include "node_intern.hh"
using namespace blender;
Remove
using namespace blender
@ -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;
Specifying
blender::
manually is now unnecessary inside the namesapce@ -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)
C/C++ allow you to break const correctness and retrieve a mutable pointer from the node. Better to return a
const bNodeSocket *
here :)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
?Either that or return a const socket, just don't do the
const -> no const
change here@ -0,0 +242,4 @@
static bNodeSocket *node_find_preview_socket(const bNode &node)
{
LISTBASE_FOREACH (bNodeSocket *, socket, &node.outputs) {
if (socket->is_visible()) {
Could probably use socket declarations and
is_default_link_socket
, but that could be adjusted laterI've added it to my todo list so that I'll take more time to improve that socket seeking. #110353
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.
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 ?
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.
Yeah, that's in the first part of the video.
That's more stuff than I can do this evening, I'll figure it out next week.
@blender-bot package
Package build started. Download here 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.
@blender-bot build
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 materialpreview_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:
@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.