Patch: yellow signal patch (revisited) for r11704

[wolfc]

I thought I would try some patching on OpenTTD, so I started out by rewriting the yellow signal patch by egladil.

Here it is.

You also need the yelsig2.grf file from egladil. You can find them here.

Note that the patch only includes penalty calculation for NPF. I will try to work it out for YAPF.

PS. Just as in the original patch trains stop on the main menu screen.

[BigBB]

Hi wolfc,

welcome to the forum.

First a basic: don't make patches with git, allwasy in diff style (svn diff > ...).

Second: your patch is the wrong way, with that you can make your working copy to the trunk. But a patch usual work in the other way (make the trunk to a working copy).

Then, you increase the struct Tile in map_type.h with one byte, but this increase the struct with two bytes and for a max map of 2048*2048 would this a waste of 4MiB of memory. That is why there is the struct TileExtended with m7 . But patches which increase the tile size haven't very much chances to get into trunk, especially if this isn't needed.

You also need the yelsig2.grf file from egladil. You can find them here.

I don't think that this will work, since OTTD changed the whole behavior with it's own grfs (for 100, 200 or so revs ago).

Regards,
BigBB

PS: I personally think, if it is a rediff, post it in the original thread and don't make a new own one.

[wolfc]

Hi wolfc,

welcome to the forum.

First a basic: don't make patches with git, allwasy in diff style (svn diff > ...).

I dont' see the harm here. According to the 'Howto apply a patch/diff file' thread a patch is allowed to have leadings dirs.
Just do:

Code: Select all

patch -p1 <yellow-signal-0.1.1.patch

Second: your patch is the wrong way, with that you can make your working copy to the trunk. But a patch usual work in the other way (make the trunk to a working copy).

Doh, my bad. Updated to 0.1.1.

Then, you increase the struct Tile in map_type.h with one byte, but this increase the struct with two bytes and for a max map of 2048*2048 would this a waste of 4MiB of memory. That is why there is the struct TileExtended with m7 . But patches which increase the tile size haven't very much chances to get into trunk, especially if this isn't needed.

Yes, I was already pondering this when I put that piece of code in. Maybe with some smart directives the Tile struct can be made a union with bit fields. Although that needs to be coded/verified against each compiler (even gcc differs with arch/version combinations). It would make the code more readable though. Besides 4MiB on a desktop machine is peanuts (not on the handhelds though).
Anyway my guess is this patch won't make trunk, because first routing needs to be finished, then PBS. Besides I don't want to lose semaphores. So we need new graphics. Anybody wants to volunteer for yellow lever signals?

You also need the yelsig2.grf file from egladil....

I don't think that this will work, since OTTD changed the whole behavior with it's own grfs (for 100, 200 or so revs ago).

Regards,
BigBB

Yes, I noticed so I have forced the yelsig2.grf in the old way and bumped the load_index. It doesn't crash without extra grfs, I still have to try with.

PS: I personally think, if it is a rediff, post it in the original thread and don't make a new own one.

I thought that thread had been declared dead.

[BigBB]

I dont' see the harm here. According to the 'Howto apply a patch/diff file' thread a patch is allowed to have leadings dirs.

I know that, the problem is not for Linux (Unix) users but for Windows users. It's not so easy to use a git patch under Windows and "svn diff > ..." don't make more work .

Yes, I was already pondering this when I put that piece of code in. Maybe with some smart directives the Tile struct can be made a union with bit fields. Although that needs to be coded/verified against each compiler (even gcc differs with arch/version combinations). It would make the code more readable though. Besides 4MiB on a desktop machine is peanuts (not on the handhelds though).

Wrong way: every waste byte must be removed! And if you put your variable into TileExtended and not into Tile you have removed this 4MiBs.
But then you have still (addicted to the amount of rail tiles on your map) up to 4MiBs waste, because these tiles don't need this extra byte. And you can see in e.g. docs/landscape_grid.html that a rail tile have 22 free/unused bits and you should use them before extended the whole map array.

