Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo during CGR formation hangs game

[Stefan F. <ste...@we...>]

Erik:
as a follow up to your current fixes:
Maybe it is my fault that I am not able to fully explain the issue.
I do NOT claim that there is anything wrong with your fixes, thus
I will try to focus on why I think removing the mixture of referring by type 
and by id protects against several cases of (future) potential problems. 

There are two (main) cases two distinguish: 
A) The interchangable objects (like 10% shares) are stored in an ordered 
collection (like an arraylist).

Then the mixing of actions that refer by ID and by type is a (necessary, but 
not sufficient) condition to the undo/reload-related problems we had.
The additional conditions are that the ordering of the storage collection is 
not observed during undos and that undos are not stored in the save files.

I will give a simple example for that:
START: Player A has two 10% shares of company B&O (order in the portfolio #1 
and #2). The pool is empty.
ACTION: Player A sells a 10% share, Rails selects #1
UNDO: order in the portfoliio is now (#2, #1)
ACTION: Player A sells a 10% share, Rails selects #2
ACTION: Player B buys a 10% share from the Pool, Rails selects #2

On Reload the sequence is:
ACTION: Player A sells a 10% share, Rails selects #1 (sells are stored by 
type)
ACTION: Player B buys a 10% share, Rails select #2 (buys are stored by id), 
this moves the 10% share from A to B, leaving 10% in the pool!

Thus fixing of any of the three conditions (mixing reference type, ordering of 
portfolios, non-saving of undos) above potentially prevents the problem. 

B) The interchangable objects are stored in an un-ordered collection (liek a 
HashSet or HashMap)

Here the problem arises much simpler: Here mixing of id and type referencing 
alone is sufficient to cause a reload problem. Now not even a undo is 
necessary.

Again a simple example:
START: Player A has two 10% shares of company B&O (unordered portfolio #1 and 
#2). The pool is empty.
ACTION: Player A sells a 10% share, Rails selects (randomly) #2
ACTION: Player B buys a 10% share, Rails selects #2

On Reload the sequence can be (in 50% of the cases):
ACTION: Player A sells a 10% share, Rails selects (randomly) #1 (sells are 
stored by type)
ACTION: Player B buys a 10% share, Rails selects #2  (buys are stored by id), 
this moves a 10% share from A to B, leaving 10% in the pool!

This problem does not occur (at least it was not observed so far), but it can 
only be avoided by either not mixing referencing by id and by type or never 
use an unordered collection. 

It is mainly case B) that I want to protects us from, as an unordered 
collection seems a valid choice for interchangable objects.

But I agree that it might be the best to wait for the time, where the downward 
compatibility will be broken anyway.

Stefan



On Saturday 17 July 2010 13:55:35 you wrote:
> Subject: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo during CGR
> formation
> hangs game
> Date: Monday 21 June 2010
> From: Stefan Frey <ste...@we...>
> To: "Development list for Rails: an 18xx game"
> <rai...@li...>
>
> 1) The BuyCertificates action stores certificate by id
> 2) The SellShares action only stores percentages sold
>
> The main issue is mixing addressing interchangeable objects once by ids and
> in
> the other case by type. This can only work, if evey selection by type works
> deterministically (thus never from unordered collections etc.) and the undo
> mechanism strictly restores the previous ordering.
>
> Stefan
>
> [EV] IIRC, in a follow-up you suggested to always use certificate IDs in
> the share buying and selling action.
>
> However, there is a reason behind this difference. Whereas you always buy
> *certificates* (normally one), you don't sell certificates but *shares*.
> The share/certificate distinction makes no difference except in the case
> where you sell half a president's certificate.
> Say player A has a 20% presidency and player B has 2 10% shares. Player A
> can then sell a 10% share - even if he does not own a 10% certificate.
> That's why I think selling can be done in no other way than by selling
> *shares*.
>
> Although certificates are normally bought one by one, the 1830 brown zone
> case requires enabling more than one cert at a time. As you say, the cert
> type is passed around via its ID, but this ID is not actually used when the
> purchase is executed: one (or more) certs are then searched afresh in the
> selling portfolio.
>
> I have not yet fully grasped why you conclude that this difference between
> selling and buying logic did contribute to the Undo-caused save/restore
> problem that has (hopefully) just been fixed. Could it be the possibility
> that a share offered for buying is not the share actually bought (as
> explained above)? If so, that should be easy to fix: just buy the share
> with the ID that has been passed around, and then (in the brown case)
> search for additional shares if requested.
> (It might even have been that way originally; I vaguely remember having
> changed this to simplify the code - that might then have been a misguided
> simplification.)
>
> If there would be a real need (or strong wish) to align buying and selling
> on this aspect, I think we should go for using shares (rather than
> certificates) all the way. But I'm not yet convinced that there is a strong
> case for such a change. In any case, such a change would need careful
> precautions to avoid invalidating all current saved files.
>
> Erik