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

Fix vector layer color import #1798

Merged
merged 3 commits into from
Nov 25, 2023

Conversation

J5lx
Copy link
Member

@J5lx J5lx commented Nov 24, 2023

This fixes the crash reported over here on the forum. The crash was caused by the dangling pointer to the imported file’s Object dereferenced in the vector paint code in order to resolve colour indices.

This PR completely removes Object pointers from the vector classes and instead passes the object as a parameter at paint time. (Tbh I’m also thinking about splitting out the painting code altogether, but I didn’t want to let myself get derailed.) It also removes Object pointers from layer classes, which only used it in the constructor and setObject to generate layer ids, and instead passes those ids directly.

Lastly, a proper colour import mechanism for vector layer import is implemented as well.

Copy link
Member

@MrStevns MrStevns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work figuring out that crash and how to fix it!
I also like the removal of Object in Layer, as I find that LayerManager fulfill that responsibility.

I've looked through the changes and I think they all look good. Feel free to merge when you're satisfied with the PR.

@J5lx
Copy link
Member Author

J5lx commented Nov 25, 2023

Thanks for reviewing!

@J5lx J5lx merged commit 18c5494 into pencil2d:master Nov 25, 2023
8 checks passed
@J5lx J5lx deleted the fixes/vector-layer-import-crash branch November 25, 2023 12:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants