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

Minor iconforge fixes #197

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

tigercat2000
Copy link

@tigercat2000 tigercat2000 commented Dec 29, 2024

Just fixes a width() call where there should be height()

@ZeWaka ZeWaka self-requested a review December 29, 2024 05:00
@ZeWaka
Copy link
Contributor

ZeWaka commented Dec 29, 2024

@itsmeow please advise

@itsmeow
Copy link
Contributor

itsmeow commented Dec 29, 2024

Just need to run a tracy profile on this and compare the performance and it's good with me

@tigercat2000
Copy link
Author

Added another fix for another bug I found: for y in 0..std::cmp::min(image.width(), other_image.width()) only works for square icons. Using height works for both square and non-square icons.

@itsmeow
Copy link
Contributor

itsmeow commented Dec 30, 2024

So turns out this is way horrible for performance and is likely related to having massive spritesheet file sizes and no page cache on the Linux runners? I'm not sure if merging this is a good idea as they work fine for spritesheets that aren't massive and this adds seconds of overhead to small spritesheets that don't have issues. This needs to check for a certain size (what that size is, I have no idea)

@tigercat2000 tigercat2000 changed the title Make iconforge call create_dir_all, flush, and sync_all to deal with file system caches Make iconforge call create_dir_all, minor fixes Dec 30, 2024
Copy link
Contributor

@ZeWaka ZeWaka left a comment

Choose a reason for hiding this comment

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

Just checking in, is all this code still needed?

@itsmeow
Copy link
Contributor

itsmeow commented Dec 31, 2024

For some use cases yes. It adds a little bit of overhead, but it's acceptable. The bug fix is necessary.

@tigercat2000 tigercat2000 changed the title Make iconforge call create_dir_all, minor fixes Minor iconforge fixes Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants