Patches in FS

[Bilbo]

I have question about accepting patches in FS. None of my patches from last few weeks was accepted. I even see no comment about them, so it seems they are just ignored (well, not much patches from other people accepted either ...). I wonder why - Are they bad? Are devs occupied by other things, so they have no time for looking at them? Is trunk in some feature freeze state?

[richk67]

Real people. Real lives. Volunteer work. Go figure.

[MagicBuzz]

I have question about accepting patches in FS. None of my patches from last few weeks was accepted. I even see no comment about them, so it seems they are just ignored (well, not much patches from other people accepted either ...). I wonder why - Are they bad? Are devs occupied by other things, so they have no time for looking at them? Is trunk in some feature freeze state?

Welcome to the frustrated people club

I designed a few patches at the begining of the year, then I stoped because of this : not any of my patches were accepted to go in the trunk, while most of them were just integrated in the MiniIN just after they were released.

I can understand the people working on the trunk can't review all patches everyday, because they have a real life and other things to do.
But what I don't understand is that many changes are done in the trunk everyday, proving they just spend a lot of time on the source.

Noone here is aware about these changes in the trunk (can someone tell me why the industry files change every few hours ? integrating new industry GRF support ? great... but how to test ?) but these change just make the current diff obsoletes every day, so patch designers spend more time updating their patch rather than creating new.

Nevertheless, some patches are just ready to be integrated in the trunk (tested, gamers approved the change, etc.) and many trunk changes just make the game unstable (sometimes it won't compile, sometimes the game will just crash, or anything else, proving the trunk changes are not everythime much tested and bug free).

I don't know how works exactely the official dev team, but I suggest one of the teamate just spend every day a few minutes reviewing the current patches progress, and just decide whatever integrating some patches in the trunk or saying why he won't to the patch man.

By exemple, I know my patch "Group info" can't be added "as this" in the trunk : I reuse some string ids that contains the text I wan't, but they are not designed for the same purpose than mine. I don't know if I have to duplicate these string ids and use my own, of changing the string id code in order to make it more generic.

But I still didn't had any comment about this patch, while a few player just thought it is usefull. Most Bilbo's patches are great, and I think it should be a pain if he just leave because noone in the dev team seems interrested about his patches.
OTTD is "open", so make it "open" : any patch must have a chance to be integrated in the trunk, and at least being reviewed and commented quickly. If there were thousands of patches every day, I understand devs can't review all. But the current rythm is much like 1 or 2 per week. People like Chrissicom just spend enought time to keep an alternate version up to date against the current trunk and many patches, don't tell me the the official devs can't make the same job.

[richk67]

I designed a few patches at the begining of the year, then I stoped because of this : not any of my patches were accepted to go in the trunk, while most of them were just integrated in the MiniIN just after they were released.

I accepted patches quickly into MiniIN because a) generally it didnt matter if it caused problems - it was a small userbase, b) I set no style requirements, c) I would not have to support or maintain the patches.

This last point is the killer. Once a patch is in trunk, it is up to the developers to maintain it. The original developer washes their hands of it - what I call fire and forget.

I can understand the people working on the trunk can't review all patches everyday, because they have a real life and other things to do. But what I don't understand is that many changes are done in the trunk everyday, proving they just spend a lot of time on the source.

Noone here is aware about these changes in the trunk (can someone tell me why the industry files change every few hours ? integrating new industry GRF support ? great... but how to test ?)

Yes, it is a progressive build towards full newindustries support. It is being introduced one step at a time to reduce the impact, and to increase the volume of testing on the code, since even if the actual newindustries are not added, it affects other aspects of normal industries, and it is important to test that these are unaffected.

but these change just make the current diff obsoletes every day, so patch designers spend more time updating their patch rather than creating new.

Many small increments is far easier to develop to than one very large radical restructuring. Hopefully, if patch authors stay in sync with these small changes, then when full newindustries is enabled, their patches will already work with it. etc.

Before MiniIN, I maintained the Integrated Nightly, which at the end was a single patch of 500+Kb. Keeping that in sync was impossible, hence moving to a new build using SVN.

Nevertheless, some patches are just ready to be integrated in the trunk (tested, gamers approved the change, etc.) and many trunk changes just make the game unstable (sometimes it won't compile, sometimes the game will just crash, or anything else, proving the trunk changes are not everythime much tested and bug free).

It is rare for *trunk* to be unstable... in fact, I can only remember a handful of revisions in the last 8000 that left the system unplayable. If a patch doesnt work with trunk because of trunk changes, then it is the patch author's problem to debug his patch to work properly.

I don't know how works exactely the official dev team, but I suggest one of the teamate just spend every day a few minutes reviewing the current patches progress, and just decide whatever integrating some patches in the trunk or saying why he won't to the patch man.

First, it isnt a few minutes. No patch ever is. When integrating patches into MiniIN, I estimated that it took about 1hour per small sized patch, to review the code, understand it, work out if anything important will be affected by it, integrate it, resolve conflicts, build it, test it, test that nothing else obvious has been broken by it, upload it, etc. And that is without the additional things the dev team need to consider: issues of style, is it the best solution, do we have an alternative on its way, do we want to adopt support of this patch, what do we need to know that isnt explained in the code, will it improve OTTD, is it actually wanted, is it clutter, etc.

For example, I created a patch in Feb for 3 new airports. They are seriously impressive, with big performance increases on the existing larger airports (to the point that the City and Metropolitan will become obsolete instantly). But we also had plans for loading airports via newgrf, to get away from hard-coded airports altogether. So that patch, although 90%+ ready (some heli routes need adding), is on permanent hold, and will probably never see the light of day. However, it will appear as a newgrf. So, dev team would have rejected it as something better is planned, is coming soon enough to not have a stop-gap solution.

By exemple, I know my patch "Group info" can't be added "as this" in the trunk : I reuse some string ids that contains the text I wan't, but they are not designed for the same purpose than mine. I don't know if I have to duplicate these string ids and use my own, of changing the string id code in order to make it more generic.

But I still didn't had any comment about this patch, while a few player just thought it is usefull. Most Bilbo's patches are great, and I think it should be a pain if he just leave because noone in the dev team seems interrested about his patches.

We are busy developing other useful things. Would one-way roads have happened as quick if that dev had had to review lots of patches that are just not up to scratch? Which dev do you suggest stops his project to review patches?

OTTD is "open", so make it "open" : any patch must have a chance to be integrated in the trunk,

Patches do, and the current uptake of community generated patches is at a record high IMO.

and at least being reviewed and commented quickly.

You pay our wages, you get that service. Its a volunteer project, and nobody should make such demands of others. Its totally disrespectful. Effectively it is saying "I place no value in the work you are doing... the only important thing is to review MY patch". Keep it real - OTTD is a game. Since it is open - if you dont like the speed of development, take a copy of the code and start your own branch.

If there were thousands of patches every day, I understand devs can't review all. But the current rythym is much like 1 or 2 per week.

So that is 1 developer doing nothing but evaluating patches for 1 or 2 evenings per week.

People like Chrissicom just spend enough time to keep an alternate version up to date against the current trunk and many patches, don't tell me the the official devs can't make the same job.

Precisely - it is a JOB. It is not even necessarily a pleasant job, as patch developers tend to get rather pissed when they get told that the code isnt suitable. They argue, they pester, they nag. Eventually, they meet a killfile, and it doesnt matter how wonderful their new patch is, they have lost credibility.

My biggest hassle on MiniIN was the patch authors who badgered for their patch to be integrated, sometimes when there was already a patch in the code doing the same job. It just wasnt *their* patch.

Sure we could spend time reviewing poor quality code, for functions that dont appeal to all. What projects do you suggest we drop to accommodate this?

[XeryusTC]

It took me 2-3 weeks to let my patch comply with the devs wishes, and even if one wasn't fullfilled it wouldn't go into trunk. The devs have really high standards of patches that will go into trunk, it must comply with coding style, be understandable, make sense etc. If it doesn't meet the devs wishes you have no chance of getting it into trunk.
It took me even about half a week after I met all of the wishes by the devs before getting it into trunk, mainly because I nagged A LOT about it and the devs were quite sick of it (I guess). So there is no quick and dirty way, unless you come up with a really good and small patch, but otherwise it takes ages (even with a 100 line patch).

[Rubidium]

I can understand the people working on the trunk can't review all patches everyday, because they have a real life and other things to do.
But what I don't understand is that many changes are done in the trunk everyday, proving they just spend a lot of time on the source.

Every dev has expertise in a small piece of the code. If you would ask me about the blitter code I would be useless, but ask for example Belugas about the network code and he is as useless as I would be for the blitter code.
Why do you think an issue like FS#1030 is not fixed yet? The person with (some) knowledge about that area is just way too busy in real life to even fix even critical bugs.

Noone here is aware about these changes in the trunk (can someone tell me why the industry files change every few hours ? integrating new industry GRF support ? great... but how to test ?) but these change just make the current diff obsoletes every day, so patch designers spend more time updating their patch rather than creating new.

One person is dedicated to do so.

I don't know how works exactely the official dev team, but I suggest one of the teamate just spend every day a few minutes reviewing the current patches progress, and just decide whatever integrating some patches in the trunk or saying why he won't to the patch man.

A "few" minutes won't cut it. Even small patches takes quite some time to fully understand. Another "problem" with reviewing is that the person reviewing it must understand the system that needs to be changed. So if one of them would review all patches that person would first be busy a few days to weeks to understand the subsystem that it changes to fully understand the impact. Ergo, the person that knows a lot about the particular subsystem has to handle them, but that person might not be available as much as others.
Furthermore a lot of patches I have seen on (especially) the forum are made like: "here, I change this variable and I load the savegame and it still works." Problem with that is that they do not fully understand what they are working with. For example someone might think that removing the "limitation" that the whole map must be bordered by water is changing a few lines. But that person does not realize what (enormous) amount of code depends on the fact that the whole map is bordered by water. A result of this is that virtually all patches need considerable modifications before they can actually be committed to trunk.
Another issue with a lot of patches, "autoslope" as example, is that once we give feedback the author starts complaining about all the strict issues and that the devs are dumb and that they do not want to change the patch in something "worse" when they really do not want to understand our viewpoints. This really gives negative feedback to the developers about trying to get those patches into trunk. Those events often reprioritize "user patches" to be way way down on the list.
The best thing to do is to get a developer really like the idea and then there might be a real future in it, but you should not keep whining every day about the patch because that makes people annoyed which then puts the patch down on the priority list.

People like Chrissicom just spend enought time to keep an alternate version up to date against the current trunk and many patches, don't tell me the the official devs can't make the same job.

Maybe you have not noticed all kinds of issues with the "integrated nightlies". The thing that I've seen to happen in all of them is the destruction of multiplayer games due to desyncs. As far as I am aware (virtually) nobody knows exactly how the multiplayer system works and how to debug desyncs. It might even be safe to assume that only two of the main developers actually successfully found the cause of desyncs.

And finally, a fair share of the most active the developers do their coding at work. Problem is that they cannot test anything at work, so they have to do that when they are at home. This results in the "opinion" that they do not care about the patches at flyspray, but they only got time to test their own patches at home.

[Bilbo]

Noone here is aware about these changes in the trunk (can someone tell me why the industry files change every few hours ? integrating new industry GRF support ? great... but how to test ?)

belugas is working on newidnustries, since this is the last planned feature of 0.6.0

but these change just make the current diff obsoletes every day, so patch designers spend more time updating their patch rather than creating new.

Unless something relevant was completely restructured, updating the patch is usually quite easy if the patch is not very large.

in fact, I can only remember a handful of revisions in the last 8000 that left the system unplayable.

Heh, I remember one version being even uncompilable due to some typo :)

But I still didn't had any comment about this patch, while a few player just thought it is usefull. Most Bilbo's patches are great, and I think it should be a pain if he just leave because noone in the dev team seems interrested about his patches.

Some of my patches (Allow more than 22 opened windows, Configurable limits for console history and scrollback buffer, Vehicle console commands) are completely clientside and didn't break savegame compatibility, so you can just install them on your own and use the features they provide.

But some others have to be in trunk to be of any use:

Allow disabling exclusive rights and giving money - this needs to get to servers, so admins can use it. Not possible to have only in client side.
Extra keyboard shortcuts - besides adding "V" to spawn new viewport and "M" to spawn map window, it enables to react on these keys: , . / ; ' [ ] - = \ (now in trunk it is not possible for any widget to react on them in any way). And having "this patch requires Extra keyboard shortcuts patch to work" in many other patches is probably not the best thing ...
Extra large maps (262144 high, 262144 wide) - again, need to be in trunk to be able to use in multiplayer or to show other people your savegames from large maps (4096x4096 probably will be too much for common MP and they'll be for either LAN or SP only, but sizes like 4096x1024 are well usable unless you connection is way too slow)

First two are very small and simple. Third is also quite simple, basically the trunk can already run maps this large, if there wouldn't be some artificial limitations like 'if (map_x>2048 || map_y>2048) error("Invalid map size");'. Basically all my patch have done is enable selecting the bigger sizes in GUI and saving/loading them from savegame. So IF there would be any error in the rest of the code,

If I knew they won't be accepted, I won't code these three.

We are busy developing other useful things. Would one-way roads have happened as quick if that dev had had to review lots of patches that are just not up to scratch? Which dev do you suggest stops his project to review patches?

Perhaps we won't have one way road, but we will have some other useful things ...

Since it is open - if you dont like the speed of development, take a copy of the code and start your own branch.

You'll end up with network and savegame incompatibility very quickly. Which can be quite disadvantage here. Tho I maintain my own personal fork from patches that don't break savegame or network compatibility :) Perhaps I could stick a diff somewhere on the forums ....

If there were thousands of patches every day, I understand devs can't review all. But the current rythym is much like 1 or 2 per week.

From developing my patches I know how long it takes to compile it and test it. Well ... it ain't 1 or 2 minutes.

My biggest hassle on MiniIN was the patch authors who badgered for their patch to be integrated, sometimes when there was already a patch in the code doing the same job. It just wasnt *their* patch.

I don't mind if someone will do the functionality in a better way. The "Allow more than 22 opened windows" is nice example of this - if someone figures out how to raise the limit of opened windows not to 57 but to some higher value (perhaps unlimited?), I'll gladly toss out my patch to garbage :) But now we have 22 max opened windows and according to Murphy laws, incoming newspaper will kill the most important of the windows you have already opened.

I've done also updates and splitting into smaller parts for few patches from other I'd like to see in trunk, but it seems that it didn't help either.

[Bilbo]

here, I change this variable and I load the savegame and it still works." Problem with that is that they do not fully understand what they are working with. For example someone might think that removing the "limitation" that the whole map must be bordered by water is changing a few lines. But that person does not realize what (enormous) amount of code depends on the fact that the whole map is bordered by water.

Heh, that was me who asked this :) I managed somehow to violate this condition on one 0.5.2 server and the game didn't crash or behave funny when I run train to the border (though the part where the condition was violated was in the X direction, so train looking for "tile+1" will encounter water from other side where the condition is still met)
I was suspecting that it may be a problem, tho I wanted to know how big ... I suspect all trains and roadvehs have no checks whether they are out of map or not :)

I never managed to reproduce this again, even though I tried lot of terraforming near the edge....

And finally, a fair share of the most active the developers do their coding at work. Problem is that they cannot test anything at work, so they have to do that when they are at home. This results in the "opinion" that they do not care about the patches at flyspray, but they only got time to test their own patches at home.

Well, if devs are busy with other things ... then they can hardly look at patches ...

I think I'll fork out my BilboIN ... this will give the patches some testing :)

[skidd13]

Well, if devs are busy with other things ... then they can hardly look at patches ...

I think I'll fork out my BilboIN ... this will give the patches some testing

Instead of creating lot's of IN's it would be cool to have an open VCS with less limited access for patch developers. The biggest problem with such a thing would be the handling of savegame compatibility. The other thing is how to separate the patches for the possible inclusion for trunk. But hereon are some VCS specialised AFAIK.

Just my 2 ct.

[Bilbo]

Hmmm ... I guess I stop making any new patches till I receive some feedback or response on the ones I put there already.

[richk67]

Best way to get noticed (in a positive way), is to find a highly wanted feature, code it well, and the devs will (and do) take notice.

When I started on OTTD, I most wanted to have a go at a new AI, but the support wasnt there. So I had a go at terrains, and Terragenesis was the result. Although I didnt take it on to its current state (multi-threading, new gui), the core terrain algorithm is as I wrote it one evening in Feb06.

If you want to do something WOW, then look at the roadmap and see if there is something there that inspires you. Work on it, take advice, be patient. Good coding of desirable features always gains positive attention. (eg. Road stops was on the wish list, and was accepted into trunk almost as soon as the non-dev team coder said he was happy it was ready.)

For example, a well known, highly desirable feature would be a map rotate. Its complicated in parts, and there are some awkward design decisions (what to do when other angle graphics are not available), but in principal it is doable. As a client-side feature only, it is likely to pass the network conflict problems Rubidium mentioned. And I cannot think of one user who would say "Oh no, I dont want THAT!".

[Bilbo]

When I started on OTTD, I most wanted to have a go at a new AI, but the support wasnt there.

Heh, me too ... I looked at the AI, fixed the airport placment so AI placed them more intelligently and it got accepted quickly. I planned some further fixes, but then I discovered how much the AI is broken, that it is cheating and that there is noAI .... so I decided that would be probably the last patch for current AI from me :)

Some another patch to fix broken tunnel costs was also accepted quite quickly. But no more of my patches was accepted then .... and only patch that received at least some input from one of devs was FS#925

If you want to do something WOW, then look at the roadmap and see if there is something there that inspires you. Work on it, take advice, be patient. Good coding of desirable features always gains positive attention. (eg. Road stops was on the wish list, and was accepted into trunk almost as soon as the non-dev team coder said he was happy it was ready.)

For example, a well known, highly desirable feature would be a map rotate. Its complicated in parts, and there are some awkward design decisions (what to do when other angle graphics are not available), but in principal it is doable. As a client-side feature only, it is likely to pass the network conflict problems Rubidium mentioned. And I cannot think of one user who would say "Oh no, I dont want THAT!".

Well, for some multi-tile buildings it may be kinda impossible to show correctly if the graphics from other sides does not exist (for single tile you can just use the same from all sides :)

And highly desirable? Yet I don't see it on http://wiki.openttd.org/index.php/Requested_features :)

[richk67]

Well, for some multi-tile buildings it may be kinda impossible to show correctly if the graphics from other sides does not exist (for single tile you can just use the same from all sides

Yeah, you sort of need a cooperative artist who can create replacements for all the problem graphics. skidd13 has been a massive help on my airports project.

And highly desirable? Yet I don't see it on http://wiki.openttd.org/index.php/Requested_features

Hmm... true. Maybe its one of those that keeps getting sat on the instant its mentioned because of the need for new graphics. Time to edit the wiki

[Digitalfox]

I have question about accepting patches in FS. None of my patches from last few weeks was accepted. I even see no comment about them, so it seems they are just ignored (well, not much patches from other people accepted either ...). I wonder why - Are they bad? Are devs occupied by other things, so they have no time for looking at them? Is trunk in some feature freeze state?

There is now

r10660 -Codechange: simplified tunnel cost algorithm (bilbo)

r10662 - Add: added 'V' as new shortcut for new viewport (bilbo)
- Add: added 'M' as new shortcut for smallmap (bilbo)
- Add: added '+' and '-' as shortcuts to zoom in the mainview (bilbo)
- Add: added support for other keycodes to be possible used in the future (bilbo)

[Bilbo]

There is now :P

r10660 -Codechange: simplified tunnel cost algorithm (bilbo)

r10662 - Add: added 'V' as new shortcut for new viewport (bilbo)
- Add: added 'M' as new shortcut for smallmap (bilbo)
- Add: added '+' and '-' as shortcuts to zoom in the mainview (bilbo)
- Add: added support for other keycodes to be possible used in the future (bilbo)

Good! I have updated http://wiki.openttd.com/index.php/Hotkeys to reflect the changes

Hmm ... and the others seem to be receiving comments ... :)

Well, I guess I'm going to change some of my patches to adapt to these comments ...

[athanasios]

May I repeat a few words again?
It seems there are patches left behind, because there in not enough time for the developers. It is a pity. We should consider a workable solution for this.
1. More developers with a group (possibly the newer less experienced) assigned to work on new patches.
2. Official support of an In version. Patches can be tested there and above mentioned group will forward those that are considered safe to the more experienced developers and the trunk.

Certainly, if group of developers remains as it is, with the progress of the game we will have more and more dissatisfied patchers, artists and players of course. I suggest that each experienced developer becomes the head of a group of less experienced coders that will assist him and be in a way taught from him. These 'student' developers can come from those who have contributed working patches and so have some more knowledge than others. I hope many will be willing to assist.

[XeryusTC]

I don't think that will work, the devs have a really high standard and using non-dev supporters will bring that standard down.
The MiniIN was also supported by some of the devs, it even has its own SVN branch. No original patch developers wished to support their own patches though and thus the MiniIN got canceled (I hope you know the story). Making another IN will probably have the same faith.

[richk67]

Main reason we stopped the MiniIN was that keeping it in sync with trunk was proving more and more difficult. With so many patches (I think final had 38 patches - although some had become trunk by then), a change in trunk could cause a massive cascade of problems in the MiniIN patches. And then you would get into the how to test problems.

For example, the gui system was modified to use a new improved system. All the trunk guis were updated, but the MiniIN devs then had the problem of rewriting all the guis for all the included patches. A big job.

Eventually, it was just a pain in the butt... just keeping MiniIN vaguely up to date was taking all pleasure away from OTTD coding.

If a new IN appears, the dev/devs involved would have to be really dedicated to keep it in sync. Thats OK for a while, but after a few months, it ceases to be fun, and becomes a chore to be done.

Good luck to anyone else though.

[prissi]

Without whining, I would say that any patch larger than 50 kB will almost never make it to the trunk.

rick67:
If you want to do something WOW, then look at the roadmap and see if there is something there that inspires you. Work on it, take advice, be patient. Good coding of desirable features always gains positive attention. (eg. Road stops was on the wish list, and was accepted into trunk almost as soon as the non-dev team coder said he was happy it was ready.)

Therefore this statement will only lead to frustrated coders. Because, the codebase of OTTD seems more unstable than ever (or develops faster). First, the transition to C++ which almost no patch did survive. Then core functions are slowly transferred to C++. Many functions are splitted, changing the values of global in others. (And the new code is also surprisingly often not very well documented; at least to remarks like "comment every function" patch developer recieved.) Core structures changing often.

How can are little larger patch survive, when a patch from last week cannot survive five days? I can understand the developers, but please think about people who really follow your advices. (And how can a developer verify a patch, when the patch does not apply, because the developer to review had a busy week or two?)

Probably one needs some kind of file changing highscore. Like industry_cmd.cc was changed on 33% of all commitments this week. Then patch developer know: "Ok, better wait."

And for heavy changing stuff, I though there were branches with a purpose???

[DaleStan]

Probably one needs some kind of file changing highscore. Like industry_cmd.cc was changed on 33% of all commits this week. Then patch developer know: "Ok, better wait."

"Past performance is no guarantee of future results". Or something.

But for things like industry*, it's a good idea to ask Belugas what should or shouldn't be touched. Or, better yet, ask him to list what still needs to be done, and then offer to do something that you think should be relatively easy.[0] Then do it. This will get the changes done faster, so you can go back to what you really wanted to do, and with more status than you had before.

[0] Often, different people have different ideas of what constitutes "easy". More precisely, different ideas of how to go about implementing something. The easiest valid solution is one that should be used, and usually the work should be done by whoever came up with that solution.