Thread overview | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
June 11, 2014 Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Hello. I am new to D and I must admit I really like the language. In my opinion it takes the best from C++ and, say, Python and combines it really elegantly. Great work! I am currently working on my first library in D - related to TARGA image format. Here's the link to the repo: http://bit.ly/1mIuGhv It's a work-in-progress case: at the moment the library does what I need for my other projects, but there is a couple of things that I want to add/fix soon. Perhaps someone could have a look at the code and point out some obvious traps that I fell into etc. Any feedback would be great! Best regards, Mike |
June 12, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Mike | On Wednesday, 11 June 2014 at 18:29:27 UTC, Mike wrote: > Hello. > Here's the link to the repo: http://bit.ly/1mIuGhv Hi, sorry didn't read through your code yet, but while ago I wrote some encoders/decoders for jpeg and png (https://github.com/callumenator/imaged, haven't compiled it in a while). Might it be worth stitching things together into a proper image processing package? Cheers, cal |
June 12, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to cal | On Thursday, 12 June 2014 at 00:20:28 UTC, cal wrote:
> Might it be worth stitching things together into a proper image processing package?
Well I started working on TGA because I was disappointed that no image abstraction is present in Phobos. Go has some imaging APIs and I think D would benefit from having one too (out of the box).
Would I work on std.image? Sure.
Best,
Mike
|
June 12, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Mike | On Thursday, 12 June 2014 at 15:46:12 UTC, Mike wrote:
> On Thursday, 12 June 2014 at 00:20:28 UTC, cal wrote:
>> Might it be worth stitching things together into a proper image processing package?
>
> Well I started working on TGA because I was disappointed that no image abstraction is present in Phobos. Go has some imaging APIs and I think D would benefit from having one too (out of the box).
>
> Would I work on std.image? Sure.
>
> Best,
> Mike
I'm looking over your code ATM but I'd like to reply to this first.
IMO it's not a good idea to create something like std.image. The std lib should ideally never have breaking changes, so it's easy to get stuck with a sub optimal API or become increasingly hard to maintain.
We have Dub. Better keep the std lib lean and maintainable. If you're looking to create an awesome idiomatic D image library you're probably better of building it on top of derelict-devil or derelict-freeimage, then publish it on code.dlang.org
The discoverability of good code.dlang.org projects is still limited. Some kind of rating or like system would be useful, but that's another discussion.
|
June 12, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Rene Zwanenburg | Le 12/06/2014 20:09, Rene Zwanenburg a écrit :
> On Thursday, 12 June 2014 at 15:46:12 UTC, Mike wrote:
>> On Thursday, 12 June 2014 at 00:20:28 UTC, cal wrote:
>>> Might it be worth stitching things together into a proper image
>>> processing package?
>>
>> Well I started working on TGA because I was disappointed that no image
>> abstraction is present in Phobos. Go has some imaging APIs and I think
>> D would benefit from having one too (out of the box).
>>
>> Would I work on std.image? Sure.
>>
>> Best,
>> Mike
>
> I'm looking over your code ATM but I'd like to reply to this first.
>
> IMO it's not a good idea to create something like std.image. The std lib
> should ideally never have breaking changes, so it's easy to get stuck
> with a sub optimal API or become increasingly hard to maintain.
>
> We have Dub. Better keep the std lib lean and maintainable. If you're
> looking to create an awesome idiomatic D image library you're probably
> better of building it on top of derelict-devil or derelict-freeimage,
> then publish it on code.dlang.org
>
> The discoverability of good code.dlang.org projects is still limited.
> Some kind of rating or like system would be useful, but that's another
> discussion.
I think it can be a great advantage to have some things like image management in phobos, cause often dub projects are big and users don't want necessary a complete multimedia library but just small pieces that are standard.
For example a GUI library, will allow image manipulations, but not only, and extracting only the image modules can be hard. That the case of our DQuick project.
For a project like DQuick, I would be happy to find things like images, geometric algebra,environment analysis,... in phobos. This will allow use to be focused on other things making an UI framework (Windows, events, rendering, resource management,...).
Having such minimalistic APIs would be a great benefit IMO, maybe in this case some extending libraries would appear in dub.
|
June 12, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Xavier Bigand | Le 12/06/2014 20:35, Xavier Bigand a écrit :
> Le 12/06/2014 20:09, Rene Zwanenburg a écrit :
>> On Thursday, 12 June 2014 at 15:46:12 UTC, Mike wrote:
>>> On Thursday, 12 June 2014 at 00:20:28 UTC, cal wrote:
>>>> Might it be worth stitching things together into a proper image
>>>> processing package?
>>>
>>> Well I started working on TGA because I was disappointed that no image
>>> abstraction is present in Phobos. Go has some imaging APIs and I think
>>> D would benefit from having one too (out of the box).
>>>
>>> Would I work on std.image? Sure.
>>>
>>> Best,
>>> Mike
>>
>> I'm looking over your code ATM but I'd like to reply to this first.
>>
>> IMO it's not a good idea to create something like std.image. The std lib
>> should ideally never have breaking changes, so it's easy to get stuck
>> with a sub optimal API or become increasingly hard to maintain.
>>
>> We have Dub. Better keep the std lib lean and maintainable. If you're
>> looking to create an awesome idiomatic D image library you're probably
>> better of building it on top of derelict-devil or derelict-freeimage,
>> then publish it on code.dlang.org
>>
>> The discoverability of good code.dlang.org projects is still limited.
>> Some kind of rating or like system would be useful, but that's another
>> discussion.
>
> I think it can be a great advantage to have some things like image
> management in phobos, cause often dub projects are big and users don't
> want necessary a complete multimedia library but just small pieces that
> are standard.
> For example a GUI library, will allow image manipulations, but not only,
> and extracting only the image modules can be hard. That the case of our
> DQuick project.
>
> For a project like DQuick, I would be happy to find things like images,
> geometric algebra,environment analysis,... in phobos. This will allow
> use to be focused on other things making an UI framework (Windows,
> events, rendering, resource management,...).
>
> Having such minimalistic APIs would be a great benefit IMO, maybe in
> this case some extending libraries would appear in dub.
I am thinking an other benefit is what you see as a default, the assurance of D standard conformances (portability, safety, quality, support,...).
|
June 12, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Xavier Bigand | I can shed some light on my, beginners, point of view and the rationale behind this tiny library. I want to create a bar/matrix-code generator (QR, DataMatrix etc.). I really like graphics-related subjects and I thought this would be a good start in D. Coming from Java island and having experience in Go I expected to find some basic imaging functionalities in the standard library: not necessary support for all image formats, but at least some bitmap i/o and data model (Pixel, Image, Filter ...). I found none of that :-( IMHO (one of the) pain(s) of C++ is that the stdlib is far behind what a modern developer would consider "elementary". But I expected something from D, because Phobos is already more than C++'s stdlib is: Phobos has e.g. digests. IMO digests are not (as) necessary for a standard library as, say, a qsort is. So if there are digests, why not imaging? Either way, D is really nice and if I can help, I will :-) So far - the TGA lib. Best, Mike |
June 12, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Mike | On Wednesday, 11 June 2014 at 18:29:27 UTC, Mike wrote: > Here's the link to the repo: http://bit.ly/1mIuGhv I usually don't trust shortened URL's. Can you please post full URL's when not constrained by a character limit? > Any feedback would be great! First of all, I like your coding style. It's nice and readable :) https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d All those functions can be marked pure. Pure is D is a bit different than in other languages. I can recommend the following article as a good explanation: http://klickverbot.at/blog/2012/05/purity-in-d/ Also, depending on your preference, you may want to put those functions in a pure nothrow { } block, or put pure nothrow: at the top of the source file. I'm not sure if all those functions can be nothrow though. https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L8 These array literals will be allocated every time the function is called. 'only' can be used instead: only(16, 32).canFind // etc.. http://dlang.org/library/std/range/only.html I remember a function which does something like only only + canFind on one go. It would look something like header.colorMapDepth.among(16, 32); but I can't find it right now.. Maybe it was only proposed but never added. https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L101 This has a pretty bad time complexity. In the worst case this is O(n^2), I think. You could use a hashmap to build the table, then convert to an array. Or even only use hashmaps as color tables internally to improve performance across the board. https://github.com/emesx/TGA/blob/develop/source/tga/model/types.d#L11 Depending on your taste, a union + anonymous struct could be used: struct Pixel { union { ubyte[4] bytes; struct { ubyte b, g, r, a; } } } https://github.com/emesx/TGA/blob/develop/source/tga/model/validation.d#L47 to!string is so common it has a special alias: text. It's placed in std.conv so you still need to import that. "Invalid color map pixel depth: " ~ header.colorMapDepth.text https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L24 Yeah, we need something to read an entire struct from a file and convert it to the correct endianness.. https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L24 https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L91 Unnecessary GC allocation in a hot loop == :( Make a buffer outside the loop and call the normal rawRead: auto buffer = new ubyte[colorMapByteDepth]; foreach(i; 0 .. (header.colorMapLength - header.colorMapOffset)) { file.rawRead(buffer); colorMap[i] = pixelReader(buffer); } Even better, use a static array with the max byte depth, then slice to the correct length: ubyte[MaxByteDepth] buffer; foreach(i; 0 .. (header.colorMapLength - header.colorMapOffset)) { colorMap[i] = file.rawRead(buffer[0 .. colorMapByteDepth]).pixelReader; } Do the same thing here: https://github.com/emesx/TGA/blob/develop/source/tga/io/utils.d#L5 T read(T)(File file) if(isNumeric!T){ ubyte[T.sizeof] bytes; file.rawRead(s[]); return littleEndianToNative!T(bytes); } https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L144 Depending on your taste. The first is built in, the other needs std.algorithm. pixel.bytes[] = chunk[]; chunk.copy(pixel.bytes); https://github.com/emesx/TGA/blob/develop/source/tga/io/utils.d#L40 There's a version in std.algorithm: http://dlang.org/phobos/std_algorithm.html#min I need to go. Please don't mind any typo's and untested code ;). Didn't take a look at writers yet, readers and util need some more scrutiny, but the main theme is: eliminate unnecessary temporary GC allocations. |
June 13, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Rene Zwanenburg | On Thursday, 12 June 2014 at 19:30:06 UTC, Rene Zwanenburg wrote:
> I need to go. Please don't mind any typo's and untested code ;). Didn't take a look at writers yet, readers and util need some more scrutiny, but the main theme is: eliminate unnecessary temporary GC allocations.
Thank you for feedback, hoping for more! I will apply the suggestions tonight.
|
June 13, 2014 Re: Working on a library: request for code review | ||||
---|---|---|---|---|
| ||||
Posted in reply to Rene Zwanenburg | On 06/12/2014 09:30 PM, Rene Zwanenburg wrote: > I remember a function which does something like only only + canFind on > one go. It would look something like > > header.colorMapDepth.among(16, 32); > > but I can't find it right now.. Maybe it was only proposed but never added. http://dlang.org/library/std/algorithm/among.html -- Mike Wey |
Copyright © 1999-2021 by the D Language Foundation