Anyway my guess is this patch won't make trunk, because first routing needs to be finished, [...]

The routing is finished since a long time (something around r3000 ? ). I don't know why people mention this everytime...

Yes, I noticed so I have forced the yelsig2.grf in the old way and bumped the load_index. It doesn't crash without extra grfs, I still have to try with.

Do it the right way, use svn://svn.openttd.org/extra/ottd_grf (IIRC) and do
a) (maybe only for testing) extended the openttdgui stuff
or b) define a new action 05 and include it

and then load these new openttd(w/d).grfs (copied them into the data folder) for your working copy.


Edit: I had a very quick look on your new version:

3 times you have this:

Code: Select all

- SetSignalStates(t, GB(_m[t].m2, 4, 4));
+ // FIXME: what is this?
+ //SetSignalStates(t, GB(_m[t].m2, 4, 4));

in rail_map.h you can read:

Code: Select all

/** These are states in which a signal can be. Currently these are only two, so
 * simple boolean logic will do. But do try to compare to this enum instead of
 * normal boolean evaluation, since that will make future additions easier.
 */

Don't remove (comment out) code which isn' needed especial if the reason is you don't understand how this work (a good search program can help you to find and solve a lot).


Line 481 and 689 into your patch file are unneeded.

Your way how you change GetSignalStates() and the whole save of signal states aren't good. Current it use a single bit because it has only two states * 4 depended on the code. Why you don't do something like this? :

Code: Select all

 static inline uint GetSignalStates(TileIndex tile)
 {
-	return GB(_m[tile].m4, 4, 4);
+	return GB(_me[tile].m7, 0, 8);
 }

So you have two bits, up to 4 different states.

You removed the whole SetSignalStates() and replaced it whith SetSignalStateByTrackdir() which doing nearly the same (and if you follow my last hint very nearly). And if I see it right, this change need a few other changes which aren't needed.

Line 545, a tabulator to much, have a look on cooding style, in line 689 was a similar problem.

Oh, that's really really bad:

Code: Select all

+ SPR_YELSIG_BASE = SPR_NEWGRFS_BASE, // The sprites used for yellow (three-state) signals

Two lines above can you read: /* From where can we start putting NewGRFs? */ why do you do that?

Okay, a very short and fast look.
-BigBB

[wolfc]

I know that, the problem is not for Linux (Unix) users but for Windows users. It's not so easy to use a git patch under Windows and "svn diff > ..." don't make more work .

I would say fix the problem where it is broken: get a proper OS.
Have you tried with cygwin?

Wrong way: every waste byte must be removed! And if you put your variable into TileExtended and not into Tile you have removed this 4MiBs.
But then you have still (addicted to the amount of rail tiles on your map) up to 4MiBs waste, because these tiles don't need this extra byte. And you can see in e.g. docs/landscape_grid.html that a rail tile have 22 free/unused bits and you should use them before extended the whole map array.

A yes, the minimalist approach, I can agree with that. I would still say use a union and bit fields. Let the compiler sort it out. A compiler for embedded devices should go for low memory, a compiler for 64-bit desktop could optimize it for 64-bit alignment.
Note that the documentation is out of date. This seems more in line. At the end of the day I choose a completely non-intrusive approach to make sure stuff keeps working. (Another point for the union )

The routing is finished since a long time (something around r3000 ? ). I don't know why people mention this everytime...

Cool, I thought YAPF was going to be extended to allow for easier PBS integration. I will need to dive into YAPF.

Yes, I noticed so I have forced the yelsig2.grf in the old way and bumped the load_index. It doesn't crash without extra grfs, I still have to try with.

Do it the right way, use svn://svn.openttd.org/extra/ottd_grf (IIRC) and do
a) (maybe only for testing) extended the openttdgui stuff
or b) define a new action 05 and include it

and then load these new openttd(w/d).grfs (copied them into the data folder) for your working copy.

