Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo duringCGRformation hangs game

SourceForge Headquarters 225 Broadway Suite 1600 San Diego, CA 92101 +1 (858) 422-6466

Erik,
just a follow-up, which I forgot to report from my code review:
In the brown section of the stock market, several certificates can be bought 
in one step. Actually the UI and the action in principle seem support that. 

However even if one buys several shares, only one gets moved into your 
possession, but you still have to pay the full price :-( 
On first glance the issue could stem from the fact that the action only allows 
to store the id of one certificate bought. 
This is the flip-side of the sell shares action, which works for several 
shares, but does not store any ids.

Most likely it is easier for you to fix that as you know those actions much 
better and it might have sideeffects to the changes for 1835 with all those 
special certificates.

Stefan

On Friday 25 June 2010 11:15:35 Erik Vos wrote:
> Stefan,
>
> Thanks for your analysis. Yes, I agree that Rails should be further
> hardened in the aspects you mentioned.
> Anyway, I think I'll remain in bug-fixing-and-hardening mode for some time!
>
> Erik.
>
> -----Original Message-----
> From: Stefan Frey (web.de) [mailto:ste...@we...]
> Sent: Wednesday 23 June 2010 23:35
> To: Development list for Rails: an 18xx game
> Subject: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo
> duringCGRformation hangs game
>
> Erik,
> I agree that Baden is more obvious case/bug and thus easier to fix and
> identify. In that case only the ordering of a stack/list was not observed
> during undoing or transfer. Thus it was for example to the fix of the
> PriceTokenMove, where the stack order is stored and allows the undo to
> preserve the previous ordering.
>
> The case from Aliza's savefile is less obvious, much more tricky, as it has
> to
> combine several conditions to create the problem.
>
> In that case three major ingredients are involved:
> 1) Actions on interchangeable objects
> 2) The first action stores only the object property, whereas the other
> action
> stores the object id
> 3) Undoing the first action and redoing it, chooses a different object
>
> Your suggestion will only work if the first action chooses the object in a
> deterministic way (thus for example from an ArrayList). However the
> portfolio
> class contains some HashMaps and on the first glance I suspect that in some
> cases actions retrieve their objects from those non-deterministic
> collections. In that case this has to be replaced by an ordered collection,
> before the undo mechanism makes sense.
>
> My take is to try to learn from that and try to build several layers of
> protection:
>
> A) Everything in Rails that changes any state should work
> deterministically.
>
> This includes cases that operate on objects, which are interchangable (like
> share certificates, trains, tokens, tiles).
> For non-interchangable object this already a prerequisite for the automated
> testing.
>
> B) Interchangable objects should be identified by an identifiers. Actions
> working on those should use those identifiers instead of the object
> properties. (Thus do not store selling 20%, but which certificates
> exactly).
>
> This also helps in the case of certificates with different share values.
>
> C) Undo of actions should keep the ordering of objects, including the
> ordering
> of interchangable objects.
>
> Another idea might be to store the undo actions in the save files and
> replay
>
> those after the reload.
>
> All this is not done in one step and requires a deeper code review, but I
> suspect that similar problem might be responsible for some of the
> unaccounted
> broken save files. Obeying rule B) might also required breaking the save
> file
> compatibility, so this might be a good task for Rails 2.0?
>
> Stefan
>
> On Wednesday 23 June 2010 21:39:42 Erik Vos wrote:
> > Stefan,
> >
> > Superficially the Baden issue looks similar. The problem there was that a
> > buy from IPO always took the top share. Undo does mix up the share order,
> > but another problem was even worse: moving the Baden shares from
> > Unavailable to IPO reverses the share order!
> >
> > I have fixed the Baden case by giving all certificates a sequence number
> > (index) per company, and checking that a buy from the IPO always takes
> > the one with the lowest index. This makes at least IPO buy actions
> > independent of the list order.
> >
> > >From a first glance to Aliza's problem I'm not sure how far the above
> > > can
> >
> > explain that issue. If I'm right, problems have also been reported about
> > token order errors after Undo. Perhaps we should think about ways to make
> > Undo exactly restore any previous list order, for instance by recording
>
> the
>
> > old list index in the Move.
> >
> > Erik.
> >
> > -----Original Message-----
> > From: Stefan Frey [mailto:ste...@we...]
> > Sent: Sunday 20 June 2010 20:11
> > To: Development list for Rails: an 18xx game
> > Subject: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo during
> > CGRformation hangs game
> >
> > Erik:
> > some follow up on the issue with the broken save file in Aliza's bug
> > report: As I had the hope I could easily make the file at least
>
> reloadable,
>
> > I believe
> > I found a trace to the potential problem:
> >
> > The save file breaks at the following action:
> > Action 378 delft: SellShares: 1 of 10% CPR at $110 apiece
> >
> > But delft should still have the share, as he bought CPR twice before (at
> > actions 238, 244) and sold only once (action 323).
> >
> > The problematic action is the following:
> > Action 330 azure: BuyCertificate: CPR 10% share from Pool price=$110
> >
> > which gets executed on the reload as:
> > 2010-06-20 19:58:33 DEBUG  Action (azure): BuyCertificate: CPR 10% share
> > from
> > Pool price=$110
> > 2010-06-20 19:58:33 DEBUG  >>> Start MoveSet(index=330)
> > 2010-06-20 19:58:33 INFO   azure buys a 10% share of CPR from delft for
> > $110.
> > 2010-06-20 19:58:33 DEBUG  Done: Move PublicCertificate: CPR 10% share
>
> from
>
> > delft to azure
> >
> > Thus Rails assume that the certificate bought stems from delf instead
> > from the
> > Pool! It seems that action does not correctly identify the certificate
> > bought. I checked that this is defined by the certificate ids.
> >
> > Thus I guess, that due to some undo the stack of certificates got mixed,
> > either those in the IPO or in the possession of Delft (similar to the
>
> Baden
>
> > issue in 1835). Thus in the game played the other certificate in the
> > possession of Delft was sold to the Pool in 323 and then on the reload
> > Rails
> >
> > moves the wrong one out of Delft portfolio at action 330 instead getting
> > that
> > out of the Pool.
> >
> > I hope this is of some help, however I do not know how to resolve that
> > issue
> >
> > easily, as I do not know, what is required to keep the ordering of
> > certificates in portfolios "undo-robust". It might be the better solution
> > in
> >
> > the long run to identify the certificates by their properties (thus share
> > percentage and previous owner) and be agnostic about which one exactly to
> > take.
> >
> > At least one should check that the current owner of the certificate is
> > identical to the previous owner implied by the action, but this does not
> > fix
> >
> > the reload problem.
> >
> > I have attached two trimmed save files, one before and one after the
> > error occurs.
> >
> > Stefan
> >
> > On Sunday 20 June 2010 14:54:55 Stefan Frey wrote:
> > > Aliza:
> > > thanks for the savefile.
> > > Most of the issues have to be addressed by Erik, but for the revenue
> > >
> > > calculation I can give some feedback:
> > > > (1)  When I undo a tile lay and do something else, the route is not
> > > > recalculated.  If I don't pay attention and calculate manually, I get
> > > > the run from the previous OR.
> > >
> > > Unfortunately I cannot recreate the issue you report here:
> > > I have the same problem with reloading the file, thus I wonder if the
> >
> > issue
> >
> > > occured anytime during the game or only at the specific time you
> >
> > originally
> >
> > > saved the game?
> > >
> > > Do you remember what was the case (but I fear some questions are too
> > > detailed):
> > >
> > > - You were in the revenue phase already, did undo and went to lay tile
> > > directly (as the company did not have any token)? Or have you done
> > > twice
> >
> > an
> >
> > > undo? Was a special tile lay of a private company involved? Which
>
> company
>
> > > was operating?
> > >
> > > - After undoing, did the revenue run disappear (as it should)?
> > >
> > > - Did the system calculate the revenue run (thus you got the messagebox
> > > with the run), but did not take the new (different) tile lay into
> > > account?
> > >
> > > - Or did simply nothing happened at all, thus the old revenue value was
> > > still entered?
> > >
> > > I was able to confuse the tile/token laying mechansim by undos somehow,
> >
> > but
> >
> > > even in those cases the revenue calculation still worked. But maybe
> > > fixing those errors might fix the (potentially missing) activation of
>
> the
>
> > > revenue calculation as well. I will look into that.
> > >
> > > Stefan
>
> ---------------------------------------------------------------------------
>
> > >--- ThinkGeek and WIRED's GeekDad team up for the Ultimate
> > > GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the
> > > lucky parental unit.  See the prize list and enter to win:
> > > http://p.sf.net/sfu/thinkgeek-promo
> > > _______________________________________________
> > > Rails-devel mailing list
> > > Rai...@li...
> > > https://lists.sourceforge.net/lists/listinfo/rails-devel
>
> ---------------------------------------------------------------------------
>
> >--- ThinkGeek and WIRED's GeekDad team up for the Ultimate
> > GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the
> > lucky parental unit.  See the prize list and enter to win:
> > http://p.sf.net/sfu/thinkgeek-promo
> > _______________________________________________
> > Rails-devel mailing list
> > Rai...@li...
> > https://lists.sourceforge.net/lists/listinfo/rails-devel
>
> ---------------------------------------------------------------------------
>- --
> ThinkGeek and WIRED's GeekDad team up for the Ultimate
> GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the
> lucky parental unit.  See the prize list and enter to win:
> http://p.sf.net/sfu/thinkgeek-promo
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel
>
>
> ---------------------------------------------------------------------------
>--- ThinkGeek and WIRED's GeekDad team up for the Ultimate
> GeekDad Father's Day Giveaway. ONE MASSIVE PRIZE to the
> lucky parental unit.  See the prize list and enter to win:
> http://p.sf.net/sfu/thinkgeek-promo
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel