rails-devel

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

[Erik V. <eri...@xs...>]

I have fixed this problem by ensuring that undoing any ObjectMove will
always restore the original stack order.

This covers most object types; I think only price token moves are not
included as these follow a different paradigm, but in that case stack order
restoration had already been dealt with previously. 

The case in question now works correctly. I have also tested against a
number of longish test cases, and these all work. My testing revealed
another bug, where price tokens moving upwards vanished in the empty upper
left corner of the 1835 stock chart. That bug is now also fixed. 

I'll will have another look at ObjectMoves, because I think the code can be
simplified by providing simpler way to find in which List any Moveable
object is held by its owner. Perhaps price token moves can then also be
rewritten as ObjectMoves.

I'll discuss another point raised below by Stefan in a next post.

Erik.


-----Original Message-----
From: ste...@we... [mailto:ste...@we...] 
Sent: Tuesday 13 July 2010 23:35
To: Erik Vos
Subject: Fwd: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo during CGR
formation hangs game

Fowarding test case for sequenc problem.

----------  Forwarded Message  ----------

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...>

Erik:
Just some follow-up to the issue. I explain it with an example, as it 
might be an instructive case to remember for potential issues with the
undo-function in Rails.

This bug is easily reproducible under simplified conditions. I attach the
save 
file for the example.

There are several steps involved (see example file)
A) Owner of AR (player David, owns 50%) sells a (single) certificate
B) Undo this action
C) Player A sells a (single) certificate again
D) Other player (Alice) buys that share from the pool
E) Save Game: 
Until then everything looks fine: David owns 40%. Alice owns 10%
F) Reload game: 
Now David owns only 30%, Alice owns 10%, the Pool has 10%!

What happened internally: You have to be aware, that certificates are 
internally identified by ids. Assume that David has single certificates 
called AR-1 to AR-3.

A) David sells 10% certificate (assume that this is certificate AR-1)
B) Undo that action (now AR-1 comes to the bottom of his certificates)
C) David sells 10% again, but now this is certificate AR-2
D) Alice buys AR-2 from the pool
E) save only saves actions, which were not undone (thus actions C and D)
F) on the reload for C) AR-1 gets sold to the Pool, but in D) Alice still
buys 
AR-2, but the previous owner is now not the Pool, but David!

The reason is a combination of issues:
1) The BuyCertificates action stores certificate by id
2) The SellShares action only stores percentages sold
3) The Undoing of share moves does not keep the order of certificates

By changing any of the three the problem can be solved.
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

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

[Erik V. <eri...@xs...>]

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

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

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

[Erik V. <eri...@xs...>]

OK, it's getting clear to me now. 

You disregard my observation that the certificate ID specified in a buy
action is NOT used, but that the certificate transferred is selected afresh.
It's not so easy to sort out if this second certificate selection process
will give the same result as the first, or not. Neither is it clear to me if
this detail makes the situation better or worse.

In any case, I'm getting more and more convinced that the use of unique IDs
is the weak spot here, and that BuyCertificate should be modified to specify
shares in a similar way as SellShares does.

Erik.

-----Original Message-----
From: Stefan Frey [mailto:ste...@we...] 
Sent: Sunday 18 July 2010 00:07
To: Development list for Rails: an 18xx game
Subject: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo during
CGRformation hangs game

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



----------------------------------------------------------------------------
--
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
_______________________________________________
Rails-devel mailing list
Rai...@li...
https://lists.sourceforge.net/lists/listinfo/rails-devel

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

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

Sorry: I did not disregard your observation, I forgot to go deeper into the 
details of the buy process:

You are right that you select the actual shares by type from the portfolio of 
the seller.
But how do you define the seller ("from")?
	PublicCertificateI cert = action.getCertificate();
        Portfolio from = cert.getPortfolio();

This is the owner of the certificate used in the action, which was stored by 
the ID.

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!

For the case below, what happens exactly is that you have stored certificate 
#2. The owner is still A. Thus you have wrongly defined from and then you 
select one the shares from the wrong owner.

Thus another fix would have been to use the from field of the action instead 
of the from of certificate, but this does not go to the root of the problem:

The real problem is that in general you have to decide either to use reference 
by ID or reference by type, but not a mixture of both (at least for the 
storage of actions).

I am not really decided, which direction I prefer, in general I would prefer 
IDs, but this is not by a wide margin. You will know better, which method is 
more widespread in Rails so far, thus I follow your decision.

The good thing will be that in both cases the ordering of interchangeable 
objects will play no role anymore. By type the ordering is meaningless, by ID 
there is always an order defined. 

Stefan



On Sunday 18 July 2010 19:26:06 Erik Vos wrote:
> OK, it's getting clear to me now.
>
> You disregard my observation that the certificate ID specified in a buy
> action is NOT used, but that the certificate transferred is selected
> afresh. It's not so easy to sort out if this second certificate selection
> process will give the same result as the first, or not. Neither is it clear
> to me if this detail makes the situation better or worse.
>
> In any case, I'm getting more and more convinced that the use of unique IDs
> is the weak spot here, and that BuyCertificate should be modified to
> specify shares in a similar way as SellShares does.
>
> Erik.
>
> -----Original Message-----
> From: Stefan Frey [mailto:ste...@we...]
> Sent: Sunday 18 July 2010 00:07
> To: Development list for Rails: an 18xx game
> Subject: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo during
> CGRformation hangs game
>
> 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
>
> ---------------------------------------------------------------------------
>- --
> This SF.net email is sponsored by Sprint
> What will you do first with EVO, the first 4G phone?
> Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel
>
>
> ---------------------------------------------------------------------------
>--- This SF.net email is sponsored by Sprint
> What will you do first with EVO, the first 4G phone?
> Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel

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

[Erik V. <eri...@xs...>]

Yes, I agree that finding the seller is the catch.
MoveableHolder requires all portfolios (potential sellers) to have a name,
so perhaps what we need is a Map to link names to portfolios.

Thinking about it: identifying the seller and the share type bought is only
role currently fulfilled by passing the cert (ID), because after extracting
that info the cert ID itself is ignored. So why not pass seller name and
share type explicitly? Detecting the wrong seller is exactly the problem we
face.

One other issue then is to make the old and new version of BuyCertificate
compatible for deserialization, because I would hate to invalidate all my
test cases. But that should be doable.

Erik.

-----Original Message-----
From: Stefan Frey [mailto:ste...@we...] 
Sent: Sunday 18 July 2010 20:04
To: Development list for Rails: an 18xx game
Subject: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo duringCGRformation
hangs game

Sorry: I did not disregard your observation, I forgot to go deeper into the 
details of the buy process:

You are right that you select the actual shares by type from the portfolio
of 
the seller.
But how do you define the seller ("from")?
	PublicCertificateI cert = action.getCertificate();
        Portfolio from = cert.getPortfolio();

This is the owner of the certificate used in the action, which was stored by

the ID.

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!

For the case below, what happens exactly is that you have stored certificate

#2. The owner is still A. Thus you have wrongly defined from and then you 
select one the shares from the wrong owner.

Thus another fix would have been to use the from field of the action instead

of the from of certificate, but this does not go to the root of the problem:

The real problem is that in general you have to decide either to use
reference 
by ID or reference by type, but not a mixture of both (at least for the 
storage of actions).

I am not really decided, which direction I prefer, in general I would prefer

IDs, but this is not by a wide margin. You will know better, which method is

more widespread in Rails so far, thus I follow your decision.

The good thing will be that in both cases the ordering of interchangeable 
objects will play no role anymore. By type the ordering is meaningless, by
ID 
there is always an order defined. 

Stefan



On Sunday 18 July 2010 19:26:06 Erik Vos wrote:
> OK, it's getting clear to me now.
>
> You disregard my observation that the certificate ID specified in a buy
> action is NOT used, but that the certificate transferred is selected
> afresh. It's not so easy to sort out if this second certificate selection
> process will give the same result as the first, or not. Neither is it
clear
> to me if this detail makes the situation better or worse.
>
> In any case, I'm getting more and more convinced that the use of unique
IDs
> is the weak spot here, and that BuyCertificate should be modified to
> specify shares in a similar way as SellShares does.
>
> Erik.
>
> -----Original Message-----
> From: Stefan Frey [mailto:ste...@we...]
> Sent: Sunday 18 July 2010 00:07
> To: Development list for Rails: an 18xx game
> Subject: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo during
> CGRformation hangs game
>
> 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
>
>
---------------------------------------------------------------------------
>- --
> This SF.net email is sponsored by Sprint
> What will you do first with EVO, the first 4G phone?
> Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel
>
>
>
---------------------------------------------------------------------------
>--- This SF.net email is sponsored by Sprint
> What will you do first with EVO, the first 4G phone?
> Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
> _______________________________________________
> Rails-devel mailing list
> Rai...@li...
> https://lists.sourceforge.net/lists/listinfo/rails-devel



----------------------------------------------------------------------------
--
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
_______________________________________________
Rails-devel mailing list
Rai...@li...
https://lists.sourceforge.net/lists/listinfo/rails-devel

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

[Erik V. <eri...@xs...>]

OK, I have bitten the bullet and changed BuyCertificate to use company name
and share size in stead of certificate ID, while maintaining backwards
compatibility. I have tested against various late-game test cases of all
major playable games, and it seems all right now. It would be good if anyone
able to run from the latest code could run their own cases against it.

I did not do much checking of Undo/Redo, except for the specific 1889 case
that started it all. Undo is still known to be imperfect in some cases, and
can better be avoided; it's always safer to restart from a saved game.

Erik.

-----Original Message-----
From: Erik Vos [mailto:eri...@xs...] 
Sent: Sunday 18 July 2010 22:39
To: 'Development list for Rails: an 18xx game'
Subject: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo
duringCGRformationhangs game

Yes, I agree that finding the seller is the catch.
MoveableHolder requires all portfolios (potential sellers) to have a name,
so perhaps what we need is a Map to link names to portfolios.

Thinking about it: identifying the seller and the share type bought is only
role currently fulfilled by passing the cert (ID), because after extracting
that info the cert ID itself is ignored. So why not pass seller name and
share type explicitly? Detecting the wrong seller is exactly the problem we
face.

One other issue then is to make the old and new version of BuyCertificate
compatible for deserialization, because I would hate to invalidate all my
test cases. But that should be doable.

Erik.

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

[Erik V. <eri...@xs...>]

Follow-up: I have added a generic method Util.addToList() to simplify the
object-moving-to-a-list-at-a-given-position code.

While doing this, I realised that Undo does not yet identically restores all
affected lists.
For instance, in Portfolio.addCertificate(), three lists are updated when a
certificate is added: 
- an overall list of certs,
- a map of lists of certs per company,
- a map of lists of certs per type.

To fix that, I'm now passing down an int array, with the location in each
list from which the objects are removed.

Erik.

-----Original Message-----
From: Erik Vos [mailto:eri...@xs...] 
Sent: Saturday 17 July 2010 13:11
To: 'Development list for Rails: an 18xx game'
Subject: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo during
CGRformation hangs game

I have fixed this problem by ensuring that undoing any ObjectMove will
always restore the original stack order.

This covers most object types; I think only price token moves are not
included as these follow a different paradigm, but in that case stack order
restoration had already been dealt with previously. 

The case in question now works correctly. I have also tested against a
number of longish test cases, and these all work. My testing revealed
another bug, where price tokens moving upwards vanished in the empty upper
left corner of the 1835 stock chart. That bug is now also fixed. 

I'll will have another look at ObjectMoves, because I think the code can be
simplified by providing simpler way to find in which List any Moveable
object is held by its owner. Perhaps price token moves can then also be
rewritten as ObjectMoves.

I'll discuss another point raised below by Stefan in a next post.

Erik.


-----Original Message-----
From: ste...@we... [mailto:ste...@we...] 
Sent: Tuesday 13 July 2010 23:35
To: Erik Vos
Subject: Fwd: Re: [Rails-devel] 1856 bugs in Rails 1.3 -- undo during CGR
formation hangs game

Fowarding test case for sequenc problem.

----------  Forwarded Message  ----------

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...>

Erik:
Just some follow-up to the issue. I explain it with an example, as it 
might be an instructive case to remember for potential issues with the
undo-function in Rails.

This bug is easily reproducible under simplified conditions. I attach the
save 
file for the example.

There are several steps involved (see example file)
A) Owner of AR (player David, owns 50%) sells a (single) certificate
B) Undo this action
C) Player A sells a (single) certificate again
D) Other player (Alice) buys that share from the pool
E) Save Game: 
Until then everything looks fine: David owns 40%. Alice owns 10%
F) Reload game: 
Now David owns only 30%, Alice owns 10%, the Pool has 10%!

What happened internally: You have to be aware, that certificates are 
internally identified by ids. Assume that David has single certificates 
called AR-1 to AR-3.

A) David sells 10% certificate (assume that this is certificate AR-1)
B) Undo that action (now AR-1 comes to the bottom of his certificates)
C) David sells 10% again, but now this is certificate AR-2
D) Alice buys AR-2 from the pool
E) save only saves actions, which were not undone (thus actions C and D)
F) on the reload for C) AR-1 gets sold to the Pool, but in D) Alice still
buys 
AR-2, but the previous owner is now not the Pool, but David!

The reason is a combination of issues:
1) The BuyCertificates action stores certificate by id
2) The SellShares action only stores percentages sold
3) The Undoing of share moves does not keep the order of certificates

By changing any of the three the problem can be solved.
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



----------------------------------------------------------------------------
--
This SF.net email is sponsored by Sprint
What will you do first with EVO, the first 4G phone?
Visit sprint.com/first -- http://p.sf.net/sfu/sprint-com-first
_______________________________________________
Rails-devel mailing list
Rai...@li...
https://lists.sourceforge.net/lists/listinfo/rails-devel