GPv3: Named Layer Selection node #113908

Merged
Dalai Felinto merged 3 commits from dfelinto/blender:grease-nodes-named-layer into main 2023-10-23 15:49:47 +02:00

This is part of the second milestone for the Grease Pencil and Geometry Nodes integration. It inclues:

  • Named Layer Selection Field
  • Support for a "Selection Field" option for boolean sockets to see them in the modifier.
  • Named Layer Selection Field

Known limitations to be addressed separately:

  • We are not warning/keeping track of the named layers.
  • There is no lookup for layers (groups) yet.

Also I found an unrelated bug (#113925): If the file is saved with the name already setup, the Layer domain fails to evaluate until you force it to re-calculate. It is not related to this patch (see my comments here), but it may affect you testing it.


See test file attached.

This is part of the second milestone for the Grease Pencil and Geometry Nodes integration. It inclues: * Named Layer Selection Field * Support for a "Selection Field" option for boolean sockets to see them in the modifier. * Named Layer Selection Field Known limitations to be addressed separately: - We are not warning/keeping track of the named layers. - There is no lookup for layers (groups) yet. --- Also I found an unrelated bug (#113925): If the file is saved with the name already setup, the Layer domain fails to evaluate until you force it to re-calculate. It is not related to this patch (see my comments here), but it may affect you testing it. --- See test file attached.
Author
Owner

My test file: grease-nodes-named-layer.blend

As the file is, the (1) bug is visible upon opening it.

To test it disregarding this bug, just toggle the Modifier on/off.

My test file: [grease-nodes-named-layer.blend](/attachments/f7bdbb25-a68b-457e-b0ee-3851900ad262) As the file is, the (1) bug is visible upon opening it. To test it disregarding this bug, just toggle the Modifier on/off.
Member
  • The "Named Attributes" node editor overlay could be changed to "Named Dependencies" The "1 Named Layer" would still show on a separate row from the existing warnings though.
  • The node could be named "Named Layer Selection"
  • Grease pencil data-blocks will need to reevaluate ID_RECALC_GEOMETRY when a layer name changes

The search could be implemented later, for now the node is still useful without it

- [ ] The "Named Attributes" node editor overlay could be changed to "Named Dependencies" The "1 Named Layer" would still show on a separate row from the existing warnings though. - [x] The node could be named "Named Layer Selection" - [x] Grease pencil data-blocks will need to reevaluate `ID_RECALC_GEOMETRY` when a layer name changes The search could be implemented later, for now the node is still useful without it
Dalai Felinto changed title from GPv3: Named Layer node to GPv3: Named Layer Selection node 2023-10-19 15:42:28 +02:00
Author
Owner

The lack of ID_RECALC_GEOMETRY was a bug in main. Fixed on cb0d260c4b.

The lack of ID_RECALC_GEOMETRY was a bug in main. Fixed on cb0d260c4b790b2590563e6242bf0bcd3bc0e07d.
Dalai Felinto force-pushed grease-nodes-named-layer from 19297a50d9 to 1a1eb4aedc 2023-10-19 17:19:08 +02:00 Compare
Author
Owner

I got it to work for groups as well, see test file attached to the PR description.

For now I decided to not add the named layer to the warning system. So I even went ahead and reverted the renaming change. Doing so would make the patch too unnecessarily big.

I got it to work for groups as well, see test file attached to the PR description. For now I decided to not add the named layer to the warning system. So I even went ahead and reverted the renaming change. Doing so would make the patch too unnecessarily big.
Dalai Felinto force-pushed grease-nodes-named-layer from 1f07af018b to 10d81a5383 2023-10-20 11:11:27 +02:00 Compare
Dalai Felinto force-pushed grease-nodes-named-layer from 10d81a5383 to 28bbfe5029 2023-10-20 14:51:01 +02:00 Compare
Dalai Felinto force-pushed grease-nodes-named-layer from 28bbfe5029 to 3a7f05c293 2023-10-20 17:32:48 +02:00 Compare
Dalai Felinto force-pushed grease-nodes-named-layer from 3a7f05c293 to a53ce52fca 2023-10-20 17:33:16 +02:00 Compare
Dalai Felinto force-pushed grease-nodes-named-layer from a53ce52fca to cfabc3590a 2023-10-20 17:37:42 +02:00 Compare
Iliya Katushenock reviewed 2023-10-20 17:46:38 +02:00
@ -317,2 +317,4 @@
};
class NamedLayerSelectionFieldInput final : public bke::GeometryFieldInput {
std::string layer_name_;

Can be const

Can be const
Member

I'm pretty sure storing values as const in a class means move construction won't work. Plus that's what making the class itself const is for. So I wouldn't do this.

I'm pretty sure storing values as const in a class means move construction won't work. Plus that's what making the class itself const is for. So I wouldn't do this.
mod_moder marked this conversation as resolved
@ -319,0 +321,4 @@
public:
NamedLayerSelectionFieldInput(const std::string layer_name)
: bke::GeometryFieldInput(CPPType::get<bool>(), "Named Layer node"), layer_name_(layer_name)

Move name. Argument shouldn't be const.

Move name. Argument shouldn't be const.
dfelinto marked this conversation as resolved
@ -0,0 +16,4 @@
static void node_geo_exec(GeoNodeExecParams params)
{
const std::string name = params.extract_input<std::string>("Name");

Delete const if name will be moved.

Delete `const` if name will be moved.
dfelinto marked this conversation as resolved
@ -0,0 +22,4 @@
return;
}
Field<bool> selection_field{std::make_shared<blender::bke::NamedLayerSelectionFieldInput>(name)};

Move name.

Move name.
dfelinto marked this conversation as resolved
Falk David requested changes 2023-10-20 18:04:35 +02:00
Falk David left a comment
Member

I left some comments on the grease pencil core stuff.

I left some comments on the grease pencil core stuff.
@ -2415,6 +2430,35 @@ blender::bke::greasepencil::LayerGroup *GreasePencil::find_layer_group_by_name(
return this->root_group().find_group_by_name(name);
}
blender::Vector<int> GreasePencil::find_layer_index_by_name(
Member

I'd rather make this a
IndexMask GreasePencil::layer_selection_by_name(const blender::StringRefNull name, IndexMaskMemory &memory) const
And then return IndexMask::from_indices(indices, memory);.

I'd rather make this a `IndexMask GreasePencil::layer_selection_by_name(const blender::StringRefNull name, IndexMaskMemory &memory) const` And then return `IndexMask::from_indices(indices, memory);`.
dfelinto marked this conversation as resolved
@ -2418,0 +2436,4 @@
using namespace blender::bke::greasepencil;
blender::Vector<blender::StringRef> layer_name_vector;
const LayerGroup *layer_group = this->find_layer_group_by_name(name);
Member

This should really use something like const TreeNode *node = this->find_node_by_name(name);.
I'm moving away from duplicated these for layers and groups and just have a single API.

This should really use something like `const TreeNode *node = this->find_node_by_name(name);`. I'm moving away from duplicated these for layers and groups and just have a single API.
dfelinto marked this conversation as resolved
@ -2418,0 +2444,4 @@
layer_name_vector.append(name);
}
blender::Vector<int> layer_index_vector;
Member

layer_index_vector -> layer_indices

`layer_index_vector` -> `layer_indices`
dfelinto marked this conversation as resolved
@ -2418,0 +2447,4 @@
blender::Vector<int> layer_index_vector;
for (const int layer_index : this->layers().index_range()) {
const Layer &layer = *this->layers()[layer_index];
if (!layer_name_vector.contains(blender::StringRef(layer.name()))) {
Member

Rather than getting a vector of the names and comparing strings here (which is quite expensive), we can do this is a simpler way. As suggested above, get the node by name. Then if the node is a layer, we just get the index of that layer using this->layers().first_index(node.as_layer()) and return. Otherwise if the node is a group (node.is_group()) we iterate over all the layers like you do here but then we just need to do:

if (layer.is_child_of(node.as_group()) {
   layer_indices.append(layer_index)
}
Rather than getting a vector of the names and comparing strings here (which is quite expensive), we can do this is a simpler way. As suggested above, get the `node` by name. Then if the node is a layer, we just get the index of that layer using `this->layers().first_index(node.as_layer())` and return. Otherwise if the node is a group (`node.is_group()`) we iterate over all the layers like you do here but then we just need to do: ``` if (layer.is_child_of(node.as_group()) { layer_indices.append(layer_index) } ```
dfelinto marked this conversation as resolved
@ -64,8 +64,9 @@ typedef enum NodeTreeInterfaceSocketFlag {
NODE_INTERFACE_SOCKET_HIDE_IN_MODIFIER = 1 << 3,
NODE_INTERFACE_SOCKET_COMPACT = 1 << 4,
NODE_INTERFACE_SOCKET_SINGLE_VALUE_ONLY = 1 << 5,
NODE_INTERFACE_SOCKET_LAYER = 1 << 6,
Member

NODE_INTERFACE_SOCKET_LAYER_SELECTION ?

`NODE_INTERFACE_SOCKET_LAYER_SELECTION` ?
dfelinto marked this conversation as resolved
Member

@dfelinto There are a few things in my comments (like find_node_by_name and layer.is_child_of) that I'll do in separate commits in main.

@dfelinto There are a few things in my comments (like `find_node_by_name` and `layer.is_child_of`) that I'll do in separate commits in main.
Member
Committed https://projects.blender.org/blender/blender/commit/f6c8ddda and https://projects.blender.org/blender/blender/commit/e433a8eaaf7
Dalai Felinto force-pushed grease-nodes-named-layer from cfabc3590a to 9521ec6782 2023-10-20 19:35:15 +02:00 Compare
Falk David requested changes 2023-10-20 19:39:14 +02:00
Falk David left a comment
Member

Some more comments :)

Some more comments :)
@ -514,0 +514,4 @@
/**
* Returns a `Vector` of names of all the layers within this group.
*/
Vector<StringRef> layer_names() const;
Member

Should be removed.

Should be removed.
dfelinto marked this conversation as resolved
@ -529,0 +548,4 @@
if (domain == ATTR_DOMAIN_LAYER) {
Array<bool> selection(mask.min_array_size(), false);
mask.foreach_index([&](const int64_t i) { selection[i] = true; });
Member

Use IndexMask::to_bools(...)

Use `IndexMask::to_bools(...)`
dfelinto marked this conversation as resolved
@ -2388,0 +2413,4 @@
const int64_t index = this->layers().first_index(&node->as_layer());
return blender::IndexMask::from_indices(Span{index}, memory);
}
Member

Even though we don't have another TreeNode type, feels better to use
else if (node->is_group()) and return {} otherwise.

Even though we don't have another `TreeNode` type, feels better to use `else if (node->is_group())` and `return {}` otherwise.
dfelinto marked this conversation as resolved
Hans Goudey reviewed 2023-10-20 19:39:35 +02:00
@ -318,1 +318,4 @@
class NamedLayerSelectionFieldInput final : public bke::GeometryFieldInput {
private:
const std::string layer_name_;
Member

I disagreed with Iliya's comment about using const for this variable. Generally that doesn't make sense for fields stored by-value in structs. The constness of the struct itself is the real thing that matters.

I disagreed with Iliya's comment about using const for this variable. Generally that doesn't make sense for fields stored by-value in structs. The constness of the struct itself is the real thing that matters.
dfelinto marked this conversation as resolved
@ -0,0 +23,4 @@
}
Field<bool> selection_field{
std::make_shared<blender::bke::NamedLayerSelectionFieldInput>(std::move(name))};
Member

No need for blender:: here

No need for `blender::` here
dfelinto marked this conversation as resolved
@ -0,0 +25,4 @@
Field<bool> selection_field{
std::make_shared<blender::bke::NamedLayerSelectionFieldInput>(std::move(name))};
params.set_output("Selection", std::move(selection_field));
return;
Member

Unnecessary return

Unnecessary `return`
dfelinto marked this conversation as resolved
Dalai Felinto force-pushed grease-nodes-named-layer from 9521ec6782 to 6ef2d1ce2d 2023-10-20 19:40:40 +02:00 Compare
Dalai Felinto force-pushed grease-nodes-named-layer from 6ef2d1ce2d to e9a311e2bf 2023-10-20 20:13:32 +02:00 Compare
Author
Owner

FYI there is an issue right now where sometimes the file has spurious data as the string. It shows when I toggle "Layer Selection".

~~FYI there is an issue right now where sometimes the file has spurious data as the string. It shows when I toggle "Layer Selection".~~
Falk David approved these changes 2023-10-21 14:51:42 +02:00
Falk David left a comment
Member

Looks good from the grease pencil core side now, so I will accept this.

Looks good from the grease pencil core side now, so I will accept this.
Dalai Felinto force-pushed grease-nodes-named-layer from 76263a032e to 7a18bff7f4 2023-10-21 21:04:20 +02:00 Compare
Author
Owner

Patch fully working. I fixed the issue of spurious data by simply not trying to re-use the previous data - see `id_property_create_from_socket .

Patch fully working. I fixed the issue of spurious data by simply not trying to re-use the previous data - see `id_property_create_from_socket .
Dalai Felinto requested review from Hans Goudey 2023-10-21 21:04:58 +02:00
Falk David approved these changes 2023-10-23 10:36:12 +02:00
Hans Goudey reviewed 2023-10-23 10:43:35 +02:00
@ -121,1 +121,4 @@
case SOCK_BOOLEAN: {
if (socket.is_layer_selection_field()) {
/* It is basically a SOCK_STRING minus trying to re-use the previous value
* Otherwise we get spurious data, since the previous type was SOCK_BOOLEAN. */
Member

This isn't really about reusing the old value, but creating the IDProperty in the first place from the socket. The old value is copied over elsewhere.

It's not clear that "the previous type was SOCK_BOOLEAN" is always true, I'd imagine it's only true when switching types.

This isn't really about reusing the old value, but creating the IDProperty in the first place from the socket. The old value is copied over elsewhere. It's not clear that "the previous type was SOCK_BOOLEAN" is always true, I'd imagine it's only true when switching types.
dfelinto marked this conversation as resolved
Dalai Felinto force-pushed grease-nodes-named-layer from 7a18bff7f4 to 30a2d66f1c 2023-10-23 12:04:31 +02:00 Compare
Hans Goudey reviewed 2023-10-23 12:28:00 +02:00
Hans Goudey left a comment
Member

Only larger comment is that it might be better to avoid adding is_layer_selection_field as a method on bNodeTreeInterfaceSocket. This is very geometry nodes specific, and while the flag is one thing (the only way to store this without duplicating an unreasonable amount of DNA), it's nice if the API provided by that struct stays more generic. This is a pain point with the existing node system design, so it's definitely a compromise. Sorry I didn't mention that earlier.

The rest of the code looks pretty nice :) Just had a couple small comments.

Only larger comment is that it might be better to avoid adding `is_layer_selection_field` as a method on `bNodeTreeInterfaceSocket`. This is very geometry nodes specific, and while the flag is one thing (the only way to store this without duplicating an unreasonable amount of DNA), it's nice if the API provided by that struct stays more generic. This is a pain point with the existing node system design, so it's definitely a compromise. Sorry I didn't mention that earlier. The rest of the code looks pretty nice :) Just had a couple small comments.
@ -319,0 +321,4 @@
std::string layer_name_;
public:
NamedLayerSelectionFieldInput(const std::string layer_name)
Member

const std::string layer_name -> std::string layer_name

Because you move from this variable, better not to declare it const

`const std::string layer_name` -> `std::string layer_name` Because you move from this variable, better not to declare it const
dfelinto marked this conversation as resolved
@ -529,0 +557,4 @@
}
Array<bool> selection(mask.min_array_size(), true);
return VArray<bool>::ForContainer(std::move(selection));
Member

VArray::ForSingle should handle this much more optimally :)

`VArray::ForSingle` should handle this much more optimally :)
Author
Owner

Maybe I'm missing something here, but ForSingle assumes we have the same value for the entire VArray. And in this case we have a different value for each different layer.

Also, this is also extremely fast anyways since it is iterating over the layers, not the geometry. But I'm ok using a more optimized API call. I just don't understand how to do it here.

Maybe I'm missing something here, but ForSingle assumes we have the same value for the entire VArray. And in this case we have a different value for each different layer. Also, this is also extremely fast anyways since it is iterating over the layers, not the geometry. But I'm ok using a more optimized API call. I just don't understand how to do it here.
Author
Owner

Nevermind my reply. I thought you were talking about the other ForContainer (the one for the layers). Will do

Nevermind my reply. I thought you were talking about the other ForContainer (the one for the layers). Will do
dfelinto marked this conversation as resolved
Dalai Felinto force-pushed grease-nodes-named-layer from 96279d0f2e to bce26e5571 2023-10-23 14:18:58 +02:00 Compare
Hans Goudey approved these changes 2023-10-23 15:04:45 +02:00
Author
Owner

Thanks everyone for the help and code review!

Thanks everyone for the help and code review!
Dalai Felinto merged commit e42281084b into main 2023-10-23 15:49:47 +02:00
Dalai Felinto deleted branch grease-nodes-named-layer 2023-10-23 15:49:48 +02:00
Sign in to join this conversation.
No reviewers
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
4 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#113908
No description provided.