News:

SimuTranslator
Make Simutrans speak your language.

The future of merge integration with Standard

Started by jamespetts, April 02, 2012, 10:18:05 PM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

jamespetts

As regulars here might have noticed, development of Simutrans-Experimental has not been as rapid as is customary of late. This has principally been because of the myriad difficulties that I have encountered in merging the latest changes from Standard into Experimental, more fully documented here.

The position is at present that, well over a month after beginning to attempt to merge the changes to the iterators into Experimental, I have still not been able to produce a working version. I eventually managed to get it to compile a few days ago, but it is now plagued by crashes (access violations) that seem to arise in unconnected places and that have no obvious cause (variables appear to be either null or improper addresses in circumstances where they never would have been thus before, and it is very difficult to trace how they came to be assigned those values). I am the first to admit that I am far from the world's most skilled programmer; I have had much assistance from a number of the Standard developers with the merging so far, although I have not had any suggestions as to how to attempt to track down the crashes. I cannot complain: the Standard developers are very kind to assist at all, and answering my requests is no doubt time consuming.

However, in practical terms, this does raise difficult questions. Without assistance, I doubt that I shall be able to get this merge working within anything approaching a reasonable timeframe. Assistance does not at present appear to be forthcoming. There are a growing number of identified bugs and issues in Experimental that need resolving, and a large number of new features or enhancements written by valued contributors that still have not made it into a released version, about which those contributors are quite probably feeling somewhat frustrated, and understandably so. Further, there are some significant development goals for Experimental on which no work has been done because of this: see here for details (and see especially those marked as balance critical). I had very much hoped that I should be able to concentrate on balance critical features this year and produce a workable balanced version of Pak128.Britain. The increasing overhead of merging from Standard is currently blocking the ability to do this.

On the other hand, I am extremely reluctant to give up the practice of merging from Standard, as that means that all the excellent work that goes into Standard may cease to be available in the future for Experimental players. I might be able to cherry-pick some individual enhancements, avoiding structural changes, but the more that the code-bases diverge, the more likely that it is that individual features will rely on structural elements in Standard that have diverged from Experimental and render the new features unmergeable on their own.

The ideal solution would be to have somebody to assist with the task of merging updates from Standard into Experimental. If anyone would like to volunteer to assist, I should be extraordinarily grateful. If nobody is forthcoming for that position, however, I should be grateful for any feedback as to the best way to proceed. Any input would be much appreciated. Also, I am grateful to users of Experimental for their patience so far!
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

ӔO

if the new, as of yet unfinished, code for experimental is added on, will that further complicate merges with standard?


I would be more in favour of adding the balance critical pieces first, but if the merges are bug fixes, then merges would be more preferable.



I think you are doing an excellent job for the manpower you have. If it was me, I would have thrown in the towel by now. ;)
My Sketchup open project sources
various projects rolled up: http://dl.dropbox.com/u/17111233/Roll_up.rar

Colour safe chart:

jamespetts

The 111.1-merge branch does not have any of the new Experimental code in it, although the -devel branch does (but that is merged to a less recent version than 111.1-merge). Adding in the latest changes to the currently broken 111.1-merge branch would no doubt complicate things further.

It is not so much a matter of which ought be done first, as the longer that Experimental goes without being merged, the harder that it becomes to merge it, and it seems as though merging it as it stands now is already beyond what I am able to acheive without assistance.

Incidentally, thank you for your kind comment.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

rsdworker

i think first is do some merges and keep experimetal continue for example merge to some standard like major builds or less

but i am with aeo which is i like that idea

Carl

We really appreciate all your hard work, James! Sorry I can't be more help on the frustrating parts of the code...

Milko

Hello James

I had noticed that you were less present than in the past. Frankly I was worried that you had other problems, I wanted to write even in private, now that I know that the delay is due to problems with code "I breathe a sigh of relief" (it is a saying in Italian, I hope it translates well in English, translates as "narrow escape").
Returning to the theme ... James, the only thing we can do is to thank you for what you've done and are doing. So do not worry if you feel "delay".
I unfortunately can not I can be of help in the code (I dedicate it to the airplanes that I can better :-)).
Unfortunately (or fortunately, depending on how we look) in the standard version came several developers who are bringing new methods of writing, this results in better code, but also increases the need for higher skills, and these skills are not acquired within a short time .
I have the impression that without outside help (some programmers already active in experimental) or a programmer of the standard version that we give active assistance (active = have a look at the code you've written), it will be difficult to merge. To do the merge should also "get into the heads" of those who wrote the original code.
I also believe it may help you to support the management of pakbritainexp, in order to allow you to work only on the code. And in this, I could help, even though my time is very limited.
In the management of pakbritainexp, I'd still be the second choice because I also need to come first in github logics, logics that often, as an observer, I can not understand.

Giuseppe

HeinBloed

Hi James,

I'd like to repeat the thankful attitude expressed by the previous posters - I think you're doing a fantastic job with Experimental. Even though I wasn't active very much on the forum for a long time, I still followed the development passively, therefore getting an idea of your efforts.

For me personally, it doesn't matter so much if the further development of new Experimental features is delayed a bit, however I see your point with implementing balance critical things to allow for the paksets to be worked on properly. After not having played any Simutrans for 2-3 years and only having played standard Pak128 before, I must say that Pak128.Britain Experimental "feels" quite playable though as it is now, so for playability it doesn't seem imperative to me to completely overhaul the balancing (I don't mean to say that I haven't found (m)any little things which could be improved though  ;) ).

I also agree that keeping the Standard code base and Experimental in sync is quite essential, and I also think some of the Experimental features that are already there are vital. So my two cents would be that you should take your time to understand the code changes to Standard and not be too worried about delaying Experimental due to that, as long as there's no helpful hand. Unfortunately, I can't really help you with C(++) code either, particularly when it comes to such syntactic specialties like I saw in the linked thread. If Simutrans was coded in Java, that would be a different story ... However, I was only planning to become active and track down some bugs the past weekend anway (I posted the first yesterday and was going to post two more today, so this thread is like a strange coincidence for me), so I can at least lend a hand about that. Actually, I _did_ consider checking out the code and having a look at it eventually, but that has to wait for now until I have the time.


Good luck with getting more assistance and keep up the great work!
Hein
(previously known as "tttron")

prissi

Actually I think merging would make sense as we discover a lot of errors still in the common codebase. Furthermore, lots of new features would be lost otherwise.

I think the iterators probably also found one or other error hidden before. One, which is very tricky: If you use an iterator in erase or insert, this iterator is invalid afterwards, i.e. you need to dispose it properly.

Also have a short look at the very recent changes of the hashtable_tpl, as it will increase adding, removing and even lookup quite substancially.

And finally, you can cherrypick you changes. Just leave out the iterator would be possible, if you only look at the changes between certain revisions. "svn diff -r 5500:HEAD >changes.diff" will give you only the changes between certain versions.

jamespetts

Thank you all for your input! I shall have to consider all of the suggestions and decide what to do, probably a short time after Easter (during which time I shall be visiting my parents and unable to work on Simutrans).

Prissi - thank you in particular for your more detailed suggestion. I wonder whether this might be the best answer, actually: to revert that part of the merge that removes the old iterators, and to permit the two types of iterators to continue in parallel for the time being, letting the Standard code that has changed and that merges cleanly to use the new iterators, and the code that I have had difficulty in merging to revert to the old iterators. Do you foresee any problems with this approach? Also, does anyone have any suggestions as to how this might best be done in Git? I am very grateful for the help/suggestions.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

prissi

Changing of old to new iterators is simple, you do not have to use the for macros.

Just instead of

xxx_iterator_tpl<bla*> iter(sss);
while(  iter->next()  ) {
  bla *current = iter->get_current();
  ...
}


your write

for(  xxx_tpl<bla*>::interator iter = sss.begin(), end = sss.end();  iter != end;  ++iter  ) {
  bla *current = *iter;
...
}

gives you exact the same functionality. The bonus points are easier erase/insert during iteration. The FOR macros just hide the iterator and you see the object itself.

jamespetts

Hmm, there is one very obscure case in the pathfinder that goes something like this:



slist_iterator_tpl<halthandle_t> halt_iter(haltestelle_t::get_alle_haltestellen());
all_halts_count = (uint16) haltestelle_t::get_alle_haltestellen().get_count();

// create all halts list
if (all_halts_count > 0)
{
all_halts_list = new halthandle_t[all_halts_count];
}

const bool no_walking_connexions = !world->get_settings().get_allow_routing_on_foot() || catg!=warenbauer_t::passagiere->get_catg_index();
const uint32 journey_time_adjustment = (world->get_settings().get_meters_per_tile() * 6u) / 10u;

// Save the halt list in an array first to prevent the list from being modified across steps, causing bugs
for (uint16 i = 0; i < all_halts_count; ++i)
{
halt_iter.next();
all_halts_list[i] = halt_iter.get_current();


This one has caused particular difficulty - I was really not sure how to deal with this, and this one might well have caused the crashes that are currently blighting the branch. How would this case be dealt with, do you think?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

isidoro

Following prissi's indications (without looking at the present code), it should be:

slist_iterator_tpl<halthandle_t>::iterator halt_iter=haltestelle_t::get_alle_haltestellen().begin();
all_halts_count = (uint16) haltestelle_t::get_alle_haltestellen().get_count();

// create all halts list
if (all_halts_count > 0)
{
all_halts_list = new halthandle_t[all_halts_count];
}

const bool no_walking_connexions = !world->get_settings().get_allow_routing_on_foot() || catg!=warenbauer_t::passagiere->get_catg_index();
const uint32 journey_time_adjustment = (world->get_settings().get_meters_per_tile() * 6u) / 10u;

// Save the halt list in an array first to prevent the list from being modified across steps, causing bugs
for (uint16 i = 0; i < all_halts_count; ++i)
{
all_halts_list[i] = *halt_iter;
++halt_iter;
[...]


I would recommend you to have a look at how STL deals with lists, etc.  The interface here is the same.

el_slapper

Just to say that you guys are doing a wonderful job.

prissi

If get_alle _haltestellen by accident is changed during processing, crashes are almost sure to follow.
You should also use the vector_tpl for the all_halts_list; that way just just no care about size and do only append_unique. But in the curretn context, I fail to see the need, as you have eaxtly the same information you already have in the previous list. IF you need this to be a vector, just make haltestelle_t::get_alle_haltestellen() using vectors. Adding/removing to it are not very frequent (at least in standard) thus a vector is also fine.

jamespetts

Hmm - I did not write this particular piece of code: this is part of Knightly's path explorer.

Firstly, Isidoro, did you mean


slist_tpl<halthandle_t>::iterator halt_iter=haltestelle_t::get_alle_haltestellen().begin();


instead of:


slist_iterator_tpl<halthandle_t>::iterator halt_iter=haltestelle_t::get_alle_haltestellen().begin();


? The [anything]_iterator_tpl types were removed. Prissi:

Quote
If get_alle _haltestellen by accident is changed during processing, crashes are almost sure to follow.

As indicated above, I did not write this code, but the following comment gives a clue as to how it was intended to deal with this issue, I think:


// Save the halt list in an array first to prevent the list from being modified across steps, causing bugs


For reference, incidentally, the following is what I attempted to do in the 111.1-merge branch by using the FOR macro. Are there any obvious errors here?


all_halts_count = (uint16) haltestelle_t::get_alle_haltestellen().get_count();

// create all halts list
if (all_halts_count > 0)
{
all_halts_list = new halthandle_t[all_halts_count];
}

const bool no_walking_connexions = !world->get_settings().get_allow_routing_on_foot() || catg!=warenbauer_t::passagiere->get_catg_index();
const uint32 journey_time_adjustment = (world->get_settings().get_meters_per_tile() * 6u) / 10u;

// Save the halt list in an array first to prevent the list from being modified across steps, causing bugs
uint16 i = 0;
FOR(slist_tpl<halthandle_t>, const& halt_iter, haltestelle_t::get_alle_haltestellen())
{
if(++i >= all_halts_count)
{
break;
}
all_halts_list[i] = halt_iter;

// create an empty connexion hash table if the current halt does not already have one
if ( connexion_list[ all_halts_list[i].get_id() ].connexion_table == NULL )
{
connexion_list[ all_halts_list[i].get_id() ].connexion_table = new quickstone_hashtable_tpl<haltestelle_t, haltestelle_t::connexion*>();
}
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

isidoro

Quote from: jamespetts on April 06, 2012, 12:09:22 AM
[...]
Firstly, Isidoro, did you mean

slist_tpl<halthandle_t>::iterator halt_iter=haltestelle_t::get_alle_haltestellen().begin();


instead of:


slist_iterator_tpl<halthandle_t>::iterator halt_iter=haltestelle_t::get_alle_haltestellen().begin();

?
[...]

Sure.  My fault.

kierongreen

all_halts_count[0] is never set, and therefore last halt will not fit into array?

i.e. would having
            if(++i >= all_halts_count)
            {
               break;
            }
at end of while loop be better?

Incidentally I've been programming in C and C++ for 14 years now and actually had to look up what the ++i syntax meant!

Combuijs

Hmm, if somebody in our company uses i++ or ++i other than in a singleton statement like

i++;

he gets flogged company-wide.  ;)

Don't like it, makes for less readable code...

Bob Marley: No woman, no cry

Programmer: No user, no bugs



kierongreen

QuoteDon't like it, makes for less readable code...
Agreed!

            i++;
            if(i >= all_halts_count) {
               break;
            }

is clearer, same number of lines and conforms more to simutrans coding guidelines!

jamespetts

#19
Isidoro,

thank you for confirming that. Replacing your code with the FOR based code that I had used seems to stop the path explorer based crashes on my 111.1-merge branch. It is still unstable, however, in that it freezes with what appears to be an infinite loop somewhere in convoi_t::laden shortly after loading the demo map. The odd thing is about that that I can't find any FOR based structures in the vicinity of the trouble.

Edit: The infinite loop appears to be here:


for (slist_tpl<ware_t>::iterator iter_z = fracht.begin(); iter_z != fracht.end()
            {
                ware_t &tmp = *iter_z;
               
                // New system: only merges if origins are alike.
                // @author: jamespetts

                if(ware.can_merge_with(tmp))
                {
                    tmp.menge += ware.menge;
                    total_freight += ware.menge;
                    ware.menge = 0;
                    break;
                }
            }


The program never exits the outer for loop.

Edit: Never mind - fixed:


uint16 count = 0;

            for (slist_tpl<ware_t>::iterator iter_z = fracht.begin(); iter_z != fracht.end();)
            {
                if(count++ > fracht.get_count())
                {
                    break;
                }
                ware_t &tmp = *iter_z;
               
                // New system: only merges if origins are alike.
                // @author: jamespetts

                if(ware.can_merge_with(tmp))
                {
                    tmp.menge += ware.menge;
                    total_freight += ware.menge;
                    ware.menge = 0;
                    break;
                }
            }
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

isidoro

I think that this should be more correct:

for (slist_tpl<ware_t>::iterator iter_z = fracht.begin(); iter_z != fracht.end(); ++iter_z)
            {
                ware_t &tmp = *iter_z;
               
                // New system: only merges if origins are alike.
                // @author: jamespetts

                if(ware.can_merge_with(tmp))
                {
                    tmp.menge += ware.menge;
                    total_freight += ware.menge;
                    ware.menge = 0;
                    break;
                }
            }


Notice the end of the for statement.

The iterator interface is meant to be capable visiting each element of a container, accessing the element with * operator and visiting the next one with the ++ operator...

jamespetts

Thank you for that. You say that this is more correct - does it have any different effect from the last version, or is it just better formatting?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

dannyman

I have a light education background in C, and I speak scripting, particularly in Python (I'm a Unix SysAdmin) so I doubt I could be much effective help.

... this question may be off-topic but if there's any short answers ... any chance all the Simutrans devs would want to get on the same page wrt revision control and/or bug tracking?  I like to think the Standard guys think the Experimental branch is worth pursuing.  My impression is that git which Experimental uses is geared toward maintaining "change sets" which you might more easily swap between code bases ... might make it possible for Standard to evaluate and incorporate favorable changes from Experimental, like convoy upgrade dialog ... and allow Standard developers to flesh out bigger changes without much worrying about the state of trunk development.

When there's just one or two developers like the old days it is easy to keep track of everything, but it seems there are more people pushing Simutrans forward ... adopting a little more structure might be a worthwhile investment to maintain forward momentum?

That is a discussion for all the Devs to work out, but this fan of the game has been wondering . . .

Thanks,
-danny

kierongreen

Quoteany chance all the Simutrans devs would want to get on the same page wrt revision control and/or bug tracking? I like to think the Standard guys think the Experimental branch is worth pursuing.
I'm not sure how feasible this would be. While I have no problem with other people choosing to code for the experimental branch I prefer to concentrate my effort on standard. Ensuring that patches worked with both branches would be time consuming and would require a fair amount of knowledge of both versions of the code. git may have better support for branches but that is of little help when significant sections of code have been completely reworked.

Forking the project then trying to port code across will sooner or later result in problems such as these - the more experimental diverges from standard the worse it is going to get. Breaking down key experimental features into patches against standard may help if they are independent of each other - that way different people can work on maintaining each section. However I suspect experimental has diverged too much already for this to be a realistic solution.

jamespetts

Whatever people's views on Experimental and Standard, there is much to be said, I think, for using Git instead of SVN for Standard.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

isidoro

Quote from: jamespetts on April 06, 2012, 05:16:32 PM
Thank you for that. You say that this is more correct - does it have any different effect from the last version, or is it just better formatting?

I think that you should increase the iterator the way I did.  Otherwise, you are only comparing with the first element, though n times, with n=the size of the container.


Quote from: dannyman on April 06, 2012, 06:45:12 PM
[...]
... might make it possible for Standard to evaluate and incorporate favorable changes from Experimental, like convoy upgrade dialog ...
[...]

You miss the point there.   I was the author of the first versions of the convoy upgrade dialog.  It was intended for Standard, but never stood a chance.  If I recall it correctly the reason given by prissi was that the dialog was useless since you have the "dismiss all" and "copy convoy" buttons in the line dialog and that the upgrade dialog didn't account either for all cases.  Later, the patch was adopted by James and incorporated into Experimental.  I'm glad that at least it is used somewhere and I don't think it will ever be part of Standard.

I don't think that there would be any technical problems for the developers of Standard to incorporate any feature of experimental if they wanted.

HeinBloed

Quote from: isidoro on April 07, 2012, 02:11:15 AM
[...]
Later, the patch was adopted by James and incorporated into Experimental.  I'm glad that at least it is used somewhere
[...]


So am I, and I never understood the reason to keep it out of Standard. The way you describe "works", but firstly it doesn't deal with existing schedules (next stop) properly, and secondly when you use that approach with a very long (train) line with lots of convoys and lots of depots along the way, you'll either have all the replacement convoys start from a single depot far away from the outer stops of that line (if you just use copy convoy there), or you'll be in a micromanagement clicking hell if you want to spread them out evenly ... and I'm not even mentioning the more sophisticated options like the replace cycle here.

Sorry for more offtopic text, but I felt this had to be said.  :P
(previously known as "tttron")

kierongreen

The patches which had not been incorporated into standard which then formed the basis for experimental were rejected usually on the grounds that they complicated either gameplay or the user interface. That's not to say they are useful or otherwise, it just comes down to the different philosophies used. Detailed reasons for individual patches can be found if you search around the forum.

prissi

I think it would help to keep this on topic on how to merge; for the actual merge it does not matter if you are using git, svn, mercurial, hg, or whatever.

(As a short explaination: Standard started with svn, and it is exported to git. Svn has the avantage of a single number to identify a release; great to ensure a server was built from the same source code as this revision. But git is more visual and less centralized and an ugly hack on windows. But for patching both are similar in the end, as it depends on the programmer to solve conflicts.)

The convoi replacement is a nice tool for advanced players. But a replacing a convoi with a tile to long would easily cause great problems i.e. with 100% load. Also the dialog was simply too large for handheld devices (more than 640*480) Time has passend, and same indicators were added and screen sizes increased also on handheld devices. Standard have to might reconsider it.

But many things from experimental (like the way too complex city placement system for causual players) or the citycars system will not go in. Some (will) come in a different way, like a competitive routing, fixed costs etc. Also experimental has greatly benefitted from the debugging efforts in standard and the network code.

dannyman

An aside on convoy replacement ...

The convoy replacement dialog is **** handy for advanced players and was the first obvious reward I had when I first tried Experimental.  It could use some UX refinement, and it would be nice if I could use it to "clone" a particular convoy for adding capacity to a line.  I assume it is just a matter of time before this functionality in some form makes it in to Standard.

UX refinement is that in my recent playing I thought it was broken the first few times I tried it, until I figured out that I had to de-select the existing vehicle in order to purchase replacement vehicles.  Since the incumbent vehicle is already displayed once at the tippy-top ... maybe instead of "Clear" a "Base on current" button could fill in the "new" convoy based on the old convoy?  (Probably most replacements are whole convoys, but I can see where you might just want to swap out certain cars on a train ..)

Then, there's some confusion because convoy replacement is "bolted on" top of the Standard interface ... once you have "Replace" the "No load" button becomes almost pointless.  And then it is annoying that "Retire" "Withdraw" and "Sell Now" are in the details sub-window.  When this functionality comes back to Standard I'm betting someone clever will re-think the UI a bit.

The other thing that seems odd to me is "retire" vs "withdraw" ... because I think in lines, so "withdraw" means "withdraw vehicle from servicing this line" and "retire" means "withdraw this vehicle from service completely and sell it off."  But I can never think of a good short-text alternative for these buttons, and the tool tips clarify everything just fine.

All this is to say: I love the convoy replacing tool, and wish I could use it for adding a copy of a complex convoy to a line, without having to manually send someone to the depot.

ӔO

Agreed, retire and withdraw are both synonyms of each other, so that can be confusing.

maybe something like:
Sell now -> "Sell now"
Retire -> "Revise" or "Reform"
Withdraw -> "Last run" or "Resign"
My Sketchup open project sources
various projects rolled up: http://dl.dropbox.com/u/17111233/Roll_up.rar

Colour safe chart:

jamespetts

See here for the latest discussions on how the replacer might be implemented in Standard.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

Milko

Hello

I would like to thank all those who helped James in this integration code.

Giuseppe