Fix #120058: Crash on undo rename in edit mode #120099

Closed

Jake wants to merge 1 commits from Jake-Faulkner/blender:Fix-120058 into main

When changing the target branch, be careful to rebase the branch in your fork to match. See documentation.

Jake commented

2024-03-31 00:20:51 +01:00

Contributor

Drop the check for the return of BKE_id_new_name_validate, as it doesn't indicate if the name changed. More correct would be to replace it with a check for if the new name is different than the old, but I don't see any neat way to do that from down here.

Other check might also be replaced with an assert, but I'm less sure about that?

Drop the check for the return of `BKE_id_new_name_validate`, as it doesn't indicate if the name changed. More correct would be to replace it with a check for if the new name is different than the old, but I don't see any neat way to do that from down here. Other check might also be replaced with an assert, but I'm less sure about that?

Jake added 1 commit 2024-03-31 00:21:01 +01:00

1af6a4e0cd Fix #120058 : Crash on undo rename in edit mode

Drop the check for the return of `BKE_id_new_name_validate`, as it doesn't indicate if the name changed. More correct would be to replace it with a check for if the new name is different than the old, but I don't see any neat way to do that from down here.

Other check might also be replaced with an assert, but I'm less sure about that?

Iliya Katushenock added this to the Core project 2024-03-31 12:36:37 +02:00

Iliya Katushenock added the

label 2024-03-31 12:36:54 +02:00

Philipp Oeser commented

2024-04-02 13:14:27 +02:00

Member

Just noting that the regression was caused by 195bb4f8f5

Just noting that the regression was caused by 195bb4f8f5

Bastien Montagne commented

2024-04-02 15:49:05 +02:00

Owner

@mod_moder can you explain me what is linked to overrides in this PR?

@mod_moder can you explain me _what_ is linked to overrides in this PR?

Iliya Katushenock commented

2024-04-02 16:04:28 +02:00

Member

I am not really familiar with this code so that is simple: i see ID_IS_LINKED and os i link overrides here.

I am not really familiar with this code so that is simple: i see `ID_IS_LINKED` and os i link *overrides* here.

Bastien Montagne requested changes 2024-04-02 16:10:35 +02:00

Bastien Montagne left a comment

Owner

@Jake-Faulkner Thanks for the PR! You have correctly identified the cause of the issue, but the proposed solution here is not correct.

This function is only here to ensure that the name of the ID is unique, it can be called from other codepaths than renaming an existing ID.

I think the proper solution here is to finally have a new BKE_libblock_rename(Main &bmain, ID &id, StringRefNull old_name, StringRefNull new_name) function, which would ensure all required actions on ID rename are performed (afaik, at least two different code handle ID renaming: RNA, and the Outliner).

Check that old and new names are not identical, otherwise early-return.
Call BKE_main_namemap_remove_name with the old name.
Actually copy the new name to the ID's name.
Call BKE_libblock_ensure_unique_name.
Set bmain.is_memfile_undo_written to false.

@Jake-Faulkner Thanks for the PR! You have correctly identified the cause of the issue, but the proposed solution here is not correct. This function is only here to ensure that the name of the ID is unique, it can be called from other codepaths than renaming an existing ID. I think the proper solution here is to finally have a new `BKE_libblock_rename(Main &bmain, ID &id, StringRefNull old_name, StringRefNull new_name)` function, which would ensure all required actions on ID rename are performed (afaik, at least two different code handle ID renaming: RNA, and the Outliner). * Check that old and new names are not identical, otherwise early-return. * Call `BKE_main_namemap_remove_name` with the old name. * Actually copy the new name to the ID's name. * Call `BKE_libblock_ensure_unique_name`. * Set `bmain.is_memfile_undo_written` to false.

Bastien Montagne added

and removed

labels 2024-04-02 16:11:23 +02:00

Bastien Montagne referenced this pull request

2024-04-03 08:54:35 +02:00

Fix #120058: Undoing a rename while in edit mode crashes Blender. #120196

Philipp Oeser commented

2024-04-03 08:54:39 +02:00

Member

I am not really familiar with this code so that is simple: i see ID_IS_LINKED and os i link overrides here.

When in doubt (and given the fact that ID_IS_LINKED is not relevant for the changes here), it is better to not do anything here then

> I am not really familiar with this code so that is simple: i see `ID_IS_LINKED` and os i link *overrides* here. When in doubt (and given the fact that `ID_IS_LINKED` is not relevant for the changes here), it is better to not do anything here then

Bastien Montagne commented

2024-04-03 08:57:21 +02:00

Owner

@Jake-Faulkner Thanks again for identifying to source issue of this crash and for the PR,

Given the severity of the bug (and therefore the urgency to fix it, and get it to sink a bit in main before backporting to 4.1.1), I went ahead and created an alternate fix (!120196).

Turns out we already had a BKE_libblock_rename that did everything correctly, we 'just' needed to use it everywhere!

@Jake-Faulkner Thanks again for identifying to source issue of this crash and for the PR, Given the severity of the bug (and therefore the urgency to fix it, and get it to sink a bit in main before backporting to 4.1.1), I went ahead and created an alternate fix (!120196). Turns out we already had a `BKE_libblock_rename` that did everything correctly, we 'just' needed to use it everywhere!

Bastien Montagne closed this pull request

2024-04-03 08:57:24 +02:00

Bastien Montagne referenced this issue from a commit

2024-04-05 03:19:49 +02:00

Fix #120058: Undoing a rename while in edit mode crashes Blender.

Bastien Montagne referenced this issue from a commit

2024-04-11 18:56:21 +02:00

Fix #120058: Undoing a rename while in edit mode crashes Blender.

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Animation & Rigging

Asset Browser Project Overview

Automated Testing

Blender Asset Bundle

Dependency Graph

Development Management

EEVEE & Viewport

Images & Movies

Motion Tracking

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Render Pipeline

Sculpt, Paint & Texture

Video Sequencer

Virtual Reality

Blender 2.8 Project

Milestone 1: Basic, Local Asset Browser

Good First Issue

Animation & Rigging

Development Management

EEVEE & Viewport

Nodes & Physics

Pipeline, Assets & IO

Platforms, Builds & Tests

Render & Cycles

Sculpt, Paint & Texture

Needs Info from Developers

Needs Information from User

No Milestone

No project

No Assignees

4 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: blender/blender#120099

No description provided.