I have fixed bug 2490 on branch kstevens-3.1 of the 3.1 branch. If you merge all the changes from the kstevens-3.1 branch into 3.1, this bug should get fixed.
Note that before fixing the bug, I first wrote some unit tests that triggered the exception in bug 2490. I then fixed the code, and ran the tests again to confirm my change fixes the bug.
What happens next? Do I mark the bug as fixed in bugzilla? Do I wait until it’s merged into 3.1 before marking it as fixed?
-K
Thus spake fil512:
I have fixed bug 2490 on branch kstevens-3.1 of the 3.1 branch. If you
merge all the changes from the kstevens-3.1 branch into 3.1, this bug
should get fixed.Note that before fixing the bug, I first wrote some unit tests that
triggered the exception in bug 2490. I then fixed the code, and ran the
tests again to confirm my change fixes the bug.What happens next? Do I mark the bug as fixed in bugzilla? Do I wait
until it’s merged into 3.1 before marking it as fixed?
Hadn’t we agreed that fixed bugs would go to RESOLVED-FIXED, and then
once merged would go to VERIFIED-FIXED?
(BTW, putting the closing brace of the previous “if” on the same line
as the “else” doesn’t fit with our coding standard. All the existing
code is this way:
if (…) {
}
else if (…) {
}
else {
}
)
–
J.
Hi Ken,
Had a look at your fix. Unfortunately, it’s not really a bug fix, you’re just masking the reporting of a real bug, which will lead to unpredictable and probably unexpected behavior. In general, if SequenceEncoder.Decoder.nextXXXXX() throws an exception, it’s a bug in something and we want to report it, not hide it.
To answer your question from a previous email
What do you mean by “someone else’s client”?
This bug appears to occur when Player A Connects to the server and starts up a game. Player B connects to the server and presumably synchronizes with Player A so that now player B client has a copy of all the pieces that player A has with the same type and state. [As an aside, this is why the ‘type’ information is saved in the save game as well as the ‘state’ information - It is the only way to ensure that both clients are seeing the same pieces].
Now Player A issues a command to one of his pieces which causes it to change state and a ChangePiece command is create by the A client and sent via the server to the B client to force matching piece in the B client into the same state. When the B client goes to execute the ChangePiece command on the matching piece, the number of ‘states’ (i.e. the number of traits) in the piece in the B client appears to be different to the number of traits that where in the A piece, and a NoMoreElements exception is generated and we know that something bad has happened. [Note, this is a simplistic explanation, the situation that leads up to the bug may be infinitely more complex]
Currently in 3.1, the NoMoreElements exception causes a stack trace, a bug report and a nasty message to the players so that they know something has gone wrong. Hiding this fact is not a good idea. If the 2 clients have become de-synced, anything might happen. If this happens, the playrs need to know that the integrity of their game has been compromised - and hopefully they can reproduce the problem for us
Instead of hiding the message, we actually need to keep generating the bug report, but pump a lot more information into the report so that we can try and work out why the 2 clients got out of sync in the first place. Or they might not be out of sync, but the ChangePiece Command is being generated incorrectly. It may also be a bug in the trait code somewhere. At this point we do not know. What would be nice would be to see a ‘stacktrace’ of the current counter - Basically what you did in those JUnit tests you wrote. I’m thinking each trait should have a toString() method that returns your English expansion of the trait, and use this to generate a PieceTrace dump. Bug Reports generated by traits could then include the PieceTrace.
Some sort of direct dump of the State and Type would be useful as well.
There are plenty of other places where this information would be helpful.
I’m not sure how much of this we would want to put into 3.1, perhaps just some additional state/type info for this particular bug.
Regards,
Brent.
*********** REPLY SEPARATOR ***********
On 1/09/2010 at 7:04 PM fil512 wrote:
I have fixed bug 2490 on branch kstevens-3.1 of the 3.1 branch. If you
merge all the changes from the kstevens-3.1 branch into 3.1, this bug
should get fixed.Note that before fixing the bug, I first wrote some unit tests that
triggered the exception in bug 2490. I then fixed the code, and ran the
tests again to confirm my change fixes the bug.What happens next? Do I mark the bug as fixed in bugzilla? Do I wait
until it’s merged into 3.1 before marking it as fixed?-K
Brent Easton
Analyst/Programmer
University of Western Sydney
Email: b.easton@exemail.com.au
I have moved the bug to RESOLVED-FIXED. I put in the comments the name of the branch where it’s fixed.
I have fixed the braces and committed the code.
Is the VASSAL coding standard published anywhere? I have modified the default Eclipse code formatting config so that I now follow the VASSAL standard for braces. In case there are any other Eclipse users out there, I’ve attached the Eclipse formatting config file. This should probably be under source control…
-K
I guess I misunderstood you when you wrote “The best you can do in Decorator.mergeState() is to detect the problem and fail more gracefully.” By “fail more gracefully” you meant that we should continue to throw a stack trace, but with more information?
Brent,
Thanks for taking the time to explain this to me. I now have a much clearer picture of the context of this bug. I agree with you that we should continue to throw an exception, but include more information in the stack trace. I will back out my change and start over, taking the approach you suggest above.
Joel–I need to change the bug status to REOPENED. But I don’t see REOPENED in the list of choices…? I think we need a REOPENED status for situations like this–when a rookie coder thinks they fix a bug, but someone more experienced reviews the fix and sees an issue with it.
-K
Brent,
How about something simple like the following?
public void mergeState(String newState, String oldState) {
try {
SequenceEncoder.Decoder stNew = new SequenceEncoder.Decoder(newState,
'\t');
String myNewState = stNew.nextToken();
String innerNewState = stNew.nextToken();
SequenceEncoder.Decoder stOld = new SequenceEncoder.Decoder(oldState,
'\t');
String myOldState = stOld.nextToken();
String innerOldState = stOld.nextToken();
if (!myOldState.equals(myNewState)) {
mySetState(myNewState);
}
if (piece instanceof StateMergeable) {
((StateMergeable) piece).mergeState(innerNewState, innerOldState);
}
else {
piece.setState(innerNewState);
}
} catch (NoSuchElementException e) {
String message = "Failed to merge ["+newState+"] with [" +oldState+"]";
throw new NoSuchElementException(message);
}
}Thus spake fil512:
I have fixed the braces and committed the code.
Is the VASSAL coding standard published anywhere? I have modified the
default Eclipse code formatting config so that I now follow the VASSAL
standard for braces. In case there are any other Eclipse users out
there, I’ve attached the Eclipse formatting config file. This should
probably be under source control…
Make a directory for things like this. There are some weird little odds
and ends under dist. Most of them are needed for building the packages
we distribute, but there are a few which don’t belong there—if you
make a ‘misc’ directory, then I’ll move these things there as well.
–
J.
Thus spake fil512:
Is the VASSAL coding standard published anywhere?
Aside from formatting our braces this way, and using 2-space indentation,
there’s not much else to it.
–
J.
Maybe we should post a .project file on SF for eclipse? The 2-space thing
always gets me.
On 3 September 2010 10:04, Joel Uckelman uckelman@nomic.net wrote:
Thus spake fil512:
Is the VASSAL coding standard published anywhere?
Aside from formatting our braces this way, and using 2-space indentation,
there’s not much else to it.–
J.
messages mailing list
messages@vassalengine.org
vassalengine.org/mailman/listinfo/messages
–
Michael Kiefte, Ph.D.
Associate Professor
School of Human Communication Disorders
Dalhousie University
Halifax, Nova Scotia, Canada
tel: +1 902 494 5150
fax: +1 902 494 5151
Maybe we should post a .project file on SF for eclipse?� The 2-space thing always gets me.
- M.
Thus spake fil512:
>Aside from formatting our braces this way, and using 2-space indentation,
> Is the VASSAL coding standard published anywhere?
there's not much else to it.
--
J.
_______________________________________________
messages mailing list
[messages@vassalengine.org](mailto:messages@vassalengine.org)
[vassalengine.org/mailman/listinfo/messages](http://www.vassalengine.org/mailman/listinfo/messages)
Thus spake Michael Kiefte:
Maybe we should post a .project file on SF for eclipse? The 2-space thing
always gets me.
That would likely help new devs trying to get Eclipse set up. Put it
in a new ‘misc/eclipse’ directory and I’ll merge it.
–
J.
Nitpick detail: Most projects I’ve seen use “etc” instead of “misc” for stuff like this.
I will do this. Note I don’t think that the formatter is in the .project file–it’s a workspace configuration. But eclipse provides an importer/exporter for it, so I’ll include the export of the VASSAL code style in the eclipse folder as well.
-K
Thus spake fil512:
Nitpick detail: Most projects I’ve seen use “etc” instead of “misc” for
stuff like this.
Ok, use etc, then.
–
J.
Brent,
Not sure if you saw this post. Still waiting for your feedback before I commit.
Ken
Ken,
That would do as a minimum.
Brent.
*********** REPLY SEPARATOR ***********
On 4/09/2010 at 8:59 AM fil512 wrote:
Brent,
Not sure if you saw this post. Still waiting for your feedback before I
commit.Ken
“fil512” wrote:
Brent,
How about something simple like the following?
Code:
public void mergeState(String newState, String oldState) {
try {
SequenceEncoder.Decoder stNew = new
SequenceEncoder.Decoder(newState,
‘\t’);
String myNewState = stNew.nextToken();
String innerNewState = stNew.nextToken();
SequenceEncoder.Decoder stOld = new
SequenceEncoder.Decoder(oldState,
‘\t’);
String myOldState = stOld.nextToken();
String innerOldState = stOld.nextToken();
if (!myOldState.equals(myNewState)) {
mySetState(myNewState);
}
if (piece instanceof StateMergeable) {
((StateMergeable) piece).mergeState(innerNewState, innerOldState);
}
else {
piece.setState(innerNewState);
}
} catch (NoSuchElementException e) {
String message = “Failed to merge [”+newState+“] with [”
+oldState+“]”;
throw new NoSuchElementException(message);
}
}
Read this topic online here:
Bug 2490 fixed - #13 by fil512
messages mailing list
messages@vassalengine.org
vassalengine.org/mailman/listinfo/messagesNo virus found in this incoming message.
Checked by AVG - www.avg.com
Version: 9.0.851 / Virus Database: 271.1.1/3114 - Release Date: 09/05/10
04:34:00
What is the likelihood that the client module versions are different? Maybe the error message should say something like: “Check that both players are running the same version of the module and VASSAL”.
Do we check programmatically at startup time whether the two clients are running the same version of the module and VASSAL?
Ken,
What is the likelihood that the client module versions are different?
With modules where bug fix releases have been frequent, it is quite common.
Maybe the error message should say something like: “Check that both
players are running the same version of the module and VASSAL”.
Do we check pro grammatically at startup time whether the two clients
are running the same version of the module and VASSAL?
Up until recently, it was not possible to tell. In the latest versions of 3.1, the IP number, Vassal Version, Module Version and Module checksum are now sent to the server on connection and recorded in the User Profile on the server (right-click on a user and select profile).
In 3.2, I am adding more options for users when creating new rooms. One of the options will be to restrict players joining the room to have the same Module version and checksum.
B.
Brent Easton
Analyst/Programmer
University of Western Sydney
Email: b.easton@exemail.com.au
Nice!
When do you think 3.2 will be released?
-Ken
Up until recently, it was not possible to tell. In the latest versions
of 3.1, the IP number, Vassal Version, Module Version and Module
checksum are now sent to the server on connection and recorded in the
User Profile on the server (right-click on a user and select profile).In 3.2, I am adding more options for users when creating new rooms.
One of the options will be to restrict players joining the room to
have the same Module version and checksum.Nice!
When do you think 3.2 will be released?
-Ken
At our current rate of progress, I would say about 5 years
Thus spake “Brent Easton”:
Up until recently, it was not possible to tell. In the latest versions
of 3.1, the IP number, Vassal Version, Module Version and Module
checksum are now sent to the server on connection and recorded in the
User Profile on the server (right-click on a user and select profile).In 3.2, I am adding more options for users when creating new rooms.
One of the options will be to restrict players joining the room to
have the same Module version and checksum.Nice!
When do you think 3.2 will be released?
-Ken
At our current rate of progress, I would say about 5 years
When do you think the things you’re working on will be done?
I’m trying to close out the remaining things I have as fast as I can
now. (Not very fast this week, as I’m in Duesseldorf for a conference
right now, but I should be able to do much more starting Friday.) I
think we’ll have a functional trunk again by the time the weekend is
over, so if that’s holding you up, it won’t be soon.
–
J.
When do you think the things you’re working on will be done?
I’m trying to close out the remaining things I have as fast as I can
now. (Not very fast this week, as I’m in Duesseldorf for a conference
right now, but I should be able to do much more starting Friday.) I
think we’ll have a functional trunk again by the time the weekend is
over, so if that’s holding you up, it won’t be soon.
That will help. I have one broken thing to fix, and a dozen major improvements that have had little or no testing and need substantial user testing, UI polishing, as well as regression testing against existing modules. 3.2 should probably be called v4 due to the major changes included. I am trying to clear my plate of module stuff and get back into it. Personally, I am looking at months of work of finalise all of the changes included.
