Transform: support properly transforming implicitly shared curve's positions #120824

Laurynas Duburas · 2024-04-19T13:29:20+02:00

Laurynas Duburas commented

2024-04-19 13:29:20 +02:00

The alternative implementation of #120631

Laurynas Duburas added 1 commit 2024-04-19 13:29:29 +02:00

copy data for transform to tc->custom.type.data 4116320481

Laurynas Duburas requested review from Germano Cavalcante 2024-04-19 13:30:15 +02:00

Laurynas Duburas requested review from Jacques Lucke 2024-04-19 13:30:15 +02:00

Laurynas Duburas requested review from Hans Goudey 2024-04-19 13:30:15 +02:00

Hans Goudey requested changes 2024-04-19 14:33:52 +02:00

source/blender/editors/transform/transform_convert_curves.cc Outdated

						
				@ -65,0 +88,4 @@

				  const int64_t points_num = selection.size();

				  const Vector<IndexRange> ranges{selection.to_ranges()};

				  void *data = MEM_callocN(sizeof(int64_t) * 2 + sizeof(float3) * points_num +

Hans Goudey commented

2024-04-19 14:31:33 +02:00

Could you make the customdata point to a struct like TransformArrays that contains a Array for each dhunk it needs to hold?

Could you make the customdata point to a struct like `TransformArrays` that contains a `Array` for each dhunk it needs to hold?

Laurynas Duburas commented

2024-04-19 14:47:47 +02:00

I was trying to avoid new struct, it will change a lot.

laurynas marked this conversation as resolved

source/blender/editors/transform/transform_convert_curves.cc Outdated

						
				@ -65,0 +101,4 @@

				  Span<float3> positions = curves.positions();

				  int64_t offset_dst = 0;

				  for (const IndexRange &range : ranges) {

Hans Goudey commented

2024-04-19 14:32:50 +02:00

Might it be simpler to use array_utils::gather instead of using an intermediate step of building index ranges?

Might it be simpler to use `array_utils::gather` instead of using an intermediate step of building index ranges?

Laurynas Duburas commented

2024-04-19 21:17:31 +02:00

If to go with array_utils::gather selection as IndexMask needs to be stored in tc.custom.type.data for latter use by array_utils::scatter. I didn't find how to make deep copy of IndexMask and that forces in function createTransCurvesVerts to use IndexMaskMemory instance from tc.custom.type.data instead of local variable.
And it is doable, but curve_populate_trans_data_structs is also called from grease pencil code. Selections there also has to be attached to tc.custom.type.data and it's data type (currentlyTransformArrays) exposed outside from transform_convert_curves.cc. Here things get messier.

Also with ranges I like fact that copying is done in chunks, though this advantage is selection dependent.

Can you check current version? It would transfer to grease pencil a lot easier.

If to go with `array_utils::gather` selection as `IndexMask` needs to be stored in `tc.custom.type.data` for latter use by `array_utils::scatter`. I didn't find how to make deep copy of `IndexMask` and that forces in function `createTransCurvesVerts` to use `IndexMaskMemory` instance from `tc.custom.type.data` instead of local variable. And it is doable, but `curve_populate_trans_data_structs` is also called from grease pencil code. Selections there also has to be attached to `tc.custom.type.data` and it's data type (currently`TransformArrays`) exposed outside from `transform_convert_curves.cc`. Here things get messier. Also with ranges I like fact that copying is done in chunks, though this advantage is selection dependent. Can you check current version? It would transfer to grease pencil a lot easier.

Iliya Katushenock commented

2024-04-19 21:27:38 +02:00

You need to do that:

Array<int> mapping(mask.size());
mask.to_indices(mapping);
array_utils::gather(src_positions, mapping.as_span(), dst_positions);

And after this:

for (const int i : mapping.index_range()) {
  dst_positions[mapping[i]] = src_positions[i];
}

No need for to_ranges and Array<int> offsets.

You need to do that: ```Cpp Array<int> mapping(mask.size()); mask.to_indices(mapping); array_utils::gather(src_positions, mapping.as_span(), dst_positions); ``` And after this: ```Cpp for (const int i : mapping.index_range()) { dst_positions[mapping[i]] = src_positions[i]; } ``` No need for `to_ranges` and `Array<int> offsets`.

Hans Goudey commented

2024-04-19 21:29:13 +02:00

Storing the selection for later would really just be an optimization anyway, it can always be recomputed from the current attributes. Anyway, I'm not convinced that storing Array<IndexRange> is faster than IndexMask general. Especially since there's a lot of single threaded work going on computing the offsets, etc, it doesn't seem worth the complexity.

I would suggest moving IndexMaskMemory and IndexMask into TransformArrays. Since TransformArrays is becoming more general that way, maybe it should be renamed CurvesTransformData or something.

You're right about that being more different for grease pencil, but to me it seems better to find the optimal solution separately for each case. GP is doing a different thing by combining the data from multiple CurvesGeometry into one container.

Storing the selection for later would really just be an optimization anyway, it can always be recomputed from the current attributes. Anyway, I'm not convinced that storing `Array<IndexRange>` is faster than `IndexMask` general. Especially since there's a lot of single threaded work going on computing the offsets, etc, it doesn't seem worth the complexity. I would suggest moving `IndexMaskMemory` and `IndexMask` into `TransformArrays`. Since `TransformArrays` is becoming more general that way, maybe it should be renamed `CurvesTransformData` or something. You're right about that being more different for grease pencil, but to me it seems better to find the optimal solution separately for each case. GP is doing a different thing by combining the data from multiple `CurvesGeometry` into one container.

Laurynas Duburas commented

2024-04-20 00:29:52 +02:00

I don't see to separate GP without two separate curve_populate_trans_data_structs functions.
Current version works for both, also I think it'll be handy for Bezier handles also.

I don't see to separate GP without two separate `curve_populate_trans_data_structs` functions. Current version works for both, also I think it'll be handy for Bezier handles also.

Laurynas Duburas commented

2024-04-20 00:47:45 +02:00

No need for to_ranges and Array<int> offsets.

Thanks for suggestion.
I wanted ranges to copy in chunks. I was frightened by fact that many curves with all vertices selected will be copied point by point.
Anyway I think I solved it with IndexMasks.

> No need for `to_ranges` and `Array<int> offsets`. Thanks for suggestion. I wanted ranges to copy in chunks. I was frightened by fact that many curves with all vertices selected will be copied point by point. Anyway I think I solved it with `IndexMask`s.

👍 1

laurynas marked this conversation as resolved

source/blender/editors/transform/transform_convert_curves.cc Outdated

						
				@ -170,3 +236,2 @@

				  pseudoinverse_m3_m3(smtx, mtx, PSEUDOINVERSE_EPSILON);

				  MutableSpan<float3> positions = curves.positions_for_write();

				  int64_t position_num = *blender::ed::transform::curves::custom_data_to_point_num(

Hans Goudey commented

2024-04-19 14:33:45 +02:00

I think you can remove the blender:: from here

I think you can remove the `blender::` from here

laurynas marked this conversation as resolved

Laurynas Duburas changed title from ~~Transform: support properly transforming implicitly shared curve's positions~~ to WIP: Transform: support properly transforming implicitly shared curve's positions

2024-04-19 14:47:51 +02:00

Laurynas Duburas added 1 commit 2024-04-19 18:03:17 +02:00

struct TransformArrays added dbf00e3579

Laurynas Duburas added 1 commit 2024-04-20 00:23:50 +02:00

gather and scatter 37186dbd70

Laurynas Duburas changed title from ~~WIP: Transform: support properly transforming implicitly shared curve's positions~~ to Transform: support properly transforming implicitly shared curve's positions

2024-04-20 00:48:16 +02:00

Laurynas Duburas added 1 commit 2024-04-20 09:07:12 +02:00

cleanup 20f94e5865

Jacques Lucke requested changes 2024-04-22 11:46:06 +02:00

Dismissed

source/blender/editors/transform/transform_convert.hh Outdated

						
				@ -83,0 +87,4 @@

				struct CurvesTransformData {

				  blender::IndexMaskMemory memory;

				  blender::Vector<blender::IndexMask> selection_by_layer;

				  blender::Vector<int> layer_offsets;

Jacques Lucke commented

2024-04-22 11:46:00 +02:00

Add comments for what layer_offsets and positions contain.

Add comments for what `layer_offsets` and `positions` contain.

laurynas marked this conversation as resolved

source/blender/editors/transform/transform_convert.hh Outdated

						
				@ -161,6 +172,12 @@ void curve_populate_trans_data_structs(TransDataContainer &tc,

				                                       bool use_connected_only,

				                                       int trans_data_offset);

				CurvesTransformData *create_curves_custom_data(TransCustomData &custom_data);

Jacques Lucke commented

2024-04-22 11:44:00 +02:00

These functions are in the global namespace, so they should have a bit more specific names.

laurynas marked this conversation as resolved

source/blender/editors/transform/transform_convert_curves.cc Outdated

						
				@ -65,0 +73,4 @@

				  transform_data.layer_offsets.append(data_offset + selection.size());

				  array_utils::gather(

				      positions,

				      transform_data.selection_by_layer.last(),

Jacques Lucke commented

2024-04-22 10:12:13 +02:00

selection

`selection`

laurynas marked this conversation as resolved

Laurynas Duburas added 1 commit 2024-04-22 14:26:28 +02:00

Comments added f2abc80fba

Hans Goudey approved these changes 2024-04-22 15:09:00 +02:00

source/blender/editors/transform/transform_convert_curves.cc Outdated

						
				@ -82,2 +101,3 @@

				    else {

				      selection_per_object[i] = ed::curves::retrieve_selected_points(curves, memory);

				      selection_per_object[i] = ed::curves::retrieve_selected_points(

				          curves, curves_transform_data->memory);