Sorting vehicles also by name

[minime]

Adding the possibility of sorting the vehicle lists by different criteria was a wonderful idea, and definitely comes in handy at times. However, I've noticed that you can't sort the vehicles by names. This feature would be handy for those of us, who prefer to name the trains to organise them better, or possibly to achive more realism.
It could be that the reason for not implementing is that it's too hard to do, but maybe it's not. If I remember right, the current sorted lists are already cached to reduce the speed impacts - this could be used for this feature as well, since the list would need to be resorted only when a consist is created, deleted or renamed.
What do you guys think?

Cheers,
minime

P.S.: Keep up the great work! What you've done for this game is hard to believe, hats down to you all.

[goalie]

it is good i would use it

[spaceman-spiff]

I would use it, but I'm doing fine with the ones I have, if I need to replace a certain model, then I sort them by speed or cargo, that can do the trick

[Patchman]

This is actually surprisingly difficult, due to the fact that some names are generated automatically and some are custom. Also the numbered trains should be sorted numerically, but custom named ones alphabetically. Note easy.

[minime]

Interesting,
so basically by default the consist has a blank name. And whenever a consist with a blank name is encountered the game automatically refers to it by an automatically generated name.
Hmm, that would be a real PITA to sort, definitely harder than sorting by a single byte like in the case of the consist number. (wild guess, am i right?)
That's too bad, it would have come in handy at times, mostly with passenger trains.
Anyway, thanks for your reply and clearing this up. Good luck with further patching.

Cheers,

minime

[Vloris]

Hello guys,

First, sorry for digging up this old thread, but I would really like this functionallity.
Actually I tried looking at the sorting code in The Patch, and I think I added a working sort-by-name algorithm.
I 'solved' the problem of sorting generated names by ignoring them and putting them after the custom names in a rather random order.

Only the vehicles that have a custom name are being sorted. Attached is the patch file with my additions, this is against svn revision 1579.

There is one thing in my code I'm not sure is acceptable: to add another sorting method, I needed to have a text-id immediately following the already existing sorting methods text-id's.
As it is very discouraged to move text-id's around I was a little hesitant to do so, but the one that needed to be moved, sort_hint, was only used in a single place as far as I could see, so it should be no big problem I hope.

Please let me know what you think of my code, I'm open to all comments!

[DaleStan]

There is one showstoper (ourtext.inc); all the rest are relatively minor quibbles:
In ourtext.inc:
When the comments say "make sure that the order above is unchanged", they do, in fact, mean that. And when they say "ADD NEW ENTRIES DIRECTLY ABOVE THIS LINE ONLY", they mean that too.
Leave sorthint where it was and add sortname at the end, or you'll break some of NewGRF's promises.

In sortlist.asm:
The imul should indeed be replaced with "mov esi, [esi+veh.name]; shl esi, 5". In things like this, which can be called every tick, speed reigns surpreme. It might also be preferable to use a movzx and drop the and, but I'm not quite sure exactly certain of the the desired effect there.

The addr macro is depricated. Although it will still work, there is no longer any reason to use it.

At .sortname and .continue, I would have used ~7 instead of -8.

[Csaboka]

Oops, it seems sortlist.asm needs a minor update. Back when I was writing it, the text IDs were still movable, so it seemed easier to just assume they're always one block. I'll rewrite it to use an array of textIDs, but you'll have to wait for that until tomorrow, the day of my (hopefully) last exam.

[Pookey]

Do we have another Patch Developer Vloris?

[Vloris]

There is one showstoper (ourtext.inc);

Yes, I understand now it should absolutely not be changed.

In sortlist.asm:
The imul should indeed be replaced with "mov esi, [esi+veh.name]; shl esi, 5".

The sorting is actually not called everytick, it depends on your sortvehlist setting. The imul, as well as the -8, where shamelessly copied from somewhere else in the patch code. To find out where the vehicle-name strings where stored I got to patches/gotodeo.asm lines 1590 and further.

The addr macro is depricated. Although it will still work, there is no longer any reason to use it.

Ah, that's the reason the last sort-method hadn't addr() wrapped around it and the other ones did

Do we have another Patch Developer Vloris?

I don't know, maybe I must say, this first attempt to change anything wasn't too hard for me. I havent felt to call the insane asylum the 00readme.txt warns me about!

you'll have to wait for that until tomorrow, the day of my (hopefully) last exam.

Well, good luck with your exam!

[DaleStan]

In sortlist.asm:
The imul should indeed be replaced with "mov esi, [esi+veh.name]; shl esi, 5".

The sorting is actually not called everytick, it depends on your sortvehlist setting. The imul, as well as the -8, where shamelessly copied from somewhere else in the patch code.

Nothing wrong with shameless copying. I've done a fair bit in my day, and I should have said "may be called". The exact line between speed-optimized and size-optimized is often fuzzy and is definitely subject to interpretation. I'm rather biased against (i)mul and (i)div, but I don't have any particularly good reason for that.

I havent felt to call the insane asylum the 00readme.txt warns me about!

You'll get there. Don't worry.

[Csaboka]

I'm rather biased against (i)mul and (i)div, but I don't have any particularly good reason for that.

Well, according to the AMD Athlon Code Optimization Guide I downloaded some time ago, IDIVing with a 32-bit number can take up to 43 clocks. OTOH, the slowest version of IMUL (IMUL reg32, mem32, imm8) takes 18 clocks, the others can take up to 9 clocks. I guess the relative numbers are roughly the same for other CPUs. So here is your reason for not liking (I)MUL and (I)DIV.

In general, you should avoid (I)DIV in frequently executed code whenever you have an alternative. (I)MUL isn't that bad, but it may be worth replacing the slowest form with a MOV and an IMUL reg, imm. And, of course, for multiplying with powers of two, SHL is always preferred to (I)MUL. You can also use some LEA trickery to multiply with 3, 5 and 9.

[m3henry]

I can fit lots of clocks in my computer, but I dont have enough clocks yet.

[Vloris]

I'll rewrite it to use an array of textIDs, but you'll have to wait for that until tomorrow, the day of my (hopefully) last exam.

Or I have to do it myself
Here is the improved version of my patch:
- no more imul's but nice shl's;
- fixed the textID list by using an array 'sorttexts'
- undid the offensive change in ourtexts.inc, now I very nicely only appended one entry to the end.
- removed the deprecated addr() macro. (Maybe for clarity, they can be removed from all the entries in sortfuncs, but that has nothing to do with my patch so I didn't)

*edit*
Ow, damnit... stupid bug, there was another bit of code that relied on the order of the sort texts. I updated the attachment with the fix.

[Csaboka]

Or I have to do it myself

Heh I was going to do it today, but you beat me to it.

[minime]

Considering I've requested this long long time ago, I'd like to thank both of you for implementing this.

Regards,
minime

[wallyweb]

I'm running r1593 but I don't see this yet ... Has it been committed? Do I need to set a switch? I have sortvehlist 10.

[Csaboka]

Please wait until tomorrow - Vloris reminded me to commit it, and I did so today. On the positive side, though, I've modified it a bit so it sorts unnamed vehicles by consist number.

[wallyweb]

Please wait until tomorrow - Vloris reminded me to commit it, and I did so today. On the positive side, though, I've modified it a bit so it sorts unnamed vehicles by consist number.

ok
/me returns to the "picky,picky" bin for something else in need of nitpicking.