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

Drhuffman12 update stumpy bmp #6

Merged
merged 15 commits into from
May 6, 2021
Merged

Conversation

drhuffman12
Copy link
Contributor

NOTE: This replaces #3

As of commit drhuffman12@6d90cf3:

  • I forked stumpy_bmp as-is and tried to run the specs (using Crystal 1.0+). The specs wouldn't run; instead I got errors. (See Not working on Crystal 1+ #4)
  • So, I made some 'updates' and managed to get the specs to run; however, the spec failed with 1/1 tests failing.
  • So, I split the one test into per-pixel tests; the spec failures went from 1/1 failure to 51/60 failures (an improvement).
  • Now, I'm wondering what is causing the mis-calculations of the colors? Was it my 'updates', or differences in how Crystal calculates things, or something else?
  • Since drhuffman12@6d90cf3, I've done some code splitting (to make it easier to test smaller chunks so as to help narrow down the issues), prep'd it for docs, added another test image, switched the tests over to Spectator, etc. You can keep or ignore these additional changes if you want as you see fit.

Any suggestions for why the the pixel color tests are not matching up? If not, I can work thru adding tests for the split-up methods.

My goal is to have:
(a) This shard working on Windows, Mac, and Linux .. and with Crystal 1+ ...
(b) All the specs passing
(c) Add some write methods (so that I can use it for https://github.com/drhuffman12/crystal_ray_tracer and maybe also for https://github.com/edin/raytracer)
It is at least running on all 3 OS's, so that much is good; next we need to get it to pass the specs. Then, add some write methods (see #2).

* drhuffman12_update_stumpy_bmp_part_2 code cleanup

* drhuffman12_update_stumpy_bmp_part_2 re-add original example image as 'spec/stumpy_bmp/example/example1/image.bmp'

* drhuffman12_update_stumpy_bmp_part_2 re-add '.travis.yml' file (for comparison and in case they want to keep it)

* drhuffman12_update_stumpy_bmp_part_2 add 3rd example image (3wx7h w/ transparency, so 32 bbp)

* drhuffman12_update_stumpy_bmp_part_2 add 'SHARDS_OPTS=--ignore-crystal-version' to travis config

* drhuffman12_update_stumpy_bmp_part_2 Adjust to handle 32 (bgra) vs 24 (bgr) bits per pixel

* drhuffman12_update_stumpy_bmp_part_2 add 'example2' tests and split generic tests from 'example0' tests

* drhuffman12_update_stumpy_bmp_part_2 code cleanup

* drhuffman12_update_stumpy_bmp_part_2 add 'file_data_spec.cr' and update 'bmp_spec.cr' for original image

* drhuffman12_update_stumpy_bmp_part_2 code cleanup; prep for tests re remaining  methods

* drhuffman12_update_stumpy_bmp_part_2 'StumpyCore::Canvas' vs 'Canvas'
@l3kn
Copy link
Collaborator

l3kn commented May 4, 2021

Very nice, thanks!

I'm not programming in crystal anymore but if @sol-vin is ok with it, I can add you as a maintainer for this repo.

@sol-vin sol-vin closed this May 5, 2021
@sol-vin sol-vin reopened this May 5, 2021
@sol-vin
Copy link
Member

sol-vin commented May 5, 2021

My bad, totally fine with me!

@sol-vin sol-vin marked this pull request as ready for review May 5, 2021 21:06
@sol-vin sol-vin requested a review from l3kn May 5, 2021 21:06
@sol-vin sol-vin merged commit b249f9f into stumpycr:master May 6, 2021
@drhuffman12
Copy link
Contributor Author

drhuffman12 commented May 7, 2021

Thanks! :) My next step would be to add method(s) for writing bmp files, at least supporting the 24bit rgb and 32bit rgba that I added tests for via this pr. See: drhuffman12#5 .

@drhuffman12
Copy link
Contributor Author

Also, can we get a Release bump? Thanks.

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