News:

Simutrans Tools
Know our tools that can help you to create add-ons, install and customize Simutrans.

XML saves cannot be reloaded

Started by ceeac, July 25, 2021, 05:17:56 AM

Previous topic - Next topic

0 Members and 1 Guest are viewing this topic.

ceeac

Saving demo.sve as XML in the current master branch (commit 51fc2d5) and reloading results in a fatal error message:

FATAL ERROR: loadsave_t::start_tag() - expected
"/roadsign_t", got "koord3d>
"

Ranran(retired)

#1
As far as I can tell, we can also see this symptom with Extended 14.3 #eb71c1e.
This build is around February 27, 2019.

EDIT2:
I have confirmed that this bug can be reproduced at least with Extended 12.2 #6473cf3.
I haven't confirmed the builds before that, but it seems to be an old bug.
I also checked some reproducible patterns on a small map and I can reproduce this just by placing one train staff cabinet on the track.
ひめしという日本人が開発者達の助言を無視して自分好みの機能をextendedに"強引に"実装し、
コードをぐちゃぐちゃにしてメンテナンスを困難にし(とりわけ道路と建物関連)、
挙句にバグを大量に埋め込み、それを知らんぷりして放置し(隠居するなどと言って)別のところに逃げ隠れて自分のフォーク(OTRP)は開発を続けている
その事実と彼の無責任さに日本人プレイヤーは目を向けるべき。らんらんはそれでやる気をなくした(´・ω・`)
他人の振り見て我が振り直せ。ひめしのようにならないために、らんらんが生み出したバグや問題は自分で修正しなくちゃね(´・ω・`)

Ranran(retired)

I checked the contents of sve in xml format.

This seems to be normal data.
   <roadsign_t>
    <obj_t>
     <i8>0</i8>
     <i8>0</i8>
     <i8>0</i8>
    </obj_t>
    <i8>16</i8>
    <i8>16</i8>
    <i8>0</i8>
    <i8>1</i8>
    <i8>10</i8>
    <![CDATA[traffic_light]]>
   </roadsign_t>



Below is the one-train-staff data that is causing the error.
   <roadsign_t>
    <obj_t>
     <i8>0</i8>
     <i8>0</i8>
     <i8>0</i8>
    </obj_t>
    <i8>16</i8>
    <i8>16</i8>
    <i8>0</i8>
    <i8>0</i8>
    <i8>8</i8>
    <![CDATA[one-train-staff]]>
    <koord3d>
     <i16>0</i16>
     <i16>0</i16>
     <i8>0</i8>
    </koord3d>
    <i8>0</i8>
    <bool>false</bool>
    <bool>false</bool>
    <i64>0</i64>
   </roadsign_t>

Since the format is different and there is data such as koord3d, it seems that it can not be read due to an error.

I am not familiar with this, so I hope someone who is familiar with it will solve it. (´・ω・`)
ひめしという日本人が開発者達の助言を無視して自分好みの機能をextendedに"強引に"実装し、
コードをぐちゃぐちゃにしてメンテナンスを困難にし(とりわけ道路と建物関連)、
挙句にバグを大量に埋め込み、それを知らんぷりして放置し(隠居するなどと言って)別のところに逃げ隠れて自分のフォーク(OTRP)は開発を続けている
その事実と彼の無責任さに日本人プレイヤーは目を向けるべき。らんらんはそれでやる気をなくした(´・ω・`)
他人の振り見て我が振り直せ。ひめしのようにならないために、らんらんが生み出したバグや問題は自分で修正しなくちゃね(´・ω・`)

prissi

#3
This looks correct, unless the reading and writing routines are different (i..e. writing koord3d a; a.rdwr(file) and reading  file.rdwr_short(a.x); ...)
Each structure using the structure::rdwr(file) functions will start a new header in xml.

EDIT:
The routine in extended is somwhat broken. There should be a signal_t::rdwr(file), which will then also call roadsing_t::rdwr. Everything else will never work with XML saves.

in signal.h
virtual rdwr(loadsave_ t*file) OVERIDE;


in signal.cc
signal_t::signal_t( loadsave_t *file
{
rdwr(file);
}
...
void signal_t::rdwr(loadsave_t *file)
{
xml_tag_t r( file, "signal_t" );]

roadsing_t::rdwr(file);

if(file->get_extended_version() >= 12)
{
signalbox.rdwr(file);
...


and probably also xml tags for signalbox and other places.

Mariculous

#4
The following seems to be an issue to me:

if(desc && desc->is_signal_type() && file->is_saving())
{
signal_t* sig = (signal_t*)this;
sig->rdwr_signal(file);
}

https://github.com/jamespetts/simutrans-extended/blob/51fc2d595f4129c580ea3d6fc359d026566330f5/obj/roadsign.cc#L812-L816

Signal data is written to the save, but not read.
I'm wondering why this doesn't corrupt the savegame in binary mode?

Edit: I see, the rdwr_signal is called from the signal_t constructor. As there are no headers in binary format, this works but it messes up headers of xml saves.

jamespetts

Interesting. I have not tested with the XML saves as I had no idea that it was even possible for a save routine that works in one file format not to work in another. I do not understand what assumptions that the XML save format makes that are violated by the signal saving/loading code; are these documented anywhere?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

prissi

The xml_tag_t r( file, "roadsign_t" ); write a start <roadsing_t> and on destruction (at the end of the function) a </roadsing_t> field in xml mode. If addintional data is written (like the signalbox) but not read, the structure will have too many fields before closing. Since xml_tag_t is only doing this in xml save mode, it will not afect binary saves, as long as the same number of fields are written.

jamespetts

Quote from: prissi on August 02, 2021, 02:47:06 PM
The xml_tag_t r( file, "roadsign_t" ); write a start <roadsing_t> and on destruction (at the end of the function) a </roadsing_t> field in xml mode. If addintional data is written (like the signalbox) but not read, the structure will have too many fields before closing. Since xml_tag_t is only doing this in xml save mode, it will not afect binary saves, as long as the same number of fields are written.

Thank you - can I clarify what specific functions are those in which the start and end trigger the XML writes? There may be a whole lot of data saved in Extended where this has not been implemented at all.

May I ask whether anyone uses XML saves at all? I am not sure whether it is worth retaining this or whether it would be better being deprecated for Extended.
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

wlindley

The existing "XML" format, with numeric attributes ("<i8>" and "<i32>") rather than meaningful attribute names ("<goods_type>"), seems more trouble than it is worth.  Perhaps later an existing, external library could be used for save files that standard tools can manipulate, maybe in JSON or YAML or... but for now, I would say the "XML" is redundant.

prissi

xml_tag_t r( file, "roadsign_t" ); will write the opening and closing tags.

Using even more elaborate tags would require changing all the line containing code saving data (about 2000 lines). It will increase the compressed size even further by a factor of about 1.2, since there is less repetition.

The xml is good for debugging and editing, and not much else.

Matthew

Quote from: jamespetts on August 02, 2021, 05:14:23 PM
May I ask whether anyone uses XML saves at all? I am not sure whether it is worth retaining this or whether it would be better being deprecated for Extended.

The XML saves have been broken in Extended for as long as I've been modding the game. But based on my experience of other games that save in human-readable formats, I think that they could be very useful for debugging and fixing errors if they were working again, especially since we use nightly builds in production and therefore expect things to break often. Meaningful attribute names would make this much easier, but getting the existing functionality working is the first step.
(Signature being tested) If you enjoy playing Simutrans, then you might also enjoy watching Japan Railway Journal
Available in English and simplified Chinese
如果您喜欢玩Simutrans的话,那么说不定就想看《日本铁路之旅》(英语也有简体中文字幕)。

jamespetts

Quote from: Matthew on August 03, 2021, 04:18:18 PM
The XML saves have been broken in Extended for as long as I've been modding the game. But based on my experience of other games that save in human-readable formats, I think that they could be very useful for debugging and fixing errors if they were working again, especially since we use nightly builds in production and therefore expect things to break often. Meaningful attribute names would make this much easier, but getting the existing functionality working is the first step.

I imagine that meaningful attribute names would be a truly gargantuan amount of work, and would require constant intensive maintenance specifically for this feature with every change to the save game format, unless anyone knows of some very sophisticated technique that will avoid all of this?
Download Simutrans-Extended.

Want to help with development? See here for things to do for coding, and here for information on how to make graphics/objects.

Follow Simutrans-Extended on Facebook.

wlindley

If a save file looked and acted more like a .dat file (perhaps JSON), with a tolerant parser as used when reading datfiles, that should obviate the need for version-specific save file formats, since there would be no binary- or sequence-dependent parts.  It would be more in the UNIX tradition ("Be generous in what you accept, rigorous in what you emit." -- Eric Raymond, The Art of Unix Programming, chapter 11)

Such save files would also not have the "loader crashes because binary stuff from different version is one byte off what it expected" problem; also it would be possible to write tools in other languages to display or manipulate save files. Then imagine the network game being run through JSON messages over websockets to arbitrarily-coded clients... or a user-written scenario engine operating that way...

All that's far, far more work than we all want to do right now, better to concentrate on other stuff first but consider future plans?

p.s., Actually even if datfiles were json, we could eliminate the whole makeobj step and have the game engine read them directly.  This could even be done today with the reader routines, with perhaps the requirement to define a tilesize= parameter in datfiles since that is now specified only in the makeobj invocation.

Mariculous

#13
Seems like a preprocessor job to me:
1. Add an additional string argument to all loadsave_t::rdwr_* function, which accepts the name of the tag.
2. Create an n-arg macro function for each rdwr_* function that passes the name of the variable to the related n+1-arg rdwr_* function.
Then a call to, for example
byte=42;
file->rdwr_byte(byte)

actually results in
byte=42;
file->rdwr_byte(byte,"byte")

Still, such a change is a lot of effort given the huge number of such loadsave_t::rdwr_*functions.

That function could then do whatever it wants with the additional informations. For example to write <byte>42</byte> or <i8 name="byte">42</i8> instead of <i8>42</i8>
That's obviously only meaningful if variable names are meaningful.
Renaming variables in the code can be done step-by-step, but beware that such changes might then change the save format, so this has to be handled properly!
Anyways, using the rdwr_* functions (not the macro) explicitly, pasing the old name can be used to read old saves using the old variable name.

Due to the naming convention, there shouldn't be name clashes as enclosing tags are named after the related class, whilst variables shall not have the "_t" suffix.

Quote from: wlindley on August 03, 2021, 06:35:07 PMIf a save file looked and acted more like a .dat file (perhaps JSON), with a tolerant parser as used when reading datfiles, that should obviate the need for version-specific save file formats, since there would be no binary- or sequence-dependent parts.
Yes, I guess JSON has some merit over XML here, but it cannot replace binary saves! Dat files are meant to be human readable, whilst binary saves are meant to be as compact as possible.
Especially in multiplayer games the size is quite important!

The only way I can think of to get closer to "version independent saves" without extremely blowing up the code size is to generate a structure files for each simutrans version, describing the structure of the binary save.
If the simutrans installation already knows how to read saves of a given version i.e. is already has the structure file, then it can load any saves of that game version.
That means simutrans servers usually won't pass that file around whilst saves uploaded somewhere should always be packed with the structure.

Anyways, I suspect that's a quite major project.

Quote from: wlindley on August 03, 2021, 06:35:07 PMSuch save files would also not have the "loader crashes because binary stuff from different version is one byte off what it expected" problem;
That could as well be achieved by adding a small header to some of the larger objects in the binary save format resulting in a rather small (compared to human readable saves) increase of overall save size.

Quote from: wlindley on August 03, 2021, 06:35:07 PMThen imagine the network game being run through JSON messages over websockets to arbitrarily-coded clients... or a user-written scenario engine operating that way...
This might be interessting, but in the first place I see some opportunities in a nettool backend using web standards.

Quote from: wlindley on August 03, 2021, 06:35:07 PMActually even if datfiles were json, we could eliminate the whole makeobj step and have the game engine read them directly.
I like the idea of JSON dats anyways, as some data in .dat files is hierarchical and I like the idea of loading dat files directly as this could assist in pakset developement, but please let's not get rid dat file copilation as that speeds up game loading times quite much.

Ranran(retired)

The XML save is broken only in the part related to the signal, so it would be desirable if it could be repaired.


  <signal_t>
   <roadsign_t>
    <obj_t>
     <i8>0</i8>
     <i8>0</i8>
     <i8>0</i8>
    </obj_t>
    <i8>16</i8>
    <i8>16</i8>
    <i8>0</i8>
    <i8>0</i8>
    <i8>8</i8>
    <koord3d>
     <i16>0</i16>
     <i16>0</i16>
     <i8>0</i8>
    </koord3d>
    <i8>0</i8>
    <bool>false</bool>
    <bool>false</bool>
    <i64>0</i64>
    <![CDATA[one-train-staff]]>
   </roadsign_t>
  </signal_t>

I suppose it's correct to output and load such a format, but I gave it up after a bit of trial... (´・ω・`)
ひめしという日本人が開発者達の助言を無視して自分好みの機能をextendedに"強引に"実装し、
コードをぐちゃぐちゃにしてメンテナンスを困難にし(とりわけ道路と建物関連)、
挙句にバグを大量に埋め込み、それを知らんぷりして放置し(隠居するなどと言って)別のところに逃げ隠れて自分のフォーク(OTRP)は開発を続けている
その事実と彼の無責任さに日本人プレイヤーは目を向けるべき。らんらんはそれでやる気をなくした(´・ω・`)
他人の振り見て我が振り直せ。ひめしのようにならないために、らんらんが生み出したバグや問題は自分で修正しなくちゃね(´・ω・`)

prissi

I had a closer look at the code. The "INLINE_OBJ_TYPE" hack break this (and any other places that use inheritance upon loading.)

(Also the sln is incompatible with vcpkg, so it takes a long tome to set everything up.)