These signals should be a real extension (/ newgrf) not something to embed in the base. What is action 5?
Do you have a good newgrf editor / viewer?

Edit: I had a very quick look on your new version:

3 times you have this:

Code: Select all

- SetSignalStates(t, GB(_m[t].m2, 4, 4));
+ // FIXME: what is this?
+ //SetSignalStates(t, GB(_m[t].m2, 4, 4));

in rail_map.h you can read:

Code: Select all

/** These are states in which a signal can be. Currently these are only two, so
 * simple boolean logic will do. But do try to compare to this enum instead of
 * normal boolean evaluation, since that will make future additions easier.
 */

Don't remove (comment out) code which isn' needed especial if the reason is you don't understand how this work (a good search program can help you to find and solve a lot).


Line 481 and 689 into your patch file are unneeded.

Your way how you change GetSignalStates() and the whole save of signal states aren't good. Current it use a single bit because it has only two states * 4 depended on the code. Why you don't do something like this? :

Code: Select all

 static inline uint GetSignalStates(TileIndex tile)
 {
-	return GB(_m[tile].m4, 4, 4);
+	return GB(_me[tile].m7, 0, 8);
 }

So you have two bits, up to 4 different states.

You removed the whole SetSignalStates() and replaced it whith SetSignalStateByTrackdir() which doing nearly the same (and if you follow my last hint very nearly). And if I see it right, this change need a few other changes which aren't needed.

Like the comment says, you must not rely on the bit logic of the signals. So SetSignalsStates is a no-go.
It all broke down near rail_cmd.cpp@2302 where the bits are supposed to be the same as the 'signal present' bits.
(I want double yellow and divergent signals.)

Line 545, a tabulator to much, have a look on cooding style, in line 689 was a similar problem.

In the first part I used tabs for source code alignment instead of just for indentation. @689 is just something stupid (I should check my vim settings). Nice spotting. Fixed.

Oh, that's really really bad:

Code: Select all

+ SPR_YELSIG_BASE = SPR_NEWGRFS_BASE, // The sprites used for yellow (three-state) signals

Two lines above can you read: /* From where can we start putting NewGRFs? */ why do you do that?

I told you I forced the yelsig2.grf in somehow.
Maybe I should have said: quick & dirty hack.

