Transform: support properly transforming implicitly shared positions #120631

Jacques Lucke · 2024-04-14T12:07:00+02:00

Jacques Lucke commented

2024-04-14 12:07:00 +02:00

There seems to be a fairly fundamental problem with transforming points in curves edit mode right now. The transform code keeps non-const pointers into potentially shared (and therefore immutable) position arrays. This is not possible allowed with implicitly-shared data. Instead, the transform code should work on a local copy of the data that it actually owns and then copy the changes back into the geometry.

I don't know the transform code all that well, so it's possible that this has been fixed in a different way elsewhere already. Some feedback would be welcome.

There seems to be a fairly fundamental problem with transforming points in curves edit mode right now. The transform code keeps non-const pointers into potentially shared (and therefore immutable) position arrays. This is not possible allowed with implicitly-shared data. Instead, the transform code should work on a local copy of the data that it actually owns and then copy the changes back into the geometry. I don't know the transform code all that well, so it's possible that this has been fixed in a different way elsewhere already. Some feedback would be welcome.

Jacques Lucke added 1 commit 2024-04-14 12:07:10 +02:00

5864a71aa5 copy data for transform

Jacques Lucke referenced this pull request

2024-04-14 12:09:05 +02:00

Curves: Transform Bezier handles #120222

Jacques Lucke requested review from Germano Cavalcante 2024-04-14 12:09:51 +02:00

Jacques Lucke requested review from Hans Goudey 2024-04-14 12:09:52 +02:00

Germano Cavalcante commented

2024-04-15 14:06:56 +02:00

I'm not a big fan of the idea of increasing the size of TransData (with the float loc_copy[3] member). It would be better if we could shorten this struct. To give you an idea, when we transform a mesh with Proportional Editing enabled, the number of TransData is equal to the number of vertices which causes memory usage to be greater than that of the mesh itself.

If we really need a copy with the curve locations. We could allocate an array and reference it in tc->custom.type.data. This member is customizable for each type of "transform_convert". It can be created and set within the transform_convert_curves.cc file.

(More information that might be useful:)

To update positions during transformation. Just edit the recalcData_curves function.
If it is useful to call a function at the end of the transformation, simply create a callback and set it to the special_aftertrans_update member of TransConvertTypeInfo.

I'm not a big fan of the idea of increasing the size of `TransData` (with the `float loc_copy[3]` member). It would be better if we could shorten this struct. To give you an idea, when we transform a mesh with Proportional Editing enabled, the number of `TransData` is equal to the number of vertices which causes memory usage to be greater than that of the mesh itself. If we really need a copy with the curve locations. We could allocate an array and reference it in `tc->custom.type.data`. This member is customizable for each type of "transform_convert". It can be created and set within the `transform_convert_curves.cc` file. --- (More information that might be useful:) - To update positions during transformation. Just edit the `recalcData_curves` function. - If it is useful to call a function at the end of the transformation, simply create a callback and set it to the `special_aftertrans_update` member of `TransConvertTypeInfo`.

Hans Goudey commented

2024-04-15 16:00:36 +02:00

I agree that keeping pointers to mutable shared data is problematic. I've removed them from a fair amount of sculpt code for the same reasons.

I think a better long term solution would be to completely replace the TransData::iloc pointer. The only thing the transform system should need to store per element is an index (or a BMVert pointer for BMesh). Instead the transform system should retrieve the data on-demand and write it back to the geometry as a separate step.

I agree that keeping pointers to mutable shared data is problematic. I've removed them from a fair amount of sculpt code for the same reasons. I think a better long term solution would be to completely replace the `TransData::iloc` pointer. The only thing the transform system should need to store per element is an index (or a `BMVert` pointer for BMesh). Instead the transform system should retrieve the data on-demand and write it back to the geometry as a separate step.

Laurynas Duburas commented

2024-04-16 23:23:47 +02:00

I like array in tc->custom.type.data solution more too.
Maybe I could implement it in #120222, because in any case I'll have to add Bezier handles to the solution?

I like array in `tc->custom.type.data` solution more too. Maybe I could implement it in #120222, because in any case I'll have to add Bezier handles to the solution?

Jacques Lucke commented

2024-04-17 18:45:43 +02:00

@laurynas it would be very helpful if you could look into fixing this issue properly. I'd recommend doing it separate from your existing patch though, as the problem exists in main already.

@laurynas it would be very helpful if you could look into fixing this issue properly. I'd recommend doing it separate from your existing patch though, as the problem exists in `main` already.

Laurynas Duburas referenced this pull request

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

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

Hans Goudey commented

2024-04-22 17:14:25 +02:00

#120824

Hans Goudey closed this pull request

2024-04-22 17:14:27 +02:00