Create account / Log in
Bug 4278 - map level global properties of the same name messed up online
map level global properties of the same name messed up online
Status: CLOSED FIXED
Product: VASSAL
Classification: Applications
Component: Player
3.1.18
PC Windows
: high major
: 3.1.20
Assigned To: Brent Easton
http://www.vassalengine.org/forum/vie...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-02-13 15:54 CET by barbanera
Modified: 2012-09-02 11:34 CEST (History)
4 users (show)

See Also:


Attachments
see post of around 10 february 2012 on the forum (128.40 KB, application/octet-stream)
2012-02-13 15:59 CET, barbanera
Details

Note You need to log in before you can comment on or make changes to this bug.
Description barbanera 2012-02-13 15:54:37 CET
See post on the forum and attached mod.
Comment 1 barbanera 2012-02-13 15:59:23 CET
Created attachment 3559 [details]
see post of around 10 february 2012 on the forum
Comment 2 Seth Gilchrist 2012-02-13 16:09:31 CET
See forum thread:
http://www.vassalengine.org/forum/viewtopic.php?f=3&t=4905

Global property changes on zones, maps, and the game module are not correctly transferred through the server.  It appears that ChangePropertyCommandEncoder encodes the original command as a string which consists only of the property name, old value, and new value. Then the decoder (GameModule.decode) goes through each of the registered ChangePropertyCommandEncoders (which consist of every zone, map, and module) until it finds one that successfully sets the property, then terminates.  I may be missing something, but I cannot find any encoding of the MutablePropertyContainer which actually contained the original property - furthermore, my tests suggest that the ChangePropertyCommand will, in fact, be executed on the first MutablePropertyContainer which contains a property with the same name - first zones, then maps, then the module.

Suggested fix: Add getID() method to interface MutablePropertiesContainer, encode with the ChangePropertyCommand, and check for a match upon decoding.  Maps can reference the map name; zones should reference both map name and zone name.

-Seth
Comment 4 Brent Easton 2012-07-20 06:32:00 CEST
> Suggested fix: Add getID() method to interface MutablePropertiesContainer,
> encode with the ChangePropertyCommand, and check for a match upon decoding. 
> Maps can reference the map name; zones should reference both map name and zone
> name.

Seth, I think you have diagnosed the problem correctly, along with the correct
way to approach the solution.

One thing we have to be careful of is to ensure compatibility with existing
logfiles. Best way to do this is probably to implement a new Command prefix in
ChangePropertyCommandEncoder 

protected static final String COMMAND_PREFIX2 = "MutableProperty2\t";

to handle encoding with the correct Id, and leave support for the original
Command prefix in decode().

Had a quick look tonight and there are quite a few implementations of
MutablePropertiesContainer, but its not too bad. Also something strange in that
Map implements MutablePropertiesContainer, but also has a separate
MutablePropertiesContainer defined. 

I have some time to look at it over this weekend unless you want to have a go?

Brent.
Comment 5 Bob Seifert 2012-07-20 10:45:54 CEST
Brent/Seth:

I'm not adept at Java but if there is any way I can help while you guys work on it this weekend, let me know.

-Bob
Comment 6 Brent Easton 2012-07-22 18:42:02 CEST
Brent-3.2@8216 has a fix for this problem.

Same named properties in different property containers where not having the identity of the container encoded in SetPropertyCommand or ChangePropertyCommand and their associated CommandEncoders. 

This meant that all references to the same named property in different containers where handled by the first found reference to that property name.
Comment 7 Joel Uckelman 2012-07-23 14:50:39 CEST
Merged to trunk@8218.
Comment 8 Joel Uckelman 2012-07-25 13:56:10 CEST
Merged to 3.1@8223.