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

Add dmi_resize_png using the image crate #39

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

ShadowLarkens
Copy link
Contributor

Please be gentle this is literally the first time I've ever touched rust ;w;
I am probably going to be absolutely helpless if this needs any optimization or tweaks to file writing

@optimumtact
Copy link
Member

Thanks for the PR, and welcome aboard 👍

src/dmi.rs Outdated Show resolved Hide resolved
@SpaceManiac
Copy link
Contributor

Have you done any tests of how fast this is compared to http://www.byond.com/docs/ref/info.html#/icon/proc/Scale ?

@AnturK
Copy link
Member

AnturK commented Oct 4, 2020

What's the use case here ?

@ShadowLarkens
Copy link
Contributor Author

@AnturK @SpaceManiac The point of this is that Scale() has this little caveat: Scale() automatically performs antialiasing to avoid unwanted artifacts. I have seen examples of this antialiasing not occuring, but... I have no idea why. There doesn't appear to be any consistent way to convince BYOND "Hey, don't just randomly apply bilinear antialiasing to this pixel art that I'm trying to upscale."

@ShadowLarkens
Copy link
Contributor Author

Like, if anyone knows what the rhyme or reason to Scale() occasionally just deciding to apply bilinear antialiasing and how to prevent it from doing that, I'd be happy to implement my icon upscaling with native BYOND procs. This idea came up while working with the spritesheet asset datum and needing to upscale a spritesheet for a UI.

@ZeWaka
Copy link
Contributor

ZeWaka commented Oct 4, 2020

Like, if anyone knows what the rhyme or reason to Scale() occasionally just deciding to apply bilinear antialiasing and how to prevent it from doing that, I'd be happy to implement my icon upscaling with native BYOND procs.

You probably don't have appearance_flags |= PIXEL_SCALE set if you're performing this operation on an atom. However, that var doesn't seem to exist for /icon or /image, which could be your usecase.

@ShadowLarkens
Copy link
Contributor Author

Yeah, it's direct /icon modification that matters.

src/dmi.rs Outdated Show resolved Hide resolved
src/dmi.rs Outdated Show resolved Hide resolved
@ShadowLarkens
Copy link
Contributor Author

Hrm. Can someone help me with this? Even after reading through the rust error handling guidelines, I can't figure out how to implement non-panick file read/write, as the image library returns ImageError instead of Error and there's no type conversion defined for "ImageError -> Error".

@ShadowLarkens
Copy link
Contributor Author

Well it was an adventure, but with PJB's help definitely just doing it for me with how bad I am at rust it finally returns an error and doesn't blow up rust-g when it fails

@spookydonut
Copy link

@ShadowLarkens new conflicts

@ShadowLarkens
Copy link
Contributor Author

@spookydonut Resolved, let me know if there's anything I need to change to follow #42

src/dmi.rs Outdated Show resolved Hide resolved
src/dmi.rs Outdated Show resolved Hide resolved
@spookydonut
Copy link

@aspenluxxxy

src/dmi.rs Outdated Show resolved Hide resolved
src/dmi.rs Outdated
}
}

fn resize_png(path: &str, width: &str, height: &str) -> std::result::Result<(), ResizePngError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn resize_png(path: &str, width: &str, height: &str) -> std::result::Result<(), ResizePngError> {
fn resize_png<P: AsRef<Path>>(path: P, width: u32, height: u32) -> std::result::Result<(), Error> {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can BYOND call functions with u32 arguments? I thought it explicitly only worked with strings

Copy link
Contributor

Choose a reason for hiding this comment

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

Correct, but you'd convert them to u32 in the byond_fn!

Copy link
Contributor Author

@ShadowLarkens ShadowLarkens Oct 15, 2020

Choose a reason for hiding this comment

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

Trying to do that results in the ? no longer working for the ParseIntErrors because byond_fn! doesn't actually return a Result (Or something like that?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's probably not exactly why it doesn't work, but I have no idea how to make it stop yelling at me with this
https://i.tigercat2000.net/2020/10/Code_sYiIJS9DzI.png

src/dmi.rs Outdated Show resolved Hide resolved
src/dmi.rs Outdated Show resolved Hide resolved
src/dmi.rs Outdated Show resolved Hide resolved
Copy link
Member

@Cyberboss Cyberboss left a comment

Choose a reason for hiding this comment

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

Please fix the merge conflict

Tweak from panic to error return

Finally fix the resize_png fn (Oh god thank you PJB3005)

Allow user to pick resize mode, use rust_g::Error instead of custom enum
@ShadowLarkens
Copy link
Contributor Author

@Cyberboss Done

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.

None yet

8 participants