-
Notifications
You must be signed in to change notification settings - Fork 9
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
Improves DMI saving (smaller files, better formatting) #15
Conversation
We've been using the write_to wrapper function, which doesn't allow us to pass in a compression level. This means we've been using the default, which is for some reason the fastest/worst for file size. This commit instead "inlines" the write_to function, and uses the Best compression method (slowest/lowest file size) This strategy reduces a large file I tested (screen_cyborg.dmi) from 82kb to 21kb (byond's output if you're curious is about 26kb) I've also added saving to the test suite, to ensure the path gets walked and to allow for visual inspection if changes are made
We, like byond, arrange our saved DMIs in squares. However unlike byond we currently don't remove extra height that we don't use, which leads to odd looking files. This commit fixes that
Do you think you could compare the relative speeds for a large DMI? Just want to make sure we're not paying a 100x increase or something (and if so, we could try and move it up a level). Honestly it should be a unit test.... :whelm: |
Ran it on the biggest dmi I know of, wallening's hierophant walls Fast strategy took 0.61s and produced 3.74mb |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks all good now.
Pogged |
For the record: |
What I did
Actually compresses dmis when saving them
We've been using the write_to wrapper function, which doesn't allow us to pass in a compression level.
This means we've been using the default, which is for some reason the fastest/worst for file size.
This commit instead "inlines" the write_to function, and uses the
BestAverage compression method (very good compression, acceptable speed)This strategy reduces a large file I tested (screen_cyborg.dmi) from 82kb to 23kb (byond's output if you're curious is about 26kb)
I've also added saving to the test suite, to ensure the path gets walked and to allow for visual inspection if changes are made
Improves our image formatting somewhat
We, like byond, arrange our saved DMIs in squares. However unlike byond we currently don't remove extra height that we don't use, which leads to odd looking files. This commit fixes that
Of note, this isn't exactly the same as byond. Byond squares based off PIXEL count, whereas we square based off ICON count. Could look into fixing this if anyone cares but it's like eh yk?
What I didn't do
I haven't converted us over to a less cursed image saving format. We don't really need much of what the image crate provides here, but I don't trust myself to deal with the like, requirements for sending empty data that the recommended solution, lodepng, would require.
I can look into it if you'd like I'm just not totally confident in my ability to do it performantly without some oversight.