Nodes: Move preview images to an overlay #108001
No reviewers
Hans Goudey
Pablo Vazquez
Brecht Van Lommel
Jeroen Bakker
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
8 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#108001
Loading…
Reference in New Issue
No description provided.
Delete Branch "Kdaf/blender:main"
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?
Move the node previews to the tooltip region, atop each nodes.
It allows nodes to keep the same size when the preview is toggled, which is more convenient for large nodes and large nodetrees.
The preview has to be drawn from
node_draw_extra_info_panelbecause there could be overlapping between info text and the preview.When the node is out of the view, it also has to make sure that the preview is also out of the view before exiting the draw function.
It would be helpful to add an example image
For reference, in this topic various users preferred the placement above the node. This is also consistent with the way geometry nodes show additional information like timings.
https://devtalk.blender.org/t/gsoc-2023-add-a-shader-preview-node-feedback/28334/
I'm fine with this if it's what users prefer.
For the design, I don't think we need the double border. I'd make the preview image take up the full size of the box and make the corners square.
@ -2481,3 +2454,3 @@const int color_id = node_get_colorid(tree_draw_ctx, node);node_draw_extra_info_panel(tree_draw_ctx, snode, node, block);node_draw_extra_info_panel(tree_draw_ctx, snode, node, NULL, block);NULL->nullptrin C++ code (here and above)@brecht I cant fit the preview without space around, it makes it ugly with a weird bevel under. I think I'll just put the preview above the extra info texts so that it doesn't need to display the bevels around.
If there is both extra info and a preview, maybe it looks best to have the extra info on top with the preview below. So the extra info can have rounded corners and still have things align nicely.
But that may be more a theoretical concern at this point since no node type has both currently.
I see, I'll do that instead. I think the preview is more important than the infos for the end user, and it might make more sense to have it steady.
Package build started. Download here when ready.
@blender-bot package
Package build started. Download here when ready.
Code-wise seems fine and think it is what users are more familiar with looking at other texture/shader/compositors and what some add-ons are currently trying to mimic.
It might change the interactivity with nodes in general, but that requires some feedback from users and tweaking. Eg what is the expected selection hotspot of the nodes, would that include the info panel? Currently the hotspot is smaller as the previews aren't part of the hotspot zone.
As the info panel is a bit disconnected from the node, do we need to render it in a different pass (together with the links or a new one), so it gets occluded when dragging a node (I expect this should be consistent for any node-editor) but not sure what would be better. Might also be a different layer for dragging vs regular drawing.
Would like to have some feedback from UI team and Nodes team. They might already have made an idea/decision on these topics.
@ -2076,6 +2039,7 @@ static void node_draw_extra_info_row(const bNode &node,static void node_draw_extra_info_panel(TreeDrawContext &tree_draw_ctx,const SpaceNode &snode,const bNode &node,const rctf &rct,For clarity use better descriptive parameter names.
panel_rect?
I'll better call it node_rect, because it is the node rect on which the function calculates the extra_info rect.
The main reason to do this is for links - they connect 2 nodes. These nodes can be in different passes and we don't want to break the link at intermediate nodes.
Frame nodes are drawn from the bottom to ensure that their hierarchical structure is directed towards the user.
Not sure if frame nodes can support resizing with this data. Right now it's just statically expanded to fit.
On the other hand, the overlay can still be visible when node selected (due to node drawing is reordered) (as well as the error message, like the icon in the corner of the header).
I'm not sure if we want to draw parts of a node (those preview data, timings, ...) on top of other intermediate nodes, detached from the original.
So it seems that the overleys should be the same pass with their nodes.
@Kdaf I just realized this is a node overlay, not a tooltip (naming in pull request).
Move the node previews to the tooltip regionto Move the node previews to the overlay regionMove the node previews to the overlay regionto Nodes : Move the node previews to the overlay regionI like the previews on top, as an overlay that can be globally toggled (and per node too).
Previews would show immediately above the node, any timings/attributes or other info will always draw above the preview.
There is some minimal padding around the preview to align with the info text and node padding. And rounded corners matching the node curving, such as the Preview panel in Material properties.
In the future this style should be used by any other previews such as the Compositor, for consistency.
I dont know what it would look like, because the info panel would change size when toggling the previews. I think it would be a bit inconsistent to have this option I wonder.
Imho, the node infos should be toggleable as a whole, not individual parts of it.
I think giving users more precise control would generally be helpful. Geometry nodes already has toggles for individual parts of the overlay.
Note there will be no timings for the shader nodes anytime soon, it's not practical to do granular timing like that in rendering. The double border feels a bit redundant then, but I guess it's ok for consistency with other node types.
This PR changes the preview for compositing nodes already, it's not a future thing.
Ok ok guys, I'll do like that in a few days.
@pablovazquez I wonder if we really need to have rounded corners for the image. It is a bit expensive to do it I guess.
If we were looking at a 3d render, the rounded corners would overlap the background of the blank.
But is it reasonable to do them for an image that is a procedural texture?
At it is not clear yet, I ended with a slight outline going a bit above the preview in the corners.
From end-user performance point of view this is not measurable.
@Jeroen-Bakker I'm not sure what you mean. You mean that rounding the image is not an issue, and that the user wont notice the difference ?
@Kdaf Drawing an overlay is really very cheap. Such a corner frame should not require a lot of cpu/gpu effort.
@mod_moder for the round outline of the image I understand, but for the image, I’ll probably have to draw a rounded white image and multiply the preview on it. I don’t see any other way of doing this because there is no such function already in the drawing libs.
Ah, I get it. I very superficially saw the code of the interface. But in theory, you just need to draw overlay image on the rounded polygone. True, it is better to find out from people related with the interface.
Rendering previews using
uiTemplatePrevieware already drawn rounded without actually modifying the texture. You can look how it was done there and see if that works.uiTemplatePreview first draws the texture without rounded corners. Then it will removes the borders by drawing an rounded outline where the outside color is the same as the background. It seems to be related to
UI_BTYPE_EXTRAbefore drawing the actual texture. The extra will add an additional mask.@Jeroen-Bakker, the node overlays are semi-transparent, drawing mask over the image will only work correctly if we make them opaque. Otherwise I guess we have to make a shader that can draw an image with rounded corners directly.
Personally I think it's a waste of time to make the image draw with rounded corners, we have more important things to do.
Two issues noticed in testing:
8efd6d5as an example of the changes needed inversioning_400.cc,DNA_space_defaults.handBKE_blender_version.h.@pablovazquez Can you look at the current state of this now, does the preview drawing look good enough to you?
There is simply no system right now to estimate the size of the overlay to see if the node itself needs to be drawn.
2dcb6759bbtoc7f8cab20cJust compiled the latest version and it looks great with the outline. +1
A bit unrelated to the looks but it'd be great to fix. It seems that the preview is cleared while adjusting values (so it shows a checkerboard) until it's replaced with the updated preview, which makes it feel quite flickery. Would it be possible to somehow avoid this by keeping the old preview around until the updated preview is finished calculating?
Fixed in
4b4c95cnow.Nodes : Move the node previews to the overlay regionto Nodes : Move the node previews to an overlayNodes : Move the node previews to an overlayto Nodes: Move previews to an overlayNodes: Move previews to an overlayto Nodes: Move preview images to an overlayApproving this UI wise (not code). Super nice improvement!
It seems that if node drawing clipping just takes into account the size of the maximum overlay, would that be reasonable for the initial variant.
Not sure I understand the question. If computing the exact bounds is relatively expensive then it can use some bigger bound that is faster to compute.
@brecht Yes, that's what I meant. The preview frame has a size, depending only on the presence of an image and the size of the node. So checking their maximum sizes would be the fastest.
The problem with precise sizing is that it relies on logging and creating all the data that the overlay draws. Not sure if clipping makes much sense if the entire overlay still needs to be computed.
@brecht @Jeroen-Bakker @HooglyBoogly Could you take a look at this code wise during the week ? So that I can move on.
I'm still seeing this issue, was this supposed to be fixed?
I think that clipping task should definitely be done, but I'm not sure to integrate it to this patch. Imho this patch is just about moving UI things, and maybe someone (I can) do another patch for the overlay clipping.
I think you mean that that clipping patch should be done before committing this one ?
Yes, I think the clipping issue should be solved before this is committed.
So, I updated this patch by adding the total rectangle to the nodes so that the overlay is also considerer for the size of the node.
The workaround is to load the overlay's datas in the bNodeRuntime structure in order to be able to calculate the size before drawing.
It may seems that I modified many many things in
node_draw.cc, but I just moved the functions relative to the extra-infos before the update functions.Usually just code moving is something to avoid (I've been hit a lot for this :) ).
Also, it seems that all you needed was to add a measure of the largest overlay frame (node size * image factor) in the node clipping, I don't think that was not such a big change?
@mod_moder There is a need to have the exact box around the node because we use that rectangle to determine when the user clics on the overlay.
Ah, new functionality...
Why do we need to detect clicks on the overlay? I'm not sure the overlay needs to be interactive at all.
@HooglyBoogly Some users tried the patch and said that it is annoying to have a large overlay which can not be clicked. With a preview in it, the overlay will take more place, so it would make sens to be able to select it and move it with the node
Here is the result all bundled together. I think it is a bit weird to be able to click on overlays, but overlays can take half the size of the node, so the user will have half chance to click on the overlay instead of the node. It would then make sens to make te preview clickable just for moving nodes.
@Kdaf Overlay is runtime data that depends on the context of the node tree. The sizes of the frame node, this is the DNA data. You should not make the overlay affect the frame node. Try to make a copy of the node group, open it in 2 editors. But one has an over, and the other doesn't (due to unused). I haven't tested it, but I see that it shouldn't work correctly.
I'm very sorry for the different directions I'm going to. This is better now than after committing it.
Sooooo,
After discussion, it becomes clear that modifying the node rect is not something we would want in this patch, but we want to have a better handling of nodes out of the view.
It would probably still be a good idea to think about this selecting and tweaking aspect of overlays, but in another patch after this one.
Anyway, this patch is FINALLY ready for review, and I apologies for the time it took me to figure out what I needed to do and what not.
I'll update the patch description according to what it does.
The implementation seems reasonable to me. Just some comments about the style guide.
@ -267,0 +270,4 @@LISTBASE_FOREACH (ScrArea *, area, &screen->areabase) {LISTBASE_FOREACH (SpaceLink *, space, &area->spacedata) {if (space->spacetype == SPACE_NODE) {SpaceNode *snode = (SpaceNode *)space;Use
reinterpret_casthttps://wiki.blender.org/wiki/Style_Guide/C_Cpp#C.2B.2B_Type_Cast@ -2173,0 +2168,4 @@node_draw_preview(preview, &preview_rect);}/* resize the rect to draw the textual infos on top of the preview. */Comment style https://wiki.blender.org/wiki/Style_Guide/C_Cpp#Comments
@ -2188,2 +2188,4 @@{const float iconbutw = NODE_HEADER_ICON_SIZE;bNodeInstanceHash *previews =(bNodeInstanceHash *)CTX_data_pointer_get(&C, "node_previews").data;static_cast@ -2208,1 +2214,3 @@node_draw_extra_info_panel(tree_draw_ctx, snode, node, block);bNodePreview *preview = nullptr;if (node.flag & NODE_PREVIEW && previews && snode.overlay.flag & SN_OVERLAY_SHOW_PREVIEWS) {preview = (bNodePreview *)BKE_node_instance_hash_lookup(previews, key);static_cast@ -2504,1 +2504,4 @@static bool rna_SpaceNode_supports_previews(PointerRNA *ptr){return ED_node_supports_preview((SpaceNode *)ptr->data);static_castJust merged main to fix the merge conflicts in versionning_400, it can be committed to main I guess
I'll merge it into main.
@blender-bot build
Release notes updated at:
https://wiki.blender.org/wiki/Reference/Release_Notes/4.0/User_Interface#Nodes