Use a ForwardRange!dchar instead of dstring for Text objects

[aubade]

I've been experimenting a bit with ranges, and given that Phobos is becoming very fond of substituting Ranges and lazy evaluation wherever possible instead of using actual strings, to save on memory allocations, it seems a sensible thing to go for here too.

[Jebbs]

I'll have to look into this more, though it sounds like a great improvement. I just don't really know much about ranges since I haven't had to use them much myself. I've been really busy with classes lately, but I should have some time pretty soon to dive into this.

[Jebbs]

I hate to be the bearer of (possibly) bad news, but as part of the reduction in the amount of maintenance I have to do for the library I decided it would be a good idea to wrap up Text in C code again. There is a lot of complicated code that I don't understand well, and I would hate to screw it up even when copying and pasting most of it. So for this at the very least I probably won't need to make this change.

[Jebbs]

And, well...it looks like Text will stay in the D end after all.

I was wrapping everything up today and it turns out that when it comes to actually drawing the thing there is no good way to do it without making other parts of the binding ugly. So I have decided to keep it the D end.

That also means that I will be looking into this more, and will probably end up doing it, though to be honest I have never had any issues performance wise with the Text object due to string allocations.

[aubade]

Aw, darn. :( I'm sorry to hear that. Though we'll likely have to store something anyway--I've had problems in my code on occasion with ranges that don't like being stored over time--this might be a bit of a dead end.

[Jebbs]

Oh, it isn't that big of a deal. I just need to make sure I update Text ever iteration of SFML.

And your issue with ranges might be worth bringing up as a bug report. That sounds suspicious.

For a reduction in allocations, I'm sure that I could whip something up that doesn't just do direct copying of strings. Maybe using a dchar[] internally instead of a dstring and copying the contents instead of making a copy of the array.

[Jebbs]

I think that I'm going to give this some thought today as I should mostly have #95 done.

I'll either close this and open up a different issue or we can rework it to continue to use a range instead. Have you by chance found out what might have been the cause of your range issues you mentioned having?

[aubade]

I confess I've been a bit focused on some collision-related code and haven't dug around in anything DSFML-touching for a while, so no. ^^; I can try looking in that direction next though.

[Jebbs]

Yeah, I ended up being busy so I didn't get to finish what I wanted anyway. If you are busy with other things then you don't need to worry about it too much.

I can just mark this for 2.2 and we can figure it out then.

[aubade]

Oh, I think I figured out the problem. I was creating strings inside a Destructor, which is apparently undefined behavior. It worked sometimes but barfed horribly at other times!

[SyntaxColoring]

@aubade Creating strings in a destructor is undefined behavior? Do you have a link to where that's said in the language specification? I think I have some of my own code to go back and fix....

[aubade]

Specifically, it's concatenating strings (or anything that allocates from the heap). As I understand it, it's not part of the language spec, but it's one of the quirks of the current garbage collector.

http://forum.dlang.org/thread/mailman.4996.1421777547.9932.digitalmars-d@puremagic.com?page=1

[Jebbs]

I'll try to take a serious look at this over the weekend, but if I remember correctly everything should be fine.

[Jebbs]

Well, I can say that I am actually working on this little bit at the moment. I still don't know that much about ranges, but I'll be giving this a good hard look. I don't see why I won't merge it though.

I do have a question, though I know it has been a super long ime since you have looked at this code.
what's going on here? You catch the UTFException if there is one, but do nothing with it?

[aubade]

Like the comment mentions; I was v. inexperienced with treating strings as ranges, and I tried returning the traditional "this is invalid unicode" character, but it made things mess up in a way I can't recall.

The actual correct approach, I believe, is to structure a loop like:

import std.encoding;

while (!charrange.empty) {
      dchar iteratorchar = charrange.safeDecode();

      //do your processing based on the inputted character here.
}

Since safeDecode automatically does a popFront.

If it'd apply to the 2.1 stuff you're working on, I can update this pull request, otherwise I'd suggest just looking at this as a base and going from here if you do want to take the stored-range approach.

Personally, I'm coming to believe that the best approach might be to have overloads to take either a string or a range, and if it's anything but a string, to copy its data into our own internal buffer--It'll cost more memory, but it'd give us universal conversion, while not having to re-do any processing if we're given a range with lazy evaluations.

[Jebbs]

It is related to the 2.1 release. I was going to add my own code to it based on the string overload work I did, but feel free to update it if you want. I'll be taking a while to learn more about ranges anyway.

I'm wondering where it saves on the allocations through. It is when we set a new string for the Text to display? What's the difference between using ranges and strings directly in that case?

[aubade]

This code saves on allocations if the user isn't storing their string data in a dchar[], because it's converting the char[] to dchars every time. IF the user's data is already a dchar[], then it's pretty much the same

[Jebbs]

It converts it automatically? Wouldn't you get a compiler error from anything that wasn't in dchar format due to the is(ElementType!T : dchar) in the function constraints?

[aubade]

Since an individual char is implicitly convertible to dchar, it'll take a range of char. Then the decodeHead/safeDecode takes as many characters as necessary off the front of a char range to be able to decode it to a dchar.

[Jebbs]

What about wchars? It's probably not the same case, is it?

[aubade]

Nope, it works for wchar too! (Albeit, with all the various caveats that wchar brings with it. Hissss. we hateses wchar, precious.)

[Jebbs]

Oh, don't get me wrong. I think wstrings are silly too. :P

But if that's the case, then I have a bit of work to do. This solution is much better and more elegant than my own, so I'll be using it in some other places. You are welcome to update things if you like, but the only thing missing is the safe decode, right?

[aubade]

Yeah, safeDecode is the big thing that's wrong with that code.

(There may also be other issues but I'm hardly a master of ranges either)

[Jebbs]

After looking around, I think what I'll actually end up doing is call validate on the range when m_string is set in either setString or the constructor. This will throw an exception there(shouldn't we want one?), and we won't have to do a safe decode and try to catch errors inside updateGeometry.

[aubade]

Honestly I'm not sure! To me personally, it's more intuitive, when dealing with unicode errors, to print an invalid character glyph and move on? But I can also see wanting to get an exception.

[Jebbs]

I'd like to revisit this. Anyway you can clean up this pull request and we can get back into it? There seem to be a lot of commits in here.

[aubade]

Either I did a PR from my git master or this is what happens when you close the branch you PRed from...

I'll investigate reimplementing it though; I think I have a thought for a cleaner way to implement this.

[aubade]

To further specify, here is my thought:

If we actually make Text an OutputRange, a lot of neat stuff becomes available. Phobos' std.format has a template called formattedWrite(), which grants the all the power of printf without allocating any memory itself (though an struct's or class' toString method may do this by itself)

Additionally, I believe that the findCharacterPos method can be more efficiently implemented by just lookign at the internal vertex array rather than manually recomputing the position each time it's called, thus alleviating any internal requirement to store a copy of the original string.

So my proposal is to make the core of Text done with put() and reset() methods, with an additional property flag to save a copy of the string or not. We then have a setString convenience method, which could actually take any data type--and multiple instances thereof. getString() will return the internally stored copy if the SaveCopy flag is true, and throw an Error if not.

This should be completely backwards-compatible, and make Text super-powerful.