It should actually become more in the likes of loading a signal grf, signals show up in signal gui. Placing signals will introduce visitor callbacks from vehicles to signal. (And visitor callbacks from signal state processing (signal to signal).)
Hmm, why is signal state part of the map anyway? In the end we can have a lot of different states (4 aspect with repeater, divergent and speed delimiter? Okay, I'm going over the top, but more than 4 states should be possible).

Repeaters will start making sense once speed limits are assigned to track segments.

Okay, a very short and fast look.
-BigBB

Thanks, keep it coming.

I found a bug which prevents a train from leaving a depot when there are only green exit signals available. So 0.2.

[BigBB]

I've never said that I have problems with git and that isn't the point. svn diff is the way we all use here and if you want to write patches you should follow our way to make patches. I'm bored about this discussion, use the forum search and you find several posts/reasons why this is so. I don't answer on this anymore.

A yes, the minimalist approach, I can agree with that. I would still say use a union and bit fields. Let the compiler sort it out. A compiler for embedded devices should go for low memory, a compiler for 64-bit desktop could optimize it for 64-bit alignment.
Note that the documentation is out of date. This seems more in line. At the end of the day I choose a completely non-intrusive approach to make sure stuff keeps working. (Another point for the union )

Nobody says that the current map array is good and there is a brunch called map which based on unions but this is irrelevant on this point. Either you make a own new map array which the devs like first or live with that what we have. And it's absolutly not needed to increase it if you have 26 (I forgot that you remove 4) free bits and only need 8 of it. Btw, a different size of the map array for 64bit machines: don't forget save/load compatiblity . The link you posted is nearly the same as in docs/landscape.html , the most infos you need are in the source code.

These signals should be a real extension (/ newgrf) not something to embed in the base. What is action 5?

The way I wrote is the way how it now work in OTTD/with openttd(w/d).grf. For a first impression have a look into my lamp patch (sig) or better the FS post of my missing shore patch and the official discription.

Do you have a good newgrf editor / viewer?

Yes: vim, kwrite, ... for Linux.

[Rubidium]

I've never said that I have problems with git and that isn't the point. svn diff is the way we all use here and if you want to write patches you should follow our way to make patches. I'm bored about this discussion, use the forum search and you find several posts/reasons why this is so. I don't answer on this anymore.

It's probably a defacto Windows 'users' standard because I myself have no problems with git patches; I even post them myself.

Do not say that git diffs are no diffs and svn diffs are diff because they are both diffs, only their format is different... but any *good* program can apply either of them without any problem.

Furthermore doing "svn diff" on a git branch does not work, so it is not as 'easy' as doing "svn diff". Using a proper tool to apply diffs instead of only a very limited subset of diffs is a much better solution.

[BigBB]

I've never said that I have problems with git and that isn't the point. svn diff is the way we all use here and if you want to write patches you should follow our way to make patches. I'm bored about this discussion, use the forum search and you find several posts/reasons why this is so. I don't answer on this anymore.

It's probably a defacto Windows 'users' standard because I myself have no problems with git patches; I even post them myself.

Do not say that git diffs are no diffs and svn diffs are diff because they are both diffs, only their format is different... but any *good* program can apply either of them without any problem.

Furthermore doing "svn diff" on a git branch does not work, so it is not as 'easy' as doing "svn diff". Using a proper tool to apply diffs instead of only a very limited subset of diffs is a much better solution.

You're right, that is what I tried to say. Of course, Windows users can use git but how.... . The only way is a mingw installation and this is nothing else than a (very complicate to install) Linux shell emulation, a VM. But what is the openTTD philosophy? Have compatiblity for all supported systems and if possible more .... . Or are "we" a TTDP clone for all system exclusive Windows (TTDP is a very good system, but it only support win and I personally think this is its problem ...) ?

And you both are right too, Windows isn't so compatible how other OSes, but it is current the main OS (I don't use it ...) for the most people. And I personally think it's the best way to support the most systems and not the (maybe or not) best one.

Similar one was for 25 years ago with VHS and HI8 for people in my stage of life. The one (HI8) was better, but the other one (VHS) has enforced itself because the most people (systems) had supported it (current is a similar event: BluRay (best) and HD-Disk ( low-cost )).

So decides yourself but don't forget the openTTD philosophy!

-BigBB

[Sacro]

Similar one was for 25 years ago with VHS and HI8 for people in my stage of life. The one (HI8) was better, but the other one (VHS) has enforced itself because the most people (systems) had supported it (current is a similar event: BluRay (best) and HD-Disk ( low-cost )).

Actually I think you mean VHS and BetaMax, Hi8 was a camcorder tape and is still used today.

Also, I will hopefully be hacking this patch apart for the signalling rewrite which I am slowly working my way through!

[DaleStan]

I've never said that I have problems with git and that isn't the point. svn diff is the way we all use here and if you want to write patches you should follow our way to make patches. I'm bored about this discussion, use the forum search and you find several posts/reasons why this is so. I don't answer on this anymore.

It's probably a defacto Windows 'users' standard because I myself have no problems with git patches; I even post them myself.

Well, since BigBB won't answer, I will. Windows users want the output of svn diff because TortoiseMerge refuses to touch patches that are not the output of svn diff. I agree that this is a misfeature, if not an outright bug, but that's the way it is.
Not that anyone who has any business using patches should have any trouble getting a copy of either patch or BuildOTTD, as appropriate.

[Bilbo]

So report the bug to TortoiseMerge authors and hope for it to be fixed ... in the meantime, use commandline patch from cygwin for example :)