Fix regression crash when removing cel in tool loop (fix #5717)#5720
Merged
Conversation
Collaborator
|
clang-tidy review says "All clean, LGTM! 👍" |
miguelfabiao125-bit
added a commit
to miguelfabiao125-bit/aseprite
that referenced
this pull request
Mar 27, 2026
Upon investigating the cell-addition logic for tilemap layers, I found that Manual Mode was creating new cels without properly attaching them. While PR aseprite#5720 resolved the immediate crash, the first interaction in Manual Mode was still incorrectly committing a tile. To fix this, I implemented an early exit in the commit function that clears the pending data and opts out of the commit process when in Manual Mode. Currently, this leaves "ghost pixels" on the canvas, which I suspect are uncleaned preview tiles. I am working on a fix for the ghosting, but I would appreciate feedback on whether this "early exit" approach is the correct architectural path for the original bug.
miguelfabiao125-bit
added a commit
to miguelfabiao125-bit/aseprite
that referenced
this pull request
Mar 27, 2026
Upon investigating the cell-addition logic for tilemap layers, I found that Manual Mode was creating new cels without properly attaching them. While PR aseprite#5720 resolved the immediate crash, the first interaction in Manual Mode was still incorrectly committing a tile. To fix this, I implemented an early exit in the commit function that clears the pending data and opts out of the commit process when in Manual Mode. Currently, this leaves "ghost pixels" on the canvas, which I suspect are uncleaned preview tiles. I am working on a fix for the ghosting, but I would appreciate feedback on whether this "early exit" approach is the correct architectural path for the original bug.
miguelfabiao125-bit
added a commit
to miguelfabiao125-bit/aseprite
that referenced
this pull request
Mar 27, 2026
Upon investigating the cell-addition logic for tilemap layers, I found that Manual Mode was creating new cels without properly attaching them. While PR aseprite#5720 resolved the immediate crash, the first interaction in Manual Mode was still incorrectly committing a tile. To fix this, I implemented an early exit in the commit function that clears the pending data and opts out of the commit process when in Manual Mode. Currently, this leaves "ghost pixels" on the canvas, which I suspect are uncleaned preview tiles. I am working on a fix for the ghosting, but I would appreciate feedback on whether this "early exit" approach is a good fix for the original bug.
miguelfabiao125-bit
added a commit
to miguelfabiao125-bit/aseprite
that referenced
this pull request
Mar 27, 2026
Upon investigating the cell-addition logic for tilemap layers, I found that Manual Mode was creating new cels without properly attaching them. While PR aseprite#5720 resolved the immediate crash, the first interaction in Manual Mode was still incorrectly committing a tile. To fix this, I implemented an early exit in the commit function that clears the pending data and opts out of the commit process when in Manual Mode. Currently, this leaves "ghost pixels" on the canvas, which I suspect are uncleaned preview tiles. I am working on a fix for the ghosting, but I would appreciate feedback on whether this "early exit" approach is the correct architectural path for the original bug.
miguelfabiao125-bit
added a commit
to miguelfabiao125-bit/aseprite
that referenced
this pull request
Mar 30, 2026
Upon investigating the cell-addition logic for tilemap layers, I found that Manual Mode was creating new cels without properly attaching them. While PR aseprite#5720 resolved the immediate crash, the first interaction in Manual Mode was still incorrectly committing a tile. To resolve this, I implemented an early exit within the commit function to bypass the data-writing process when in Manual Mode. However, this initially prevented the preview layer from being cleared. To ensure a consistent UI state, I updated drawing_state to explicitly clean the preview buffer when the drawing loop is destroyed. This ensures that "ghost" pixels are removed even when a commit is opted out.
miguelfabiao125-bit
added a commit
to miguelfabiao125-bit/aseprite
that referenced
this pull request
Mar 30, 2026
Upon investigating the cell-addition logic for tilemap layers, I found that Manual Mode was creating new cels without properly attaching them. While PR aseprite#5720 resolved the immediate crash, the first interaction in Manual Mode was still incorrectly committing a tile. To resolve this, I implemented an early exit within the commit function to bypass the data-writing process when in Manual Mode. However, this initially prevented the preview layer from being cleared. To ensure a consistent UI state, I updated drawing_state to explicitly clean the preview buffer when the drawing loop is destroyed. This ensures that "ghost" pixels are removed even when a commit is opted out. I have also added a test to make sure no other crash or tile creation is added by the manual tilemode in the future.
miguelfabiao125-bit
added a commit
to miguelfabiao125-bit/aseprite
that referenced
this pull request
Apr 4, 2026
Upon investigating the cell-addition logic for tilemap layers, I found that Manual Mode was creating new cels without properly attaching them. While PR aseprite#5720 resolved the immediate crash, the first interaction in Manual Mode was still incorrectly committing a tile. To resolve this, I implemented an early exit within the commit function to bypass the data-writing process when in Manual Mode. However, this initially prevented the preview layer from being cleared. To ensure a consistent UI state, I updated drawing_state to explicitly clean the preview buffer when the drawing loop is destroyed. This ensures that "ghost" pixels are removed even when a commit is opted out. I have also added a test to make sure no other crash or tile creation is added by the manual tilemode in the future.
miguelfabiao125-bit
added a commit
to miguelfabiao125-bit/aseprite
that referenced
this pull request
Apr 4, 2026
Upon investigating the cell-addition logic for tilemap layers, I found that Manual Mode was creating new cels without properly attaching them. While PR aseprite#5720 resolved the immediate crash, the first interaction in Manual Mode was still incorrectly committing a tile. To resolve this, I implemented an early exit within the commit function to bypass the data-writing process when in Manual Mode. However, this initially prevented the preview layer from being cleared. To ensure a consistent UI state, I updated drawing_state to explicitly clean the preview buffer when the drawing loop is destroyed. This ensures that "ghost" pixels are removed even when a commit is opted out. I have also added a test to make sure no other crash or tile creation is added by the manual tilemode in the future.
Member
|
I'll merge this one as it fixes the crash, and then continue with #5732 as I think the Manual mode shouldn't create new tiles. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Regression from 05f2a1e, because of the reworked logic we were calling
getDestTileset()even when we created a new cel, so the new cel was never attached to a layer and we crashed when we removed it later.This logic could probably be simplified a bunch with #5684. Related: #5678