Works the same as for Objects with options: Color, Type, Prefix, Suffix
Details
Details
- Reviewers
Campbell Barton (campbellbarton) Steve (Cruentus_Nex) - Maniphest Tasks
- T37874: Select Similar for Nodes
Diff Detail
Diff Detail
- Branch
- D288
Event Timeline
Comment Actions
Requesting some changes,
- Should be one operator, with an enum to choose what property you are selecting by, see; http://www.blender.org/documentation/blender_python_api_2_69_release/bpy.ops.object.html#bpy.ops.object.select_grouped
- Access by pressing Shift+G which opens a menu.
- Merge the code from this patch into bpy.ops.node.select_same_type() and rename operator bpy.ops.nodes.select_grouped
| source/blender/editors/space_node/node_select.c | ||
|---|---|---|
| 286 | odd names, maybe use node and node_active or so? | |
| 287 | can be a bool | |
| 289 | best use nodeGetActive | |
| 301 | best use equals_v3v3 here. | |
Comment Actions
Updated the UI interaction. Shift + G brings up a menu to select by either Type, Name, or Color.
Updated the conditional check on color to use equals_v3v3. Couldn't find one to compare the first X of a string,
and wasn't sure if I should create one. As a result the name conditional still looks quite ugly. I thought about
using strcmp but with the way it functions it ends up looking just as bad.
Changed some of the variable names.
Removed some functions that were no longer necessary like the sub-exec functions because they are now getting called from node_select_grouped(... function,
so the execs aren't needed.
None of them are posting feedback to the console which I don't understand at the moment.
Apologies for the condition of the diff, I checked the formatting in my IDE and it seems my
tab size got reset to 8 somehow. I'm manually fixing it, I just want to make sure that
this behaves as requested, check for requested additions, and perhaps find out why there's no
feedback to the console.
| source/blender/editors/space_node/node_ops.c | ||
|---|---|---|
| 85 | configure your editor not to strip white-space (avoid noisy diffs) | |
| source/blender/editors/space_node/node_select.c | ||
| 211 | indentation is weired here, also not following our code style so much http://wiki.blender.org/index.php/Dev:Doc/Code_Style | |
| 294 | This is a bit strange comparing first 3 characters is quite arbitrary and wont work right for utf8, best get the prefix/suffix of both - then compare. Blender already has this for bones. add a bone- go into editmode and press Shift+G. theres options for prefix/suffix, check how that works, Im sure it can be applied here. | |
| 325 | maybe use compare_v3v3 here? | |
| 330 | the second check isnt needed, just else { is fine | |
| 350 | node_select_same_type_np isnt used, and can likely be removed, so think theres no need to add these functions at all. | |
| 371 | still not using equals_v3v3 | |
| 454 | no need for |= here, can just assign. | |
Comment Actions
Cleaned the diff up, and the code(indentation, consistency etc).
Screen-caps of a demo:
| source/blender/editors/space_node/node_intern.h | ||
|---|---|---|
| 110 | If context is needed, its typically the first arg, but in this case think args can be struct SpaceNode *snode, struct bNode *bnode | |
| 124 | this should be removed now, since NODE_OT_select_grouped replaces it. | |
| source/blender/editors/space_node/node_select.c | ||
| 34 | seems like this shouldn't be needed. | |
| 250 | its incorrect to use vgroup limits here, perhaps we should move BKE_deform_split_prefix to more generic names too. but a bit out of scope for this patch. Or - use this to get the prefix BLI_replace_extension(node->name, sizeof(node->name), ""), then add a function to BLI_path.h which gets the extension since afaics none exist. | |
| 255 | node->flag && SELECT - incorrect. | |
| 286 | This overwrites the selection, in object mode, theres an option not to do this (extend), think it would be good to have with node selection too so they're consistent. | |
Comment Actions
CHANGES:
- Removed decl of object_select_grouped_ function set from node_intern.h. They are all called from node_select_grouped_exec in the same file. I'm not sure now why I decided to do that, but as most of the exec functions in node_select.c are not declared because they are called from another function in the same file, it seemed appropriate.
- Changed ret-type of object_select_grouped_<subtype> functions to static bool. They now return a boolean value indicating whether or not they actually changed anything. This way ED_node_sort only gets called if something actually changed.
- Added an actual check (Shift + Ctrl + G) to allow the user to "extend" the current selection instead of resetting.
- Removed "DNA_object_types.h" and replicated the functionality of split_suffix/prefix into node_name_split_(prefix/suffix). They are declared in node_intern.h and defined in node_select.c. I couldn't put them in BLI_path etc as for some reason blender wouldn't compile with a wall of undefined references. Not sure if I'm doing something wrong with that.
- #define'd MAX_NODE_NAME 64 node_intern.h
- Added a secondary utility function in node_select.c to identify the characters that qualify as separators for the split functions.
Comment Actions
This is looking much closer to being ready... however before committing I would make some changes.
So if you can do this its good, but this does extend out into other API's a bit, some of my preview may be also personal preference a bit too.
Listed requests inline.
| source/blender/editors/space_node/node_select.c | ||
|---|---|---|
| 268 | For all these checks, I think it makes sense to first check !(node->flag & SELECT) first, even in separate block. eg: if ((node->flag & SELECT) == 0) {
if (this_check_can_be_slow(node)) {
nodeSetSelected(node, true);
}
}Goes for all select grouped functions here. | |
| 269 | *style*, for new code use lowercase true/false: http://wiki.blender.org/index.php/Dev:Doc/Code_Style#Value_Literals | |
| 315 | noted in previous review, no need for |= here. value is only ever assigned once. | |
| 974 | These could be made generic - ultimately - having prefix+suffix functions for every data type - gets a bit hard to maintain well and bugs/fixes in one - may not apply to another. I think it would be better to investigate having a generic function which partitions a string, something like: Then this can be reused everywhere... This would work a bit different to python's BLI_str_utf8_partition(const char *str, const size_t str_len, const unsigned int delimiters[], size_t *r_sep, size_t *r_tail) so you could pass... unsigned char delims[4] = {'.', '_', ' ', 0};
BLI_str_utf8_partition(str, sizeof(str), delims, &sep, &tail);See string_utf8.c for related functions. this is more work but longer term more useful IMHO. Or if this is too much hassles, the prefix/suffix part of the patch could be dropped for now. | |
Comment Actions
Changes:
All TRUE -> true
Changed the conditionals to check for selection first then do the comparison.
Removed |= from if then assignments.
**New Functions**
BLI_string_split & BLI_string_split_utf8
I think this is what you wanted. I had to add a check for forward/backward to switch getting
prefix or suffix(also it's an int b/c C89 thinks bool is a dirty word). Without it we would get back the
tail end which could have multiple delimiters in it since the function would only read up to the fist
delimiter before splitting. This could be fine as long as the user only delimits the name once,
however this way allows the user to create name sets like.
abc_123.001 matching prefix: abc matching suffix 001
abc_234.002 matching prefix: abc matching suffix: 002
bcd_345.001 matching prefix: bcd matching suffix: 001
bcd_456.002 matching prefix: bcd matching suffix: 002
cde_567.001 matching prefix: cde matching suffix: 001
cde_678.002 matching prefix: cde matching suffix: 002
Also I wasn't sure if you wanted to just split the string in half, replicate the functionality of prefix/suffix,
or split it arbitrarily, and decided to go with this as it worked well with the way I was already doing it.
Comment Actions
talked on IRC with @Steve (Cruentus_Nex)
Generally good
Prefer to have python style string functions str.partition, rpartition since they can return offsets only (without copying into a new string)
However the string api issue is really outside the scope of a quick hack. so to get the functionality approved that part could be removed from the patch.
Either way is fine with me.
Comment Actions
Changed the string splitter function:
BLI_string_split is non-destructive and puts the index of the first and last occurrence of any
of delims into pre and suf parameters.
As a result I also had to change the algorithm to identify matching prefixes and suffixes.
Comment Actions
Some comments, these are changes I would make when applying the patch... at this point we're nearing release thouh. so the patch may have to wait until after 2.70
| source/blender/blenlib/intern/string.c | ||
|---|---|---|
| 12 | There is no utf8 version of this function? | |
| 17 | ||
| source/blender/editors/space_node/node_select.c | ||
| 39 | use if (STREQ(a, b)) { | |
| 52 | This should be terminated, {'.', '-', '_', '\0'}; | |
| 58 | our convention is if () { -- http://wiki.blender.org/index.php/Dev:Doc/Code_Style#Operators_and_Statements | |
| 60 | use if (STREQ(a, b)) { | |
Comment Actions
Last(?) revision..
Added UTF-8 version of string splitter, and
cleaned up some of the code styling.
Comment Actions
Note that this won't make it for the up-coming 2.70 release.
but by all means get it ready for inclusion after release, when we will have time to do final review and commit it.
Comment Actions
Updated patch against current master, and reworked the BLI_str part.
_utf8 version was really buggy, only working since delimiters were one-char only utf8 values, else...
I also thought it was very unlikely we'd need to get both first and last sep at the same time,
only searching one of them allows us to avoid cycling over the whole string.
Also, I made both left and right versions into a single func (with a bool to choose which direction).
Campbell, I know you usually do not really like extra parameters in BLI funcs, but imho here it makes sense.
Maybe you'd like a triplet though (BLI_str_partition()/BLI_str_rpartition()/BLI_str_partition_ex())?
Anyway, I would commit this part of the patch separately, as suggested earlier here it should be used elsewhere
in Blender as well (like for vgroups)...
I also factorized node prefix/sufix search funcs into a single one, for same reason (99% of code is shared).
And did some minor style cleanup.
I have one question here, though: why do we remove NODE_OT_select_same_type_step? This one seems to have
a completely different goal to me (select next/previous node of same type, can be useful e.g. when cycling
between viewer nodes or so I guess).
Comment Actions
Updated from comments, also (as talked about on IRC) added simple gtest for BLI_str new stuff.
Comment Actions
Btw, asking question again: why do we remove NODE_OT_select_same_type_step?
This one seems to have a completely different goal to me (select next/previous node of same type, can be useful e.g. when cycling between viewer nodes or so I guess).
Comment Actions
Mont29,
It's been awhile since I worked on this but I think what may have happened is when I was when I was removing "NODE_OT_select_same_type" to replace it with "NODE_OT_select_grouped", NODE_OT_select_same_type_step" may have just ended up as collateral damage.
In short, I honestly don't recall having a particular reason for removing it.
Another possibility that I can think of would be to place its functionality in the "select_grouped" function, which was simply lost somewhere along the way, though you are right that they do seem to accomplish separate tasks. You can put it back in as it was or add it to select_grouped.
| source/blender/editors/space_node/node_intern.h | ||
|---|---|---|
| 110 | Removing these declarations. Just realized they are all called by NODE_OT_select_grouped anyway. Changed their parameter list and as a result ED_node_sort is now in node_select_grouped_exec where it needs to be. They are all now bool return and I use the return to check if I need to call ED_node_sort and WM_add_notifier from their caller(node_select_grouped exec), more in keeping with the way the object_select.c functions work. | |
| source/blender/editors/space_node/node_select.c | ||
| 268 | Agreed and done. | |
| 269 | done | |
| 315 | done | |
Comment Actions
Updated patch and restored 'same_type_step' op.
Not found of big ops doing too much things, we can't follow with labels and tips really, currently... :/
| source/blender/blenlib/intern/string_utf8.c | ||
|---|---|---|
| 717 | should be consistent here and use unsigned int | |
| 739 | This returns firstmatch, but should instead return the first/last character. So if you do this: "a.b.c-b" Splitting with delimiters -, .. will give different results depending on order of delimiters depending on from_right. | |
| tests/gtests/blenlib/BLI_string_test.cc | ||
|---|---|---|
| 34 | These tests should include some corner cases (try cases that might break) - all delimiter chars, empty string, empty delimiter list, easy to add 4-5 tests like this. | |
Comment Actions
Will make requested changes, but do not understand one comment here.
| source/blender/blenlib/intern/string_utf8.c | ||
|---|---|---|
| 739 | I don’t understand this comment… Order of delimiters have no influence here, since they are all tested in this loop, for each (unicode) char. if from_right is true, we check all (unicode) chars from right to left, else from left to right. And it always returns (length of the prefix, pointer to delimiter [i.e. first byte of delimiter], pointer to suffix [i.e. first byte of suffix]), as far as I can see? | |
Comment Actions
Updated from review, added some tests (which allowed me to find a few bugs in _utf8 version ;) ).
Comment Actions
Looks good, (minor suggesions)
| source/blender/editors/space_node/node_select.c | ||
|---|---|---|
| 274 | ND_ prefix is used for notifiers, maybe just call "NODE_SELECT_BY_COLOR" ? ...for eg | |
| tests/gtests/blenlib/BLI_string_test.cc | ||
| 37 | pathutils should be string, other tests too | |
| 38 | Think these could be split in 2, second function for rpartition | |
Comment Actions
Committed as rBe3c8cf0a9ea00e (string API) and rB52d7c357781237 (node select_grouped op).