blender
/
blender
136
412
Fork 596
Code Issues 6.3k Pull Requests 769 Projects 19 Wiki Activity

Fix #120731: sculpt trim tool crash #120733

Merged
Hans Goudey merged 2 commits from PratikPB2123/blender:120731-trim-tool-crash into main 2024-04-19 20:27:12 +02:00
Conversation 1 Commits 2 Files Changed 1 +4 -3
1 changed files with 4 additions and 3 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

7
source/blender/editors/sculpt_paint/sculpt_trim.cc
View File

@ -598,9 +598,6 @@ static void gesture_end(bContext & /*C*/, gesture::GestureData &gesture_data)
static void init_operation(gesture::GestureData &gesture_data, wmOperator &op)
{
gesture_data.operation = reinterpret_cast<gesture::Operation *>(
MEM_cnew<TrimOperation>(__func__));
TrimOperation *trim_operation = (TrimOperation *)gesture_data.operation;
trim_operation->op.begin = gesture_begin;
@ -725,6 +722,8 @@ static int gesture_box_exec(bContext *C, wmOperator *op)
return OPERATOR_CANCELLED;
}
gesture_data->operation = reinterpret_cast<gesture::Operation *>(
MEM_cnew<TrimOperation>(__func__));
initialize_cursor_info(*C, *op, *gesture_data);
init_operation(*gesture_data, *op);
Sean Kim commented 2024-04-17 10:57:50 +02:00
Outdated
Review
Copy Link

The order of initialze_cursor_info and then init_operation is actually still needed. This fix would just introduce other errors on the following line:

if (!trim_operation->initial_hit) {
    trim_operation->orientation = OrientationType::View;
}

For now, this is probably best fixed by adding something like

gesture_data->operation = reinterpret_cast<gesture::Operation *>(MEM_cnew<TrimOperation>(__func__));

above initialize_cursor_info

The order of `initialze_cursor_info` and then `init_operation` is actually still needed. This fix would just introduce other errors on the following line: ``` if (!trim_operation->initial_hit) { trim_operation->orientation = OrientationType::View; } ``` For now, this is probably best fixed by adding something like `gesture_data->operation = reinterpret_cast<gesture::Operation *>(MEM_cnew<TrimOperation>(__func__));` above `initialize_cursor_info`
Pratik Borhade commented 2024-04-17 11:29:22 +02:00
Outdated
Review
Copy Link

Hi, this will cause the memory leak. I would instead call initialize_cursor_info inside init_operation after operation is initialized.

Hi, this will cause the memory leak. I would instead call `initialize_cursor_info` inside `init_operation` after operation is initialized.
Sean Kim commented 2024-04-17 11:37:58 +02:00
Outdated
Review
Copy Link

This allocation is already occurring in the existing code inside init_operation and is cleaned up when the unique_ptr goes out of scope by the GestureData destructor, but I agree that moving initialize_cursor_info into init_operation is also a fine way of solving this.

This allocation is already occurring in the existing code inside `init_operation` and is cleaned up when the `unique_ptr` goes out of scope by the `GestureData` destructor, but I agree that moving `initialize_cursor_info` into `init_operation` is also a fine way of solving this.
Pratik Borhade commented 2024-04-17 11:58:39 +02:00
Outdated
Review
Copy Link

This allocation is already occurring in the existing code inside init_operation and is cleaned up when the unique_ptr goes out of scope by the GestureData destructor

Pointer was created in _exec function, it'll be freed once we exit the _exec function. So the earlier allocated memory in initialize_cursor_info is not freed.

BTW, I ended up moving allocation outside of init_operation 👀

> This allocation is already occurring in the existing code inside init_operation and is cleaned up when the unique_ptr goes out of scope by the GestureData destructor Pointer was created in _exec function, it'll be freed once we exit the _exec function. So the earlier allocated memory in `initialize_cursor_info` is not freed. BTW, I ended up moving allocation outside of init_operation 👀
@ -754,6 +753,8 @@ static int gesture_lasso_exec(bContext *C, wmOperator *op)
return OPERATOR_CANCELLED;
}
gesture_data->operation = reinterpret_cast<gesture::Operation *>(
MEM_cnew<TrimOperation>(__func__));
initialize_cursor_info(*C, *op, *gesture_data);
init_operation(*gesture_data, *op);