Thanks to the developers! (& suggestion for savegames)

[robn]

Just a note, you need to use hexfloats - decimal floats lose precision when you convert them to strings and back.

[robn]

Because there's no way of doing that.
You're reading a stream of bytes that have to be in the correct order.
Almost no way of telling if what you've got is valid or invalid when you read it, and if you did what would you do with it? How do you fix it up?

If you're reading a stream of bytes, then yes. If you're using an unordered k/v format, you can upgrade.

Incidentally, I wrote a proposal for all of this once, and I already implemented most of this. It was blocked on lua-equipment but that was done long ago. I already solved many of the problems that have been mentioned here. I'm not saying that you have to use my work (it'd be hard for most people to figure out where I was at, including me), but it is there. I'd like to pick it back up but it just needs time that I can't find right now.

I'll see if I can dig up the proposal I wrote. I can't find it quickly right now.

[laarmen]

I love how some people say "Why not just <rewrite one of the most complex piece of code we have> ?".

So, here is a couple reasons as far as I'm concerned:

1/ Most of the time, it works, and it's still much faster for me to track down and fix the occasional bug than to rewrite the whole thing.
2/ I've grown attached to this thing, much the same way people grow fond of the ugly limping kitty with only one eye, because they spent so much time caring for it in the first place.

Now, if you come up with a replacement I will be one of the first to praise your name and erect a statue of you in my backyard, but in the mean time I've got other things I want to do in Pioneer. Hint: look at robn's work.


About the whole fallback thing: the problem of fallbacks, is that it adds even more code that will eventually be subject to bitrot. I do get the upside of having it, but you have to understand that it can only go so far, and at some point it's probably better for everyone to just give up and declare the savegame invalid, instead of having an patched-up incoherent game.

NB: I'm one of the few people in the dev team that actively tries to ensure retro-compatibility when adding new features, so don't think I'm just being lazy about this stuff.

[robn]

I found what I wrote. Not quite a spec, just the start of my thinking after a number of experiments. It was all in posts to the now-defunct pioneer-dev mailing list two years ago.

http://pastebin.com/JiQeaDQu
http://pastebin.com/tFz8GYsm
http://pastebin.com/RZn57GsD
http://pastebin.com/R9SjYujP

That's all that actually got written about it. I did a lot more work and made a lot more decisions, which now exist only on the robn/json-savefile branch.

Long story short: replacing the save system properly (ie redesign) is immensely difficult and will force you to learn about pretty much the entire codebase, as well as making some significant structural changes in parts of it. Changing the file format from a binary stream to a kind of JSON-stream is comparatively easier, but not especially useful. I say this not to dissuade you, but to try to make you aware of the enormity of the task.

[nick]

Hi robn,

Thanks for your note about the hexfloats :)

I realise that a proper redesign of the save system is an enormous task!

Moving to nested json data (the same data - except that some integer sizes [of arrays] are no longer required, as we can just iterate through the JSON arrays) is a task that I think I have a chance of completing. But is it a worthwhile task? It won't suddenly make all savefiles forwards compatible, because there will always be occasional structural changes that can't be dealt with by using the "use default value" principle for missing data. It will however, make the save file human readable, and generally more robust due to strict-byte-order no longer being enforced. And perhaps it will be a better starting point for future structural changes in the save system? The immediate price to be paid, is that the save/load process will be slower.

Is it worth me doing this? What do developers think?

[nick]

I'll see how it goes.

I'm going through the save system in order of exectution. After the signature and version, is the galaxy generator. I don't understand the data being saved, but I've followed the existing process. The JSON file is now:

Code: Select all

{
   "galaxy_generator" : {
      "name" : "legacy",
      "sector_stage" : [
         {},
         {},
         {
            "dict" : []
         }
      ],
      "star_system_stage" : [ {}, {}, {}, {} ],
      "version" : 1
   },
   "hyperspace_duration" : -6.277438562204193e+066,
   "hyperspace_end_time" : -6.277438562204193e+066,
   "hyperspace_progress" : -6.277438562204193e+066,
   "leading_signature" : "PIONEER",
   "state" : 0,
   "time" : 48600.26666668057,
   "version" : 80,
   "want_hyperspace" : false
}

The "sector stage" and "star system stage" appear to be a list of "dict" entries (which I presume means "dictionary") which are themselves maps of "system path" to a number.
The actual virtual method called can have an empty body, which appears to be the case here for most entries - which results in an empty JSON object "{}". In fact, only one of the calls executes an actual serialisation, but the associated "dict" is empty "[]".
I have no idea what this data means, but it's what is currently being saved. (Game saved immediately after starting a new game).
hyperspace_duration, hyperspace_end_time and hyperspace_progress appear to be uninitialised here.

Floating point data will eventually be saved as hex floats (to preserve precision) where required.

The next section will be "space, all the bodies and things".

[impaktor]

I do know the code (I think) you're looking at is written to allow the player to pick which galaxy to play in (when selecting "Start New Game"). The idea is that we will eventually have a "stable galaxy" and a "dev-galaxy" where things might change quite easily. Thus the version number of galaxies. I don't know if that helped, but I'm not familiar with the code in any greater detail than that.

Recent galaxy work can be found (somewhere) in these merged pull requests by lwho:
https://github.com/pioneerspacesim/pion ... hor%3Alwho

[nick]

I'm making progress with the JSON save system, but have encountered a linker error that I can't figure out. [Edit] this is now fixed

The only new files are jsonUtils.h and jsonUtils.cpp (I didn't think I ought to put the new functions in jsoncpp.h/jsoncpp.cpp, which are probably "as is" - let me know if I'm wrong!)

Client code uses these functions with no problem in other cases (I just put #include "json/jsonUtils.h" at the top of the relevant cpp file).

However, for Model.cpp I get the linker error:

Error 12 error LNK2001: unresolved external symbol "void __cdecl Matrix4x4fToJson(class Json::Value &,class matrix4x4<float> const &,class std::basic_string<char,struct std::char_traits<char>,class std::allocator<char> >)" (?Matrix4x4fToJson@@YAXAAVValue@Json@@ABV?$matrix4x4@M@@V?$basic_string@DU?$char_traits@D@std@@V?$allocator@D@2@@std@@@Z) C:\Users\Nick\Documents\Nick\Programming\C and C++\Pioneer\pioneer-master\win32\vc2013Express\scenegraph.lib(Model.obj) modelcompiler

yet at the top of Model.cpp I put #include "json/jsonUtils.h".
[EDIT] I realise that this is the condition for getting the code to COMPILE (which it does) yet I still can't figure out this linker error (something to do with namespaces? libraries?)

I'll add some more information to this topic in a few minutes.

Any ideas will be gratefully received! :)

FYI:
* All floating point data is serialised as hexfloats.
* All 64 bit integer data is converted to string (via sprintf "%I64u") because JSON 'Number' can't deal with 64 bit integers.

[nick]

Here's jsonUtils.h:

Code: Select all

#ifndef _JSON_UTILS_H
#define _JSON_UTILS_H

#include "json/json.h"
#include "vector3.h"
#include "matrix3x3.h"
#include "matrix4x4.h"

void Vector3fToJson(Json::Value &jsonObj, const vector3f &vec, std::string name);
void Vector3dToJson(Json::Value &jsonObj, const vector3d &vec, std::string name);
void Matrix3x3fToJson(Json::Value &jsonObj, const matrix3x3f &mat, std::string name);
void Matrix3x3dToJson(Json::Value &jsonObj, const matrix3x3d &mat, std::string name);
void Matrix4x4fToJson(Json::Value &jsonObj, const matrix4x4f &mat, std::string name);
void Matrix4x4dToJson(Json::Value &jsonObj, const matrix4x4d &mat, std::string name);
void Color4UBToJson(Json::Value &jsonObj, const Color &col, std::string name);

#endif /* _JSON_UTILS_H */

and here's jsonUtils.cpp:

Code: Select all

#include "jsonUtils.h"
#include "utils.h"

// npw - new code
void Matrix4x4fToJson(Json::Value &jsonObj, const matrix4x4f &mat, std::string name)
{
	if (name.empty()) return; // Can't do anything if no name supplied.

	Json::Value matArray(Json::arrayValue); // Create JSON array to contain matrix data.
	matArray[0] = HexFloat(mat[0]);
	matArray[1] = HexFloat(mat[1]);
	matArray[2] = HexFloat(mat[2]);
	matArray[3] = HexFloat(mat[3]);
	matArray[4] = HexFloat(mat[4]);
	matArray[5] = HexFloat(mat[5]);
	matArray[6] = HexFloat(mat[6]);
	matArray[7] = HexFloat(mat[7]);
	matArray[8] = HexFloat(mat[8]);
	matArray[9] = HexFloat(mat[9]);
	matArray[10] = HexFloat(mat[10]);
	matArray[11] = HexFloat(mat[11]);
	matArray[12] = HexFloat(mat[12]);
	matArray[13] = HexFloat(mat[13]);
	matArray[14] = HexFloat(mat[14]);
	matArray[15] = HexFloat(mat[15]);
	jsonObj[name] = matArray; // Add matrix array to supplied object.
}

(only one function shown for brevity).
and here is the client code that causes the linker error:

Code: Select all

// npw - new code (under construction)
class SaveVisitorJson : public NodeVisitor {
public:
	SaveVisitorJson(Json::Value &jsonObj) : m_jsonObj(jsonObj) {}

	void ApplyMatrixTransform(MatrixTransform &node)
	{
		const matrix4x4f &m = node.GetTransform();
		Matrix4x4fToJson(m_jsonObj, m, "matrix_transform");
	}

private:
	Json::Value &m_jsonObj;
};

(which is in the file pioneer-master\src\scenegraph\Model.cpp)
The call to Matrix4x4fToJson is the offending call.
I've derived SaveVisitorJson from NodeVisitor, so this ApplyMatrixTransform will be called when appropriate.
Incidently, this part of the move to JSON is the only part that doesn't simply have functions "ToJson" (instead of "Serialize") and "SaveToJson" (instead of "Save").

[nick]

If I can remove jsonUtils.h/.cpp and put the functions in jsoncpp.h/.cpp then there won't be a problem - can I do this? :)

[Edit] Actually this is not a good idea - because it means that jsoncpp will need to know about types like matrix3x3d. The reason I created jsonUtils.h/.cpp is to keep these types and json decoupled. So I'm going to have to overcome this linker problem.