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

Yet another PR like #122 #124

Closed
wants to merge 1 commit into from
Closed

Conversation

3442853561
Copy link

Yet another PR like #122
Add set_dpi

Yet another PR like image-rs#122
Add set_dpi
@HeroicKatora
Copy link
Member

You don't need to open a new PR by the way, git allows you to force push the branch and Github then updates the PR automatically. Not sure if this is possible through the web-interface.

@fintelia
Copy link
Contributor

Yeah, the commands would be something like:

$ git add foo.rs bar.rs
$ git commit --amend
$ git push --force-with-lease

@HeroicKatora
Copy link
Member

Unfortunately, the unchecked cast from f64 to u32 in from_dpi may not be as safe as one would like. Even if, as currently expected, it were to adopt saturating semantics the value could be unexpectedly lower than intended. It's also slightly confusing to have set_dpi but no PixelDimensions::from_dpi to parallel it.

The PR is already good without set_dpi as it is pure utility. Can we address and design that in separate issue?

@HeroicKatora
Copy link
Member

HeroicKatora commented Apr 30, 2019

The conversion from inch to meter also follows from the definition of an inch as 25.4mm. Hence, the more precise number to use is (1000.0/25.4) but that shouldn't matter greatly for most calculations.

@birktj birktj mentioned this pull request May 2, 2019
14 tasks
@fintelia
Copy link
Contributor

Superseded by #403

@fintelia fintelia closed this Jul 23, 2023
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.

3 participants