Closed huggablemonad closed 6 years ago
Hi @huggablemonad this looks great! Thanks for the detailed write-up of the pros and cons. I'll take a closer look at the code this weekend.
@huggablemonad Sorry about the delay -- I wrote a long comment last week but I guess I forgot hit the "Comment" button. >_<
I'll save detailed comments for when you're ready to merge. Only thing I want to say at this point is that if you want to avoid the integration with the rest of the app, you can. In my #49 PR, I'm trying to set up AWS Lambdas that do the image conversion anyway. I bet your solution will be relatively easy to bundle up into a single executable! If the FaaS solution as a whole ends up being really annoying (still not certain), I/you/we can always do the integration as a new PR.
@huggablemonad
Note that the choice of 17 as the multiplier differs from cel2pnm's. cel2pnm uses 16, and the effect that this has is to make the colors a slightly darker shade (approximately a 6% difference: 100 - 16 / 17 * 100 = 5.88).
Fascinating! Thanks for catching this.
Hi @emhoracek, I think this pull request is ready for review. Thanks! :smile:
@huggablemonad Sorry for leaving this for so incredibly long! A couple things I ran into:
I managed to fix these and get everything to compile -- is it okay if I push to your branch?
I managed to fix these and get everything to compile -- is it okay if I push to your branch?
Definitely! Thanks for taking a look at this.
@huggablemonad I just created an issue for actually integrating the code you wrote because I want to go ahead and merge this -- I think it looks great and (despite the long silence!!) I really do appreciate you taking the time to do this.
This pull request is intended to fix #41, the goal of which is to move the cel conversion pipeline into Haskell. It simplifies the installation for new contributors by not requiring a separate compilation step for
cel2pnm
, and also removes the need to installnetpbm
.The Haskell port separates the palette parsing, cel parsing, and PNG export code into the
ParseKCF
,ParseCel
, andCelToPng
modules, respectively. ThecelToPng
function is used to convert the parsed palette and cel into an RGBA8 PNG file.There are a few infelicities in the current implementation, the biggest of which is the poor performance.
Use of partial functions: There are a few places where partial functions are used, such as accessing elements of a list. They will need to be changed into a safe variant by following the
listToMaybe
+fromMaybe
idiom elsewhere in the Smooch code.Style: Point-free style needs to be rewritten in order to make the code more accessible to new Haskellers (see @emhoracek's comment in #48 ).
No incremental parsing: The entire file has to be read into memory. This might cause problems if a malicious set was uploaded. One possible solution is to cap the maximum size of a file that will be read.
No palette groups support: This will be addressed in a separate issue in order to keep this pull request manageable for me.
No Cherry KiSS support: This will be added back in the future, probably at the same time that the rest of Smooch gets support for this format.
Slow: The current implementation is roughly 3 times slower at converting cels to PNGs than the combination of
cel2pnm
andpnmtopng
. For instance, converting all 167 cels in the Arwen set took the Haskell version 4.259s vs 1.506s forcel2pnm
andpnmtopng
, as measured by the bench benchmarking tool. When convertingbase.cel
, runtime stats showed at least 50% of the time was spent in the garbage collector, suggesting that there is some kind of memory leak. Profiling revealedgenerateFoldImage
(fromJuicyPixels
) andparseCelData
(fromParseCel
) to be the two most costly functions in terms of time and allocation costs. This information should be enough to provide a starting point for investigating the code's slow performance.At the moment, the code isn't integrated into Smooch. After the problems have been satisfactorily ironed out and changes from feedback incorporated, work can begin on switching Smooch to the Haskell implementation.