GPv3: Select similar #111410
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
2 Participants
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: blender/blender#111410
Loading…
Reference in New Issue
No description provided.
Delete Branch "Lorenzo-Carpaneto/blender:gpv3-select-similar"
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?
Still WIP, but let's discuss problems and possible solutions. Even if we have discussed already this problem, I think better to reassess here.
For the templated function, is it good to leave it in the header file or better to use other strategy?
The set should be able to handle concurrency for better performances.
LAYER and MATERIAL could be moved into a select linked operator. We were talking about submenu but I am not completely clear of the idea.
Let me know your thoughts :)
WIP: gpv3-select-similarto WIP: GPv3: Select similarSince this is still WIP, I won't comment on specific code things. I have some general thoughts instead:
I think the
case LAYER:
inselect_similar_exec
shouldn't call the same functions as all the other option. Layers are not attributes inCurvesGeometry
. All the other options select something based on an attribure. So making theLAYER
option call a different function would remove complexity inselect_similar_main
and also remove the necessity forforeach_editable_drawing_in_layer_ex
.WIP: GPv3: Select similarto GPv3: Select similarThere are some issues with this PR, I added some remarks, hopefully it's all relatively clear.
Also some general comments:
snake_case
, useconst
when possible, etc.).@ -28,2 +28,4 @@
namespace blender::ed::greasepencil {
/* WARNING: don't change existing ones without modifying rearrange func accordingly */
typedef enum eSelectSimilar {
This should be an
enum class SelectSimilarType
.@ -286,1 +304,4 @@
static int select_similar_exec(bContext *C, wmOperator *op)
{
const int type = RNA_enum_get(op->ptr, "type");
With the change above:
const SelectSimilarType type = SelectSimilarType(RNA_enum_get(op->ptr, "type"));
@ -287,0 +313,4 @@
switch (type) {
case LAYER:
ed::curves::select_similar_layer(
These functions shouldn't be defined in
ED_curves.hh
(especiallyselect_similar_layer
) since the functions are specific to grease pencil. If in the future, some of the functionality is used by theCurves
object, we can still move some of them there.I would suggest to keep it simple and define all of them as static functions above this function (
select_similar_exec
). That also means we don't have to have code in the header file.@ -287,0 +333,4 @@
grease_pencil, scene, selection_domain, type, threshold, "opacity");
break;
default:
throw std::invalid_argument("Undefined behavior for eSelectSimilar_Mode " + type);
We don't use exceptions in Blender. There wouldn't be anything catching this. Either we handle the error directly, or we just assert. In this case no default case is necessary, because you're handling all the possible . If in the future we add a new case, then the compiler will complain about a missing case.
@ -287,0 +348,4 @@
{
ot->name = "Select Similar";
ot->idname = "GREASE_PENCIL_OT_select_similar";
ot->description = "Select all strokes with similar characteristics";
This should probably say
points
instead ofstrokes
since we're only allowing this operator to run in point selection mode.@ -213,0 +227,4 @@
else if constexpr (std::is_same<T, std::string>::value) {
return "default";
}
throw std::invalid_argument("Undefined behavior for distance function for the used type");
return T(0);
@ -213,0 +278,4 @@
return currentlySelectedValues;
}
template<typename T> static float distance(T first, T second)
This should only handle the case for
ColorGeometry4f
and otherwise always usemath::distance
.@ -213,0 +300,4 @@
VArray<T> attributes = *curves.attributes().lookup_or_default<T>(
attribute_id, ATTR_DOMAIN_POINT, default_for_lookup<T>());
const OffsetIndices points_by_curve = curves.points_by_curve();
bke::GSpanAttributeWriter selection = ensure_selection_attribute(
Use
const VArray<bool> point_selection = *curves.attributes().lookup_or_default<bool>(".selection", ATTR_DOMAIN_POINT, true);
instead.I don't get how would this work... I'll publish my changes in a bit, so we can return on this point :)
@ -213,0 +305,4 @@
MutableSpan<bool> selection_typed = selection.span.typed<bool>();
// for now sequential impl, grain_size == 1
threading::parallel_for(curves.curves_range(), 1, [&](const IndexRange range) {
This should be a loop over each editable point using
editable_points.foreach_index([&](const int64_t point_i) { ... });
@ -213,0 +313,4 @@
continue;
}
for (const int index : points.index_range()) {
This should use
IndexMask::from_predicate
to build a mask using the distance condition and thenindex_mask::masked_fill
to fill the selection attribute withtrue
for the mask.@ -213,0 +326,4 @@
selection.finish();
}
static void select_similar_layer(GreasePencil &grease_pencil,
This function seems overly complicated. There is no need to store the layers in a set and loop twice.
Here is a simpler version (needs a paramter
Object &object
to account for locked materials).I have changed the code as suggested but to me it's not clear how this implementation works. Could you provide a more in detail explanation?
@ -213,0 +371,4 @@
std::string attribute_id)
{
using namespace blender::ed::greasepencil;
blender::Vector<blender::Set<T>> currentlySelectedValuesPerDrawing;
This should be a
blender::Array
since we can retrieve the editable drawings before and initialize the array withdrawings.size()
.@ -213,0 +383,4 @@
Set<T> currentlySelectedValues = join_sets<T>(currentlySelectedValuesPerDrawing);
threading::parallel_for_each(drawings, [&](const MutableDrawingInfo &info) {
blender::ed::curves::select_with_similar_attribute<T>(info.drawing.strokes_for_write(),
This should also receive an index mask for the points that are editable. See
ed::greaspencil::retrieve_editable_points
.@ -549,6 +549,11 @@ typedef struct GreasePencil {
const blender::Map<int, int> &frame_number_destinations,
const blender::Map<int, GreasePencilFrame> &duplicate_frames);
void foreach_editable_drawing_in_layer_ex(
This function is not needed. Use
ed::greasepencil::retrieve_editable_drawings
.@Lorenzo-Carpaneto Are you planning on addressing the review? Otherwise we'll just take over at some point.
Hello @filedescriptor, I wanted to address the review and I tried contacting you in blender chat for further questions but I did not receive answers. I pinged you on those chats, so if you d like, I can move forward with this pr
Checkout
From your project repository, check out a new branch and test the changes.