Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Prevent crash and error spam related to Sprite2D with a region #84361

Merged

Conversation

YuriSizov
Copy link
Contributor

@YuriSizov YuriSizov commented Nov 2, 2023

Fixes #84349 and a couple other problems.

  1. The reported issue is related to deleting the node that was previously edited by the region editor, and then undoing this. Normally, UndoRedo actions should be attached to the edited objects themselves, but in this case the action is a part of the region editor, which no longer has a valid reference to work with. I made sure we don't handle invalid values, which leads to crashes, but there may be a further improvement possible in there. We could make sure that the historical reference to the edited object is preserved and used by the editor method. But for now fixing the crash should be good enough, and there should be no visible issues to the user.
  2. Related to UndoRedo there was also an error message due to poor handling on the signal handlers. I adjusted the code to correctly disconnect them when the node disappears.
  3. While testing the linked issue I noticed that regions don't work with the Sprite 2D editor, namely when converting sprites to 2D meshes. If the region is smaller than the texture, the crop is misaligned. If the region is bigger than the texture, there is error spam related to trying to access a bad pixel coordinate. I could probably fix it properly, but this close to the release I don't want to cause any further regressions. So I simply removed the region handling code, and it is for now simply ignored. It was introduced like that in the original PR in 9e3a1e5, so it probably never worked.

While working on the third point I also cleaned up error handling logic, so error popups should now appear correctly, won't be obscured by the useless Sprite 2D editor, and excessive checks won't run on each update (since they don't validate anything that the user can change from the dialog anyway).

@YuriSizov YuriSizov added this to the 4.2 milestone Nov 2, 2023
@YuriSizov YuriSizov requested a review from a team as a code owner November 2, 2023 12:51
@YuriSizov YuriSizov force-pushed the editor-fix-sprite2d-regional-issues branch from e2b9711 to 8b45fca Compare November 2, 2023 13:28
@aXu-AP
Copy link
Contributor

aXu-AP commented Nov 3, 2023

I was also planning to fix the point no. 3. Do you want to add support for regions in converter afterwards or shall I try?

@YuriSizov
Copy link
Contributor Author

@aXu-AP Feel free!

@YuriSizov YuriSizov force-pushed the editor-fix-sprite2d-regional-issues branch from 8b45fca to 111a5e9 Compare November 4, 2023 12:04
@YuriSizov YuriSizov merged commit fae8ace into godotengine:master Nov 6, 2023
15 checks passed
@YuriSizov
Copy link
Contributor Author

Thanks for reviews!

@YuriSizov YuriSizov deleted the editor-fix-sprite2d-regional-issues branch November 6, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Crash when deleting Sprite2D with region, undoing
4 participants