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

support EMF #21

Merged
merged 6 commits into from
Nov 2, 2022
Merged

support EMF #21

merged 6 commits into from
Nov 2, 2022

Conversation

opoudjis
Copy link
Contributor

@opoudjis opoudjis commented Oct 21, 2022

Copy link
Owner

@toy toy left a comment

Choose a reason for hiding this comment

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

Thank you for creating the PR.

I've added few comments after digging into this peculiar format.
I also suggest to use non square image for test to be certain about using right values.

Gemfile Outdated Show resolved Hide resolved
lib/image_size.rb Outdated Show resolved Hide resolved
lib/image_size.rb Outdated Show resolved Hide resolved
@opoudjis
Copy link
Contributor Author

Thank you for your extremely diligent feedback. You are a better human being than I am. :-)

@toy
Copy link
Owner

toy commented Oct 30, 2022

Few small things left: CHANGELOG entry, description in gemspec and legacy ruby doesn't understand L< 😞.

I've checked few variants and did a benchmark:

require 'securerandom'
require 'benchmark'

STRINGS = 1_000_000.times.map do
  SecureRandom.random_bytes(16)
end

UMAX = 256**4
SMAX = UMAX / 2

Benchmark.bmbm do |x|
  x.report('repack') do
    STRINGS.map{ |s| s.unpack('V*').pack('L*').unpack('l*') }
  end

  x.report('conditional') do
    STRINGS.map{ |s| s.unpack('V*').map{ |u| u < SMAX ? u : u - UMAX } }
  end

  x.report('arythemical') do
    STRINGS.map{ |s| s.unpack('V*').map{ |u| (u + SMAX) % UMAX - SMAX } }
  end
end

outputs:

Rehearsal -----------------------------------------------
repack        4.400000   0.080000   4.480000 (  4.480014)
conditional   2.860000   0.070000   2.930000 (  2.934557)
arythemical   3.200000   0.030000   3.230000 (  3.228823)
------------------------------------- total: 10.640000sec

                  user     system      total        real
repack        4.590000   0.050000   4.640000 (  4.641003)
conditional   2.500000   0.010000   2.510000 (  2.510647)
arythemical   3.000000   0.020000   3.020000 (  3.012003)

not sure which is better from clarity/readability and maybe there is another option. Alternative can be conditionally used for RUBY_VERSION < '1.9'.

@toy
Copy link
Owner

toy commented Oct 30, 2022

Also forgot to attach non square emf converted from svg using Inkscape + changed values to also be negative
74x77.emf.zip (zip as emf is not supported for upload)

@opoudjis
Copy link
Contributor Author

opoudjis commented Nov 1, 2022

Thank you, will get to these.

Copy link
Owner

@toy toy left a comment

Choose a reason for hiding this comment

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

Thank you for all the changes!
To keep the history tidy could you compact commits or do you prefer that I squash merge?

@opoudjis
Copy link
Contributor Author

opoudjis commented Nov 1, 2022

Happy for you to squash merge.

@opoudjis
Copy link
Contributor Author

opoudjis commented Nov 2, 2022

And can I say again, Ivan, you are a very good human being, and I'm honoured to have contributed to your gem. Thank you for your help and diligence!

@toy toy merged commit a79f265 into toy:master Nov 2, 2022
@toy
Copy link
Owner

toy commented Nov 2, 2022

Thank you for taking time to contribute to the gem!

I've released version 3.2.0

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