Added std.experimental.color #2845
Conversation
|
As mentioned on the forum discussion, please consider putting this as a library on code.dlang.org first. It's much easier to get feedback this way. Putting it up as an incomplete pull request makes it harder to try out and needlessly uses up auto-tester cycles. (Also, once the module is ready for the Phobos review process, it should be in |
|
|
||
| // declare some common colour types... | ||
|
|
||
| alias Colour!("rgb", ubyte) RGB; |
There was a problem hiding this comment.
New-style aliases please? ;P
|
Not sure what's wrong with your keyboard, but it seems to have thrown up u's everywhere. |
|
@klickverbot Ah yes, the auto-tester... didn't think of that. I just threw it here because I could do it in 2 seconds, and I didn't look what I need to do to put it on code.dlang.org yet. |
The simpler the word the harder it is to make spelling mistakes, and those are annoying while coding. So I'd prefer |
|
The core libraries have consistently used American English so far. So unless we want to either change existing names or intentionally introduce inconsistency, I'd say there's no doubt American English should be used here as well. |
|
I was half joking about the u's :) I actually don't care, as I likely will not use it (not my area). It's probably something to ask the forum (what colour should the bikeshed be?) |
|
Ask on the forum?! You know that's gonna result in Yet Another Interminable Thread on the colo(u)r of the bikeshed, right? :-P |
|
Let's settle this with a good old fashioned google fight. |
|
Bloody hell, I'll change the colour of the bikeshed to color! (such valuable feedback) ;) Seriously, it's force of habit, and with cut&paste + auto-complete, everyone knows you only ever type any given word once in code... and it doesn't look wrong to me. |
|
This module may be considered tiny by Phobos standards, but I think it would better if you split it in two (or more) modules:
In short: I think tagging that Color type with color space is a great idea just put the conversion stuff in a separate file. |
|
@ZombineDev I'm not sure what you mean by the first point? I don't really want to define some arbitrary set POD's for this. I'd rather everything be an instance of Colour, then you can detect that various different kinds of colours are all infact Colours's. If they were unrelated POD's, then there'd be no obvious relationship Currently, the conversion functions are all separate. Basically, 'convert' (bad name in retrospect) converts between component types, 'convertColourSpace' converts between colour space, and those have nothing to do with the actual Colour type, so they are general functions anyone can use. Why should I separate that into a separate module? That kinda seems overkill... I was thinking of pre-defined colours. I think they would need to be enum members of the Colour type, such that they are expressed in the appropriate format+colour space for each instance. |
| return To(convert!TyT(a), convert!TyT(b), convert!TyT(c), convert!TyT(src[3])); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
What happens if this is false, don't you need a return here?
There was a problem hiding this comment.
It's impossible, or the constraint would fail.
The static if is only for the is() expressions, so I can give names to the inferred template arguments.
Is there a way I can do that without the static if?
There was a problem hiding this comment.
It's impossible, or the constraint would fail.
I was more thinking of if the compiler will complain, i.e. when compiling with warnings enabled.
The static if is only for the is() expressions, so I can give names to the inferred template arguments.
Is there a way I can do that without the static if?
I think I understand. You want to be able to access the template parameters declared in the Colour struct? If that's the case, you could add aliases to the struct.
There was a problem hiding this comment.
Will the compiler complain? I'll check..
I guess I could add aliases... I wonder why I didn't do that.
I guess I just always figured I could get them from the type if I wanted to. I'll add aliases, good idea :)
|
Phobos uses 4 spaces for indentation, not tabs. |
|
Jeezus, I'm loving this bikeshed! |
Someone was about to tell you sooner or later, I thought I just could tell you now and be done with it.
|
|
|
Ok, I see. |
TurkeyMan
force-pushed
the
colour
branch
2 times, most recently
from
238b044 to
29d0ac2
Compare
|
Fixed all the nitpicks |
|
|
||
| alias ComponentType = ComponentType_; | ||
| enum string components = components_; | ||
| enum ColorSpace colorSpace = colorSpace_; |
There was a problem hiding this comment.
Do these need to be public? I not, I would recommend adding the underscores to these enums/alias instead of the template parameters.
There was a problem hiding this comment.
I don't think it can hurt for them to be public. People might want to know the colour space, and the component type is definitely interesting. I could probably make components private.
Is there an idiomatic solution to this problem?
There was a problem hiding this comment.
I don't no, I guess it's fine if they're public. It just slightly annoys me that the underscores are needed.
There was a problem hiding this comment.
Yeah, believe me, those underscores REALLY upset me too ;)
I found myself wishing for behaviour something like C++'s initialiser lists, where it intelligently just knows which one I'm talking about based on common sense... >_<
|
A few things.
auto concatRawValues(T)(T[] values...)(isColor!T) {
alias U = typeof(T.r); // example
U[] ret;
foreach(value; values) {
ret ~= value.r; // example
// "
}
return ret;
}Would be nice. In other words, design wise, I'm happy. Just nitpicking. |
|
You know you can comment in particular lines of code on github right? (click the little '+' next to the lines you have issues with), makes it easier to have conversation at the offending line. Why do you think I should namespace the type aliases? Isn't that what module namespacing is for? Design-wise, I'm actually having second thoughts. I thought it would be best to have a single colour template with an associated colour space, but I'm starting to move towards a different colour type for each colour space... although I can't really articulate why. It just kinda feels like my first instinct might have been wrong. |
|
@TurkeyMan Yes I do know. RGB for example seems a little ubiquitous and could cause trouble. It's not a big deal to put the alias's into e.g. a struct. Since we do have things like selective imports and renames. I'm just anticipating problems in library code. Lets be honest here. We mostly will be dealing with RGBA as a ubyte. As long as it be swapped easily to it and pass around the reference without it being that format. There won't be any problem. About the design, even though it is a template I don't think it will cause problems. It would be nice to have all the color spaces be passed around without using templates however. |
|
Yeah, I'm not sold on namespacing either way. I'll worry about that if/when it's reviewed. So, I'm not sure I understood, was that a vote for distinct types for each colour space? |
|
More tweaks, doco improvements. Added nothrow @nogc version of colorFromString(). |
More tweaks to the docs.
|
@jpf91 Any further thoughts on that CI issue?
I did that... CI still fails. |
According to the build results 1 and 2 the workaround fixed the problem for |
|
Seems to be working 👍 The workaround comment should probably include add a link to the bug report: https://issues.dlang.org/show_bug.cgi?id=16477 |
So what's the status on this PR? What are the remaining blockers? |
I don't really know. There seems to be low enthusiasm for inclusion, which is disappointing. This will enable a lot of further multimedia work, and help to make multimedia API's interoperable. |
|
@TurkeyMan I know at this point I'm happy with it, but I don't have write access for the review require check to be completed. |
|
The trouble with doing extensive documentation and examples, is that it may change with API review. |
|
@TurkeyMan @ljubobratovicrelja
@TurkeyMan Please keep We need We will be happy to include it to the Mir Org as separate dub package. |
|
So what how do we proceed here? Two suggested ways:
@TurkeyMan maybe you can start a thread to resolve this status quo on the NG? |
|
@9il Vectorisation of color is something I'm very interested in working on too, and it's going to be kinda hard. The reason I haven't attempted it sooner though, is because I think it will require some API assistance from ndslice (for instance); the looping/iterating logic will need to be able to make some decisions about bundling/grouping sequences of elements, then you can hand-write parallel operator functions which do strips of colours in various formats. The single-element operators are useless for performance. |
|
@wilzbach Ilya makes a good point about iterating on it more, although even if it were accepted into phobos, I would like to think that's the whole point of the experimental namespace. |
|
@TurkeyMan There is nothing wrong with saying vectorization is out of scope at the moment for Phobos. |
|
This was my plan. |
This does not work at practice. The new ndslice API was created because the old one was not created for speed. The same situation will be with color. Futhermore, we would not use Phobos because its modify-test-release cycle is months vs dub's minutes. |
|
@9il that won't be an issue here. The color types are bare bones, there is no form of valid rearranging or removing of variables from what I can see. So all in all, if it goes into Phobos, it'll be a-ok to develop the faster processing algorithms separately. |
If it go to Phobos we would not care about backward updates and will work on dub package separately. I had the same situation with ndslice: it takes to much time to synchronise Phobos and a dub package. In addition, users are disoriented and it is red flag for new comers. |
|
@9il What exactly do you envision you need to alter within the existing code as part of the separate new functions that are designed for batch operations? |
We do not know until we done a large optimisation work using color and ndslice. Phobos does not have iterators and it was a mistake (you can compare |
|
@9il You misunderstand. For batch operations you will need new free-functions. You cannot simply "vectorize" existing ones. The data structures such as RGB, do not have anything other than the components (from what I can see). The only modification I can envision would be needed is padding. The existing functions deal with single color operations. Not batches e.g. an image, there are no easy wins there. So there is nothing to get a big speed boost for. Until ndslice is ready, nothing related to it batch processing wise is going into Phobos and that is ok. Just getting existing image libraries compatible is the goal here. Fast comes later. |
|
Nope. This all words are theory. Practice will show |
|
As I said in #2862, this'll have to go through the Phobos review process so there's no point in keeping this PR open. |
There's talk about adding a std.colour type, here's a starting point for consideration.
Knows about colour space and comprehensive format conversions.
More common colour spaces to come.