fiji / VIB-lib

https://imagej.net/VIB_Protocol
4 stars 29 forks source link

Amira io #3

Closed iarganda closed 10 years ago

iarganda commented 10 years ago

These changes correct different bugs when saving/loading images as AmiraMesh:

dscho commented 10 years ago

Cool! But hey, how about removing the jzlib dependency now?

dscho commented 10 years ago

Looks pretty good already! A couple of changes here and there and we're good to merge! Thanks!

iarganda commented 10 years ago

@dscho Thanks for your useful remarks! How should we proceed now? Do you want to pick some of the changes and remove the other ones? What should I use?

dscho commented 10 years ago

How should we proceed now?

I would really appreciate it if you could spend a little bit of time to address the concerns I raised...

iarganda commented 10 years ago

Yes, yes, of course. I was just wondering if I should continue adding new commits in this branch since you didn't like some of them.

Working on it!

dscho commented 10 years ago

I was just wondering if I should continue adding new commits in this branch

Please avoid adding on top of flawed commits, and rewrite the branch instead. I provided some hints in that regard for another OSS project.

iarganda commented 10 years ago

I see, that's what I was wondering. Thanks for the link! There are not too many changes so I could actually write the whole branch from scratch instead of rebasing.

iarganda commented 10 years ago

@dscho OK, i think i addressed all your comments, but please, make sure i did.

I left the reading/writing of each slice as it was before (with one I/O file access per slice). I you don't mind, I would like to write a couple of public methods to do it in a single access at the cost of more memory so I still have that possibility.

dscho commented 10 years ago

This looks already pretty good! I wrote up in a pretty detailed way how I would suggest to address the remaining issues.

I[f] you don't mind, I would like to write a couple of public methods to do it in a single access at the cost of more memory so I still have that possibility.

Oh, I would welcome that!

iarganda commented 10 years ago

That was really useful! Thanks again, Dscho, and sorry for all the corrections you still had to point out.

dscho commented 10 years ago

sorry for all the corrections you still had to point out.

No worries. I wonder whether you could add some documentation that would have helped you better than I did, i.e. before opening the Pull Request? I believe that the future contributors would benefit a lot from it.

dscho commented 10 years ago

I will release a new version and upload it later today, thank you very much for your contribution!

iarganda commented 10 years ago

Oops, I think I added some changes after you closed the pull request. I'll upload them in a few minutes. Sorry about that.

I'll definitely work in that documentation.

iarganda commented 10 years ago

Please, do not make that release yet. I might have found another problem. Working on it!