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

Improves DMI saving (smaller files, better formatting) #15

Merged
merged 4 commits into from
Nov 25, 2023

Conversation

LemonInTheDark
Copy link
Contributor

@LemonInTheDark LemonInTheDark commented Nov 24, 2023

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 Best Average 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.

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
@ZeWaka
Copy link
Member

ZeWaka commented Nov 24, 2023

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:

@LemonInTheDark
Copy link
Contributor Author

LemonInTheDark commented Nov 24, 2023

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
All numbers come from the load_and_save test in release mode on my laptop

Fast strategy took 0.61s and produced 3.74mb
Best strategy took 1.31s and produced 992kb
Default strategy took 0.88s and produced 999kb
(There are other, deprecated strategies, but I'm just gonna leave em alone)

Copy link
Member

@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.

Looks all good now.

@LemonInTheDark
Copy link
Contributor Author

Pogged

@ZeWaka ZeWaka merged commit 3374dbe into spacestation13:master Nov 25, 2023
3 checks passed
@LemonInTheDark
Copy link
Contributor Author

For the record:
It seems like default results in file sizes lower then standard dreammaker for most dmis. It is still way better then the fast option tho, it goes from like 500kb -> 99kb -> 9kb
Idk if that changes the calculus on the speed here but even if we just leave it at default this is a strong win.

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.

2 participants