Re: Working on a library: request for code review
On Friday, 20 June 2014 at 11:15:20 UTC, Mike wrote: Do you think it's ready for a v0.1 release? It be willing to add it to dub if it passes general D coding stardards. Sure, go for it :). Dub package versioning should be done using SemVer so there are some restrictions on how to number a release. http://semver.org/
Re: Working on a library: request for code review
Do you think it's ready for a v0.1 release? It be willing to add it to dub if it passes general D coding stardards. Once again, thanks for feedback and vast improvements of the code. Best, Mike
Re: Working on a library: request for code review
Once again thanks for the feedback. The RLE rewrite using std is impressive! I will use it today or tomorrow, great stuff.
Re: Working on a library: request for code review
On Thursday, 19 June 2014 at 12:06:58 UTC, Marc Schütz wrote: On Wednesday, 18 June 2014 at 14:05:12 UTC, Rene Zwanenburg wrote: On Tuesday, 17 June 2014 at 13:07:33 UTC, Marc Schütz wrote: On Monday, 16 June 2014 at 23:04:33 UTC, Rene Zwanenburg wrote: This one depends on taste, but these helpers can be eliminated by changing the Header definition a little. It's the same union / anonymous struct trick from the previous post, only this time with bitfields: http://dpaste.dzfl.pl/13258b0ce0c4 If you define nested structs/unions, it's better to make them static whenever possible, because non-static nested structs have an additional hidden field for the context. Good call. In this case it doesn't add the hidden field, but for the sake of consistency I agree it's better to add static. Interesting, I thought it's there for all nested structs. But I see it's only added if the struct has a method, but then it's always there, no matter whether the methods actually accesses the outer context. Probably for consistency reasons... The absence of the hidden field without methods is probably to maintain C compitability; code that is both valid C and D must have the same semantics. I think that includes struct layout?
Re: Working on a library: request for code review
On Wednesday, 18 June 2014 at 14:05:12 UTC, Rene Zwanenburg wrote: On Tuesday, 17 June 2014 at 13:07:33 UTC, Marc Schütz wrote: On Monday, 16 June 2014 at 23:04:33 UTC, Rene Zwanenburg wrote: This one depends on taste, but these helpers can be eliminated by changing the Header definition a little. It's the same union / anonymous struct trick from the previous post, only this time with bitfields: http://dpaste.dzfl.pl/13258b0ce0c4 If you define nested structs/unions, it's better to make them static whenever possible, because non-static nested structs have an additional hidden field for the context. Good call. In this case it doesn't add the hidden field, but for the sake of consistency I agree it's better to add static. Interesting, I thought it's there for all nested structs. But I see it's only added if the struct has a method, but then it's always there, no matter whether the methods actually accesses the outer context. Probably for consistency reasons...
Re: Working on a library: request for code review
On Tuesday, 17 June 2014 at 13:07:33 UTC, Marc Schütz wrote: On Monday, 16 June 2014 at 23:04:33 UTC, Rene Zwanenburg wrote: This one depends on taste, but these helpers can be eliminated by changing the Header definition a little. It's the same union / anonymous struct trick from the previous post, only this time with bitfields: http://dpaste.dzfl.pl/13258b0ce0c4 If you define nested structs/unions, it's better to make them static whenever possible, because non-static nested structs have an additional hidden field for the context. Good call. In this case it doesn't add the hidden field, but for the sake of consistency I agree it's better to add static. Also, when you work with bit-(un)packing, you need to make sure it works on big- and little-endian CPUs. Yeah. He's using a custom read function that takes endianness into account, so I didn't mention it.
Re: Working on a library: request for code review
On Monday, 16 June 2014 at 23:04:33 UTC, Rene Zwanenburg wrote: The writeCompressed function can probably be simplified using std.algorithm but it's not straightforward. I'll need to ponder it for a while. Here's a version using std.algorithm and std.range. I think it's easier to understand than the original: http://dpaste.dzfl.pl/88ba2592ac8d
Re: Working on a library: request for code review
On Wednesday, 18 June 2014 at 13:42:32 UTC, Rene Zwanenburg wrote: On Wednesday, 18 June 2014 at 09:41:01 UTC, Marc Schütz wrote: On Tuesday, 17 June 2014 at 18:35:34 UTC, Mike wrote: Thanks, will work on fixes tonight. The current method will not detect an error when the image type is not mapped but a color map is present. At least I assume that is not a valid TGA file? A non-mapped image may contain a color map ;-0 it's simply discarded. As per your ideas return to!bool(isColorMapped(header) ? header.colorMapDepth.among(16, 32) : header.pixelDepth.among(16, 32)); this is not nothrow, but as you can see there is nothing that can ever throw.. so perhaps I'll stick to the cast(bool) thing.. Any idea? Perhaps `return !!(...);` ? test The web interface didn't work for me for about a day. It seems to be over now. Yeah I didn't take nothrow into account. AFAIK using !!() to convert to bool in a context where it doesn't happen implicitly is indeed idiomatic. At least it's used in Phobos.
Re: Working on a library: request for code review
On Wednesday, 18 June 2014 at 09:41:01 UTC, Marc Schütz wrote: On Tuesday, 17 June 2014 at 18:35:34 UTC, Mike wrote: Thanks, will work on fixes tonight. The current method will not detect an error when the image type is not mapped but a color map is present. At least I assume that is not a valid TGA file? A non-mapped image may contain a color map ;-0 it's simply discarded. As per your ideas return to!bool(isColorMapped(header) ? header.colorMapDepth.among(16, 32) : header.pixelDepth.among(16, 32)); this is not nothrow, but as you can see there is nothing that can ever throw.. so perhaps I'll stick to the cast(bool) thing.. Any idea? Perhaps `return !!(...);` ? test
Re: Working on a library: request for code review
On Tuesday, 17 June 2014 at 18:35:34 UTC, Mike wrote: Thanks, will work on fixes tonight. The current method will not detect an error when the image type is not mapped but a color map is present. At least I assume that is not a valid TGA file? A non-mapped image may contain a color map ;-0 it's simply discarded. As per your ideas return to!bool(isColorMapped(header) ? header.colorMapDepth.among(16, 32) : header.pixelDepth.among(16, 32)); this is not nothrow, but as you can see there is nothing that can ever throw.. so perhaps I'll stick to the cast(bool) thing.. Any idea? Perhaps `return !!(...);` ?
Re: Working on a library: request for code review
Thanks, will work on fixes tonight. The current method will not detect an error when the image type is not mapped but a color map is present. At least I assume that is not a valid TGA file? A non-mapped image may contain a color map ;-0 it's simply discarded. As per your ideas return to!bool(isColorMapped(header) ? header.colorMapDepth.among(16, 32) : header.pixelDepth.among(16, 32)); this is not nothrow, but as you can see there is nothing that can ever throw.. so perhaps I'll stick to the cast(bool) thing.. Any idea?
Re: Working on a library: request for code review
On Monday, 16 June 2014 at 23:04:33 UTC, Rene Zwanenburg wrote: This one depends on taste, but these helpers can be eliminated by changing the Header definition a little. It's the same union / anonymous struct trick from the previous post, only this time with bitfields: http://dpaste.dzfl.pl/13258b0ce0c4 If you define nested structs/unions, it's better to make them static whenever possible, because non-static nested structs have an additional hidden field for the context. Also, when you work with bit-(un)packing, you need to make sure it works on big- and little-endian CPUs.
Re: Working on a library: request for code review
On Monday, 16 June 2014 at 19:42:14 UTC, Mike wrote:
I have refactored the code as recommended.
I have also modified the not-yet-reviewed writers part to take
advantage of the same approach (preallocated static-sized
buffer) rather than allocate slices in loops.
Hoping to hear something from you guys!
Best,
Mike
https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L80
Fully qualified names are only necessary when there's a name
conflict. The std.algorithm can be removed.
https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L8
It's a shame the result of among can't be returned directly but
casts are terribly heavy handed. They're best reserved for when
you're poking holes in the type system ;)
For safe conversions use to!something:
return to!bool(isColorMapped(header)
? header.colorMapDepth.among(16, 32)
: header.pixelDepth.among(16, 32));
https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L29
This one depends on taste, but these helpers can be eliminated by
changing the Header definition a little. It's the same union /
anonymous struct trick from the previous post, only this time
with bitfields:
http://dpaste.dzfl.pl/13258b0ce0c4
https://github.com/emesx/TGA/blob/develop/source/tga/model/utils.d#L92
Hashtable lookup would indeed be nice here, but if you ever need
a linear lookup it's included in std.algorithm:
http://dlang.org/library/std/algorithm/countUntil.html
const index = colorMap.countUntil(pixel);
enforce(index >= 0, "Pixel color not found in color map: " ~
pixel.text);
return index.to!ushort;
Note the use of to!ushort to avoid the cast. to!ushort also
performs overflow checking.
https://github.com/emesx/TGA/blob/develop/source/tga/model/validation.d#L52
This is one of the many places where the allowed bitdepths are
enumerated in an array / among(). Putting the allowed bitdepths
in an enum has some advantages: use of '.max' for buffer sizes,
EnumMembers in conjunction with among and only, to! checking for
correctness... Some examples here:
http://dpaste.dzfl.pl/30d2a593468f
But it depends on your taste. Personally I prefer to use strong
typing like this whenever possible. Helps to find bugs earlier,
and makes maintenance easier when your software and data format
are still in flux.
https://github.com/emesx/TGA/blob/develop/source/tga/model/validation.d#L39
This should be checked both ways. The current method will not
detect an error when the image type is not mapped but a color map
is present. At least I assume that is not a valid TGA file?
enforce(header.colorMapDepth.among(EnumMembers!ColorMapDepth));
enforce(header.isColorMapped == (header.colorMapType ==
ColorMapType.PRESENT));
enforce(header.isColorMapped == (header.colorMapDepth !=
ColorMapDepth.BPP0));
https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L95
You could change this to
foreach(ref pixel; pixels)
{
pixel = ...
}
https://github.com/emesx/TGA/blob/develop/source/tga/io/readers.d#L165
Readability can be improved using the union + bitfields thingy.
It's a bit longer but much easier on the eyes IMO:
http://dpaste.dzfl.pl/77ecbafa1e0e
https://github.com/emesx/TGA/blob/develop/source/tga/io/writers.d#L23
The with statement can be used here:
with(image.header)
{
write(file, idLength);
...
}
I think this is everything I can come up with. The
writeCompressed function can probably be simplified using
std.algorithm but it's not straightforward. I'll need to ponder
it for a while.
Re: Working on a library: request for code review
On Monday, 16 June 2014 at 19:42:14 UTC, Mike wrote: I have refactored the code as recommended. I have also modified the not-yet-reviewed writers part to take advantage of the same approach (preallocated static-sized buffer) rather than allocate slices in loops. Hoping to hear something from you guys! Best, Mike I've been pretty busy this weekend, so sorry for the late reply. I'm taking another look right now.
Re: Working on a library: request for code review
I have refactored the code as recommended. I have also modified the not-yet-reviewed writers part to take advantage of the same approach (preallocated static-sized buffer) rather than allocate slices in loops. Hoping to hear something from you guys! Best, Mike
Re: Working on a library: request for code review
On Friday, 13 June 2014 at 21:09:23 UTC, Mike Wey wrote: 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 Will apply this as well, thanks :-)
Re: Working on a library: request for code review
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
Re: Working on a library: request for code review
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.
Re: Working on a library: request for code review
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.
Re: Working on a library: request for code review
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
Re: Working on a library: request for code review
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,...).
Re: Working on a library: request for code review
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.
Re: Working on a library: request for code review
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.
Re: Working on a library: request for code review
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
Re: Working on a library: request for code review
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
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
