GPv3: Named Layer Selection node #113908
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
4 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#113908
Loading…
Reference in New Issue
No description provided.
Delete Branch "dfelinto/blender:grease-nodes-named-layer"
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?
This is part of the second milestone for the Grease Pencil and Geometry Nodes integration. It inclues:
Known limitations to be addressed separately:
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.
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.
ID_RECALC_GEOMETRY
when a layer name changesThe search could be implemented later, for now the node is still useful without it
GPv3: Named Layer nodeto GPv3: Named Layer Selection nodeThe lack of ID_RECALC_GEOMETRY was a bug in main. Fixed on
cb0d260c4b
.19297a50d9
to1a1eb4aedc
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.
1f07af018b
to10d81a5383
10d81a5383
to28bbfe5029
28bbfe5029
to3a7f05c293
3a7f05c293
toa53ce52fca
a53ce52fca
tocfabc3590a
@ -317,2 +317,4 @@
};
class NamedLayerSelectionFieldInput final : public bke::GeometryFieldInput {
std::string layer_name_;
Can be const
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.
@ -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.
@ -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.@ -0,0 +22,4 @@
return;
}
Field<bool> selection_field{std::make_shared<blender::bke::NamedLayerSelectionFieldInput>(name)};
Move name.
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(
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);
.@ -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);
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.
@ -2418,0 +2444,4 @@
layer_name_vector.append(name);
}
blender::Vector<int> layer_index_vector;
layer_index_vector
->layer_indices
@ -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()))) {
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 usingthis->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:@ -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,
NODE_INTERFACE_SOCKET_LAYER_SELECTION
?@dfelinto There are a few things in my comments (like
find_node_by_name
andlayer.is_child_of
) that I'll do in separate commits in main.Committed https://projects.blender.org/blender/blender/commit/f6c8ddda and https://projects.blender.org/blender/blender/commit/e433a8eaaf7
cfabc3590a
to9521ec6782
Some more comments :)
@ -514,0 +514,4 @@
/**
* Returns a `Vector` of names of all the layers within this group.
*/
Vector<StringRef> layer_names() const;
Should be removed.
@ -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; });
Use
IndexMask::to_bools(...)
@ -2388,0 +2413,4 @@
const int64_t index = this->layers().first_index(&node->as_layer());
return blender::IndexMask::from_indices(Span{index}, memory);
}
Even though we don't have another
TreeNode
type, feels better to useelse if (node->is_group())
andreturn {}
otherwise.@ -318,1 +318,4 @@
class NamedLayerSelectionFieldInput final : public bke::GeometryFieldInput {
private:
const std::string layer_name_;
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.
@ -0,0 +23,4 @@
}
Field<bool> selection_field{
std::make_shared<blender::bke::NamedLayerSelectionFieldInput>(std::move(name))};
No need for
blender::
here@ -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;
Unnecessary
return
9521ec6782
to6ef2d1ce2d
6ef2d1ce2d
toe9a311e2bf
FYI there is an issue right now where sometimes the file has spurious data as the string. It shows when I toggle "Layer Selection".Looks good from the grease pencil core side now, so I will accept this.
76263a032e
to7a18bff7f4
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 .
@ -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. */
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.
7a18bff7f4
to30a2d66f1c
Only larger comment is that it might be better to avoid adding
is_layer_selection_field
as a method onbNodeTreeInterfaceSocket
. 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)
const std::string layer_name
->std::string layer_name
Because you move from this variable, better not to declare it const
@ -529,0 +557,4 @@
}
Array<bool> selection(mask.min_array_size(), true);
return VArray<bool>::ForContainer(std::move(selection));
VArray::ForSingle
should handle this much more optimally :)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.
Nevermind my reply. I thought you were talking about the other ForContainer (the one for the layers). Will do
96279d0f2e
tobce26e5571
Thanks everyone for the help and code review!