-
Notifications
You must be signed in to change notification settings - Fork 102
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
Conversation
Also please be nice, this is my first Rust project ever. I know the code is kinda messy. |
3343efc
to
3dbe3a0
Compare
DNM until spacestation13/dmi-rust#20 |
…nd cleanup command
This PR is now feature complete. |
This is only failing build because rust added a new lint that affects the hash module. |
OpenDream hast mislead my blending again. 😔 |
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. |
@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 |
i would update the build fails but you are cringe and don't let me push to your branch |
sorries |
let pixel_1 = px1.channels(); | ||
let pixel_2 = px2.channels(); | ||
|
||
let blended = Rgba::blend_u8(pixel_1, pixel_2, *blend_mode); |
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.
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!
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.
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.
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.
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.
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.
I don't really have a lot of time to deeply investigate the performance implications of this at the moment
Cannot wait for an implementation of this, please @ me with the PR when its done. |
On TG? |
One more relevant PR if anyone was going to port btw: |
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.
I've also added a panic handler to the core rustg module, since this module is..
prone to panicingNot 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: