-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature: add support for XML-based save and load action #384
base: master
Are you sure you want to change the base?
Conversation
Sometime ago we received a "saved status" file to report some problem, which appeared to be unusable. I concluded that the appearance of the "Save Status" command in the File menu was confusing, because it was not really useful, and asked if we shouldn't move that command to the "Developer" menu. Just today, accidentally, I prepared a local commit that does just that. My question: has the situation changed that much that this status file has become useful for whatever purpose it has been designed, so that I can undo that commit, or should we still consider it as "experimental", so that it can better be placed under "Developer", as I proposed? |
WOuld move it to developer. |
The "saved Status" file is independent from this PR. The new format has the main benefit that it can be read and understood by a human without any technical help. For now the main save game format is the Java Serialization format. |
Yes, please, at least for now. Or another new menu (XML?) I'm not yet convinced that a complete replacement is a good idea. But we will see how it works out. Do I understand correctly, that you are foreseeing both versions to be supported options for some time? A few questions:
I'm an old-timer, so my vision on XML may be highly outdated, but I remember parsing and creating XML files to be very slow in comparison with binary files. |
Yes my plan is to support both options (XML and Java Serialization) for now. This is necessary for two reasons:
Yes XML files are larger compared to Java Serialization files. The appended test XML file is ~15x as large as the Java Serialization file (
The loading and saving time of XML should be slower compared to Java Serialization but I haven't measured it. The feeling is still smooth. If we say for example that save file using Java Serialization takes ca. 2 seconds to load then I would expect and XML file to take around 2.5 seconds. But again I haven't measured it, it just feels "fast".
I don't expect this to be a problem. I believe we can "shorten" the files so that they are small enough to fit. I actually believe that the current file size of our XML save files already fit.
That is true, but I don't think that the size and performance scales we are facing here are a reason for us to worry. |
I cannot download your XML file, but got a view on screen, and it's enormous indeed, with at least 15000 lines IIRC. Hard to find anything specific there. We might still need some tool for that. At least I would strongly suggest to add a sequence number as an attribute to each action, as is currently shown in the logs and in ListAndFixSavedFiles. Using attributes in stead of inner tags could also help to get an overview more easily, and save space, or would that complicate the code too much? In the code, I'm getting the impression that you use a different XML composer/parser than the one we use to parse the game configuration. I think ultimately we should align that. Is the new one easier to use or otherwise a better one? |
Yes the longer a game gets the more actions are contained in the XML file. This can make it challenging to find a specific action. A possible solution may be to use XPath to find a specific element inside the XML file.
Serialization public void saveGame(File file) throws IOException {
log.info("Saving to {}", file.getAbsoluteFile());
final XStream xStream = new XStream();
try (OutputStream outputStream = Files.newOutputStream(file.toPath())) {
xStream.toXML(gameIOData, outputStream);
}
log.debug("File save successful");
} Deserialization public void loadGameData(File gameFile) throws Exception {
log.info("Loading game from file {}", gameFile.getCanonicalPath());
final XStream xStream = new XStream();
this.gameIOData = (GameIOData) xStream.fromXML(gameFile);
} To load the game information I believe you have used built in functionality of Java right? As you can see from the code snippets the current implementation is quite simple. We can extend it in any way later on. For example we can change element names or change inner tags to attributes. Such customization either requires the addition of annotations to our Java classes or additional converter classes (for more details see here for annotations and here for converters). At this point I wouldn't define our XML save games as final. |
See class Your XML parser looks a lot leaner, but I can't tell if it is sufficient.
We'll see. Another option could be to keep both methods in existence, defining actual usage by some game or configuration parameter. Many players won't bother about the saved file format, and may be rather interested in efficiency or storage space. Rails could then easily convert binary to XML files (and vice versa) for debugging, testing and fixing problems. To me, that sounds like an ideal end result. I'm particularly worried about the duration of the regression test. I use that early and often, and testing the ever-growing list of saved test files already is the slowest part of that. |
I would agree -- I personally don't see the value is changing the file format. I'm highly concerned about the file size and reading and writing times especially if we are looking at ~15x in size increases. All of my games are in real-time over the net and going from fetching a 50k file to 1mb and then having to parse it seems like show stopper right there. I personally don't see the need to change the file format. Poking around in the file is something I would only expect a developer to do and we have tools for that (and are quite handy and easy to use and I have used them quite a bit when I was fixing game file bugs last year). I think trying to maintain two file formats in parallel is going to be a mess and would highly suggest that we don't do that, even for development purposes. But if the team feels strongly that being able to easily poke around in the file warrants a file format change then I have another suggestion -- instead of using XML, I'd suggest we use json. I think there are several reasons json would be superior to XML -- it can be much smaller than XML (although certainly not as small as serialization) and quite easy to read (in fact even easier IMO). And in fact, given my concern above about size and parsing time, you can even do things like "extract" the schema out and store the file in a binary format and this binary saving can be a developer flag so for development it is plain txt json AND because the format isn't really changing between txt json and binary json we only have one chunk of code to maintain and bug fix. And with libraries like Jackson it's pretty easy to work with (although XStreams does also have a json read/write although technically it's XML to json as it still deals with XML internally, or at least it did last I looked, about 3 years ago). |
enhancement: add XmlConverter standalone class enhancement: use HashMap instead of UnmodifiableMap for Serialization purposes
I agree the file size increased, but as I mentioned this is simply the "first throw". There are still options to decrease the file size. While we can't get the XML files as small as the current Java Serialization files we should be able to reduce them by half. About the time aspect, i.e. the time it takes to load a game: Executed Tests: 185 tests Java Serialization
XML
Please correct me if my experiment is somehow off, but my results suggest that there is no significant difference in the deserialization time between Java Serialization and XML.
That is exactly the reason why I opened this PR. I am a developer and to reasonable work on the codebase I need to be able to understand how my changes reflect on the save games. In addition quite a number of changes somehow result in changes to the save game format. With Java Serialization this is simply a nightmare to "fix" the save games in my opinion. When working on the Rails code I stumbled quite often over the point where my change was basically done but most of the save game files broke. I do not feel like I have the necessary tools to fix them afterwards. XML makes this much simpler.
I agree. If we decide to add XML serialization it is absolutely necessary to support both formats for a time, but after a certain time I would like to see one of the formats scraped. The problem with supporting both formats is that it needs additional maintenance and makes changes to the save game classes (i.e. all implementations to
Changing from XML to JSON is a one-liner with XStream. About XStream vs. Jackson: I would suggest to give this PR a chance. (of course after I have moved it to the |
I think I found another solution for the file size issue with XML. For your interest: I have also taken a look at how large the Java Serialization game save file becomes after zipping: the file was |
Please create a mechanismn for the XML/JSON format to validate a savegame against tampering :) |
It isn't exactly as straight forward as that. It seems like it is but in reality there are a handful of edge cases that you have to deal and it doesn't really output json as most people would expect in many of those cases.
Not sure what is meant here, specifically "see how using JSON changes the save game files and whether it helps in size and readability"? json is certainly much easier to read and is less verbose (and this comes from a guy that has repeatability rejected json in the past but have sense been lead to the light ;)
So there are multiple approaches to using XStream, implicit mappings, explicit mappings, converter classes and approaches like the one here. That is also the order that one should attempt to implement XStream. Implicit mappings are the easiest, require no code at the "bean" level but are also the most constraining. Explicit is more flexible but requires code at the bean level and is also not very friendly with backward compatibilities. Converter classes give you (great) backwards compatibility but at the expense of addition code (but tends to be pretty boilerplate) and also keeps your serde logic out of your beans. The approach in this PR is not a recommended approach as there is no clean separation between the serde logic and the business logic in addition to working with the ObjectStreams directly. If we continue with the use of XStreams I would highly recommend this PR is reworked to use converter classes as currently there is just too much serde logic embedded into our beans (I would reject it in it's current state, sorry @madoar as I do realize this was a large chunk of work for you). We could try mappings but think it will just end up a (serious) pain in the butt as we have enough churn with our objects. Regarding XStream vs Jackson: Honestly my experience with Jackson (or even GSON) is extremely light in context to my XStreams experience. But probably worth a library compare though before we spend any more work on fix this PR though. To the above two points (both reworking this PR and reviewing Jackson/GSON) I'll wait before further review of this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments on converter classes and Jackson/GSON review
I think this is one of the benefits of serialization too -- makes it harder for someone to mess with :) Not impossible of course. Four basic approaches here:
By "sign" it could be something as simple as a hash of the data with a sign or something as complex as a public/private key scenario. Personally just hashing is probably ok. The problem with any of these is that given we are an open source project, unless we as developers create a tool outside of the repo and keep access to it restricted, it would be hard to truly secure the file. So with that said, a best effort that also factors in the whole reason for this PR (ie developer "access" to the file), I'd suggest the second (if we choose something like binary json) and the first if not. |
I have done some tests using a combination of JSON and
I only wanted to say that it is not my goal to evaluate everything in this PR. This is meant to be a PoC or a "first throw", which is also the reason why I am happy to add the buttons to generate and read the new serialization format to the In my opinion most serialization frameworks can handle both JSON and XML (and even other formats like In my opinion whether JSON is easier to read and less verbose that another format is often subjective and highly dependent on the use case. I agree that JSON is in many cases better. For our use case I think XML is a good fit because it represents our inheritance class/action model quite intuitively: every class/action is represented by a tag. In JSON you need to encode the used class differently, often through additional fields inside the structs.
I fully agree with you. The issue is the code does not follow that paradigm currently, this is also why I want to use a different format that is more flexible compared to the current Java Serialization. The current classes contain quite a lot of the serialization logic inside At this point in time I am not willing to rewrite the whole data model and introduce converters or change change the data model classes to better fit recommended coding patterns. The reason being that I do not want to invest many hours into a function which is not even clear whether the community wants it at all. At the moment I want to provide a simple and (implementation wise) fast solution which can be used to evaluate the feasibility of an alternative format to Java Serialization for Rails. If the result of that evaluation is
See my comments above. |
This topic is independent of this PR right? In my opinion all mentioned approaches have the open issue that you need to trust the other players in your game. What you can do for example is compare the chain of performed actions against a trusted version (for example an older local game save file) and then validate all new (i.e. appended) actions at the end. I think we had this discussion previously but I personally try to play with people I trust that they do not cheat. Under that assumption you do not need additional verification/validation functionality. In my opinion if you do not have that certainty you either need a central trusted instance that somehow verifies that the game has not been tempered with (for example using the comparison approach I described above) or maybe some kind of distributed ledger approach (I am not an expert for such approaches). Independent of this I think we should continue the discussion about possible ways to validate game save files and sequences of actions in a different issue. |
What is the goal of the evaluation? I ask as clarifying that might clarify the next steps. Is it to determine whether we could implement another format (if so, looks like you've shown that). Is it to determine the best way to implement another format? Is it to determine if we want to implement another format? Is it to uncover issues with moving to another format? Is it to determine what that format is? It seems that the bulk of these questions would involve creating some type of proof-of-concept and playing with that (on say a branch) but not a PR merged to master. I would fully support that too as that gives other developers a chance to play around with it, testing, trying to break, etc, all without threatening the continued advancement of master. If we merged this and then decided later that we didn't want XStreams, or XML or wanted to use converters or Jackson or any of another 20 questions then cleaning up/backing out/fixing up these 45 file changes would be a serious pain and offer up opportunities of breaking code in master that we already know is quite stable. It would seem that the next step would be to clarify and get consensus and buy-in on what the long term plan is for the file format. Once that has been decided, and if it's XStreams and XML then you can clean up this PR and it can be merged. |
Agreed, hence my original comment about being an open source project. But I think @neutronc's comment has weight in the context of this PR because we are making the ability to change a saved game from somewhat difficult to simple. We should retain at least a similar level of "obfuscation" as we currently have. And while it's great that you are able to play only with people you know and trust, I wouldn't assume that is the same for our complete user base :) Currently there is a certain "height" to the bar someone would need to hurdle to cheat. A plain text file lowers that bar as low as possible and hence people that wouldn't necessarily go out of the way to compile the project for the developer tools might find a plain text file too inviting. |
Exactly. |
Dont misunderstand me, i am all for things that help us debugging and developing. |
Let me summarize my position in this matter. If a zipped format (.zrails) can solve the file size issue, XML (or JSON) files approach acceptability, but that does not mean that I am waiting for it. For me, the existing tools are more than enough. These are:
All three outputs can easily be copy/pasted to an external editor (I'm using Notepad++). I don't really need anything else. A binary-to-XML (or JSON) converter might be handy in some cases, but I would rather have that as a separate tool. Whatever way we go, any pushed code should in my opinion for the time being be stored in a separate branch, until we are ready to draw final conclusions. |
You are right I need to clarify the goal of this PR and a later evaluation. I think the goal of this PR is to show that implementing another format is feasible and does not necessarily have a negative impact on performance and file size. In addition I think I have made my case clear that a more "speaking" format like XML or JSON can help understanding what happened inside a safe game and fixing possible compatibility issues inside a safe game. I agree that at the moment it is not the best idea to merge this. The next steps in my opinion are:
|
These tools can only be used if the save game file is compatible with the current data model of Rails. For example both XML and JSON provide me this. In both cases I can open the file, search for the changed action and add and/or delete the necessary properties. With a human readable save game file format this can be done without any handwritten tools. |
Not necessarily. There always is a default value (the implied value before such a new field was added), and that can be provided by a line of code in If you want to go the path of changing all relevant saved files (those of the regession tests, and all developer's own test files), that's a lot of work whatever the format is. Somewhat easier if the format is plain text, but if you are going to zip or otherwise obfuscate it, that's extra step(s) per file anyway. |
This PR adds support for xml game save files.
An example can be found in the added file
1835_20210331_2116_Ulli_Hegemann.xml
.I only tested this PR in a limited scope.
Therefore it is quite possible that there are still some bugs with serializing or deserializing the new file format.
In addition this should be taken as a first throw.