LuaSerializer - saves some non-printable chars

[nick]

I've fixed the "loading views" bug :)
The JSON load now gets as far as LuaSerializer (the last item!) and crashes. I believe this is due to non-printable characters.

LuaSerializer::Serialize collects all the data into a std::string, and then the string is saved - as BINARY.
When coding the new JSON system, I simply saved the std::string as a JSON string. However, inspection of the string in the JSON text file reveals some non-printable chars (which appear at the end of the string as x88 xB7 xE0 in notepad++).
I think that when loading the file, the non-printable chars get parsed in a way such that they are different when read back into a std::string.

So, how to fix this? One way is to save the string chars as a bunch of ints (or some other binary-to-text encoding), but then the saved Lua data will not be readable!
Any other suggestions? Will have a think about this later this evening.

Also, I see that robn has done some work on LuaSerializer that has been added to pioneer release - does it affect this issue? Or just refactor the code? Will have a look later.

[Edit]Apart from re-structuring the Lua save process and transforming the binary data to ascii (which should be done at some point), I see no other way to fix this. I'll create a new JSON utility function that saves/loads a string containing binary data safely.

[Edit] Done - Pioneer now saves and loads using the JSON system :)
Still got some issues (e.g. cockpit mode only sees stars - no Earth) so I'll be addressing them next.

[robn]

Your "x88 xb7 xe0" could be a fragment of a UTF-8 string.

Also note that there's no actual requirement that a Lua string or a std::string contains only printable characters - it could be anything. You need to cope with that.

That said, JSON is fully UTF-8 aware, and has escaping rules for binary data. You shouldn't really need to do anything particularly complicated.

LuaServerAgent.cpp has a Lua<->JSON converter you may like to repurpose.

[nick]

Thanks robn - will look into it this evening.

Also note that there's no actual requirement that a Lua string or a std::string contains only printable characters - it could be anything. You need to cope with that.

The storing of non-printable chars in a std::string is not a problem - it's the writing to and reading from a text file.

Maybe JSON needs to be told to handle UTF-8 ? Will look for a switch somewhere.

[laarmen]

I know I'm a bit late to the party here, but why not start a bit small, and just convert *only* the Lua serializer to JSON and leave the rest in the horrible format it is in now ? This would make it a bit easier to differentiate bugs related to the generic JSON handling from the ones specifics to the converstion of the data structures. As they say, Rome wasn't built in one day.

(And as a bonus, it would make the review easier, as it could b e done in several chunks)

[nick]

I've moved on from the Lua serialisation. I got it to work by saving the string as a JSON array of ints - each int encodes a char. Unelegant, and makes the save file larger, but the Lua serialisation can be improved later :)

The JSON load now runs to completion and results in a running game - great!
But there are issues to fix. There appears to be some code that I've missed (class Node) which I'm addressing now.

To clarify the situation:
I'm working on pioneer source that I downloaded on 3rd January which isn't managed by git (I'm gradually learning it).
So when it comes to merging - it'll all be added in one go! Sorry about this, but I'd never have got started if I had to learn git first. My skills are C++, maths & physics, and object oriented programming, but I'm not so experienced with software management and merging tools (I've always been the only programmer working on any code, so I've never had to do a real merge).

[laarmen]

You can't expect us to merge a huge piece of code touching areas as critical as the serializisation as an enormous blob of diff without a proper review. And a proper review is next to impossible if the pull request isn't properly formatted.

Which means you have two viables options: learn git, or post multiple small PRs, for which we are (I am, but I'm fairly certain I'm not the only one) less picky about the formatting since the overall diff is easier to grasp by itself. Note that the latter will teach you git eventually :-).

Side note : Git is NOT a merging tool. It is a version control system that is especially good at avoiding the needs to merge code when merging back together two divergent branches of the code. When it cannot avoid it, and only then, does it ask you to do the code merge for it. If you write code, and you learn git, there will come a time in a near future when you will wonder how you did things without it. Also, learning how to do proper commits isn't a skill that is only used in git, but pretty much any time you want to contribnute to a Free Software project.

[nick]

Plan is to fix current bug(s), then learn git and update the PR.
I'm happy to learn git first?
First thing I need to figure out, is how to get the source I've been working on (from 3rd Jan) to be managed by git.
I'll do some reading up.

[FluffyFreak]

My advice is to checkout a clean clone/branch of upstream/master like this:

git checkout upstream/master
git checkout -b JSONSerialise

Then re-implement your changes - don't mean rewrite it all by hand, I just mean copy only the CODE you've written and not the FILES like you have done because you're correct that there have been changes since you started and when you copy over the file you effectively tell Git that you've made all of those changes to that file, it doesn't know that you only added two new methods, it also looks like you've changed the rendering code in "WorldView.cpp" for example and that makes the history and versioning in Got very messy even if I can fix it.

Start with the helper functions and the files JsonUtils.h/cpp, add them and then do a commit containing just them. Then implement more, do a commit with each piece of work so that when your do make a new Pull Request people can see a list of things showing what you've done and why.

I've just checked out your branch and unfortunately I think you've already done some manual merging in it which has broken it.
It won't compile anymore as you have resurrected some old rendering code that no longer exists - this is why getting the right tools for the job is so important :)

So I've mentioned WinMerge before but there are a bunch of things that come with TortoiseGit which I use constantly as well.
It's very handy being able to right-click on a file, choose "Show Log" and just get it's entire history, or "diff with previous version" or whatever.

Right-click and "Resolve" when you do get merge conflicts is another really useful feature!