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

IconForge - Building spritesheets at the speed of light #160

Merged
merged 37 commits into from
Apr 20, 2024

Conversation

itsmeow
Copy link
Contributor

@itsmeow itsmeow commented Dec 23, 2023

Adds IconForge, a new module for building asset spritesheets.

It's over 10x faster than BYOND in some cases. This module just takes a JSON input and spits out a spritesheet to a path, so it's up to the DM side to actually use it correctly, but I've tested it with a branch on Bee: https://github.com/itsmeowForks/BeeStation-Hornet/tree/asset-rustg

It is quite fast.
image

I've also added a panic handler to the core rustg module, since this module is.. prone to panicing Not anymore, but it was useful in testing. This should also help diagnose crashes from within rust-g without needing to take minidumps.

In the future I want to add a more advanced caching system to this, but as it is, it's already just a faster replacement for the existing spritesheet generation code in DM, and people are interested, so here it is. The way the JSON input is designed allows you to precisely determine if anything has changed, since it only supports DMI+icon_states and not any BYOND /icons. You can basically just hash everything and compare to get better caching.

Also adds a caching validation tool. This hashes all DMIs within the sprites object and compares them to a previous one provided by DM. It also provides the hashes as the result of the generate() function when prompted. It is up to DM to do the rest, rustg only provides a fast method for comparing the hashes with parallelism.

Cache validation check doesn't cost too much:
image

@itsmeow
Copy link
Contributor Author

itsmeow commented Dec 23, 2023

Also please be nice, this is my first Rust project ever. I know the code is kinda messy.

src/iconforge.rs Outdated Show resolved Hide resolved
src/iconforge.rs Outdated Show resolved Hide resolved
src/iconforge.rs Outdated Show resolved Hide resolved
@itsmeow
Copy link
Contributor Author

itsmeow commented Dec 24, 2023

DNM until spacestation13/dmi-rust#20

Cargo.toml Outdated Show resolved Hide resolved
@itsmeow
Copy link
Contributor Author

itsmeow commented Dec 25, 2023

If you need proof of this thing's pure efficiency, here's a graph comparison on the DM side.
image

@itsmeow
Copy link
Contributor Author

itsmeow commented Dec 27, 2023

This PR is now feature complete.

Cargo.toml Outdated Show resolved Hide resolved
@itsmeow
Copy link
Contributor Author

itsmeow commented Jan 8, 2024

This is only failing build because rust added a new lint that affects the hash module.

@itsmeow
Copy link
Contributor Author

itsmeow commented Jan 10, 2024

OpenDream hast mislead my blending again. 😔

@itsmeow
Copy link
Contributor Author

itsmeow commented Jan 10, 2024

I have a branch implementing GAGS into iconforge, although I'd prefer it to be part of another PR. Should I just push it here or wait for merge? It's just one commit atm.

@optimumtact
Copy link
Member

@ZeWaka is there a reason this isn't merge yet

@ZeWaka
Copy link
Contributor

ZeWaka commented Apr 15, 2024

@ZeWaka is there a reason this isn't merge yet

Waiting on me to do a final review early this week per my comment yesterday in tooling questions

src/hash.rs Outdated Show resolved Hide resolved
src/iconforge.rs Show resolved Hide resolved
src/iconforge.rs Outdated Show resolved Hide resolved
dmsrc/iconforge.dm Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@ZeWaka
Copy link
Contributor

ZeWaka commented Apr 16, 2024

i would update the build fails but you are cringe and don't let me push to your branch

@itsmeow
Copy link
Contributor Author

itsmeow commented Apr 16, 2024

sorries

let pixel_1 = px1.channels();
let pixel_2 = px2.channels();

let blended = Rgba::blend_u8(pixel_1, pixel_2, *blend_mode);
Copy link

@Cyprex Cyprex Apr 16, 2024

Choose a reason for hiding this comment

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

I don't think this way of looping is particularly performant- blend_mode is a loop invariant so this seems suitable for SIMD, but because the switch for blend_mode happens later down the line for each pixel the compiler may have a hard time taking advantage of that. Imo the switch for blend_mode should be hoisted outside of this loop, so that the actual pixel operations can be a tight loop. That is assuming the preceding body of this loop obtaining the pixel values is very cheap, that the actual blending perf even matters!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are correct, good catch! I can peek at the assembly and see if the compiler actually inlines the blend function. It would probably make the code much messier though, maybe I can use a macro or something.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you look at the tracy profiling, most of the time is spent parsing DMI files from the disk, but icon blending is still a pretty decent portion, especially for spritesheets like preferences which use a lot of operations. The main limitation is actually IO which is why everything is batched into groups and multithreaded where possible. Pretty light on actual computation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't really have a lot of time to deeply investigate the performance implications of this at the moment

@ZeWaka
Copy link
Contributor

ZeWaka commented Apr 16, 2024

@ZeWaka ZeWaka merged commit 9a104a6 into tgstation:master Apr 20, 2024
2 checks passed
@AffectedArc07
Copy link
Member

Cannot wait for an implementation of this, please @ me with the PR when its done.

@itsmeow
Copy link
Contributor Author

itsmeow commented Apr 21, 2024

Cannot wait for an implementation of this, please @ me with the PR when its done.

On TG?
I've already implemented it on Bee, the PRs are mentioned above.

@itsmeow
Copy link
Contributor Author

itsmeow commented May 28, 2024

One more relevant PR if anyone was going to port btw:
BeeStation/BeeStation-Hornet#10649

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.

5 participants