[PATCH] Reduce/avoid duplicated code in Atlas' ScriptInterface

[Yves]

I open a ticket for this task because there are several alternative approaches and some discussion could be needed/helpful.

We have a ScriptInterface.h and ScriptInterface.cpp file in the ​AtlasScript project. These files are more or less copies of bits from other files in the ​ScriptInterface project. Duplicated code is bad because it requires more time for updating and is more error prone.

Approach1: Remove most of the duplicated code

I still have to check how much of the scripting code in AtlasScript is actually still used. Maybe most of it can be removed and the amount of duplicated code can be reduced to an acceptable amount.

Approach2: Use the full ScriptInterface project from atlas

We include the full ScriptInterface.h header from the scriptinterface project and either (2.A) link the binary created in that project or (2.B) add another project(in an IDE or makefiles) pointing to the same files.

With both approaches we would have to add some logic to premake4.lua to use the main ScriptInterface project and we would have to remove duplicated code and implement additional features in the main ScriptInterface project (if there are any).

One problem is that all projects/modules of the main game include precompiled.h as the first file in all their .cpp files which then includes lib/precompiled.h. This file contains some types and definitions that are needed by ScriptInterface.h and other headers from the ScriptInterface project. So they actually depend on the .cpp which uses them to include additional headers. Maybe we can include everything that's needed in the header without breaking precompiled headers (I think that should theoretically work with include guards). Anyway, quite a lot of new header files would have to be included (indirectly) by Atlas projects.

2.A same project

This means we have to use the same compiler and linker flags for the ScriptInterface in the main game and in atlas. We'd need the fPIC flag for example because atlas is a shared library and needs position independent code (PIC). PIC can have an impact on performance but I don't know how big or small that impact is. Maybe there are other flags where the flexibility to have different values for the main game and atlas would be nice.

2.B separate project

Creating a separate project that points to the same source files could be a little bit confusing and means longer build times. In exchange we get the flexibility to specify different build/linker-flags for the main game and atlas.

Approach3: Update/Keep the duplicated code

If the conclusion is that all other alternatives are worse, we could still keep the duplicated code and I would have to update it for SpiderMonkey 24 too.

Question: Scripting use in Atlas

For a good decision it's important to know if we will use more or less scripting in Atlas in the future. If we will use more scripting, it would make sense to include the fully featured ScriptInterface project (Approach2) instead of keeping duplicated code (Approach3) or reducing the amount of duplicated code in the short term (Approach 1).

Opinions?

[Yves]

It looks like the JSAPI and functionality based on it is currently only used for the conversion to JSON. If we'd be sure this stays that way, we should probably implement a direct conversion from a AtObj to JSON. However, I think it's more likely that additional JS functionality will be required for Atlas (e.g. for triggers or mission scripts) and tend more to including the full Scriptinterface. I'm going for Approach 2.B now.

[Yves]

Using the ScriptInterface for Atlas is quite messy and requires too many other dependencies. ScriptInterface uses symbols from the "lowlevel" project and the "engine" project.

Philip thinks JS functionality won't be needed for atlas (triggers, mission scripts and such run in the engine). I'll try removing the AtlasScript project and the SpiderMonkey dependency completely from Atlas. The conversion from AtlasObject to and from JSON will need to be reimplemented without the intermediate conversion to JS.

[Yves]

This patch removes the duplicated ScriptInterface code from Atlas, removes the SpiderMonkey? dependency from Atlas, gets rid of the whole AtlasScript project and adds an implementation of the JSON parsing and writing which uses the JSON Spirit library.

[Yves]

adds missing file.

[Yves]

Fixed a few issues with building on Windows

[Yves]

Unfortunately we have to bundle some more headers for boost (extract to libraries\win32\boost\include\boost).

[Yves]

Unfortunately with the additional requirements for boost headers there are quite a lot of files that need to be added (see boost.7z). These headers are part of the official boost library, but they weren't added to our repository because they weren't needed at that point.

I still think this solution is easier to maintain than doing it with SpiderMonkey. The JSON Spirit library seems to be quite stable and we already use boost, so we just have to add some additional boost modules/directories and replace them too when we update to the next version. Also SpiderMonkey is really not a good choice if we just want to read and write JSON. We have to initialize a runtime, a compartment, a global and a context. We have to assign memory for the engine and care about garbage collection and potential OOM issues. We have to worry about rooting and moving GC issues etc...

OK what do you think? I've marked the ticket for review to get some other opinions.

[Yves]

I'll wait until this Thursday for feedback before I commit (I don't have much time until then anyway).

[historic_bruno]

Another option would be to leverage wxWidgets, since it's a huge library we already need for AtlasUI, and it's used in the existing implementation of AtlasObject. For instance, there's ​wxJSON. It has literally 3 source files and I think it's only dependency is... wxWidgets :)

Now I don't know how it compares with JSON Spirit, maybe it won't work or it's too difficult to use, but it uses the wxWidgets license, so at least that is OK (note: the download link has moved to ​here).

It seems reasonable to get rid of the Spidermonkey/JS dependency for AtlasUI, and on the one hand it seems potentially useful to have a non-SM JSON parsing/constructing library around. But on the other hand I can't think of where else we would ever want or need it, outside the engine, which can use SM.

Is there a situation we can think of where we want JSON parsing that's not tied to AtlasUI or SM? Or maybe we can rule wxJSON out for some other reason?

[historic_bruno]

The other thing this reminds me of: we should have some tests for AtlasObjects using JSON, like test_AtlasObjectXML does for XML data. I think it will be even more important now if we're using multiple libraries to process JSON.

[Yves]

Thanks for the feedback! I had a look at several libraries. The reason for choosing JSON Spirit were mainly:

1. It's been around for quite a while (since 2008) but got updates regularly and still got updated last year. Also check the ​revision history. WxJSON made me worry a bit if it's going to be updated to newer versions because the last updates were from 2010 (according to the ​download section on sourceforge) and it still doesn't support WxWidgets 3.0 (it has been released in November last year).
2. It only relies on boost which we already use (I figured out later that it needs additional headers which we excluded on our bundled copy of the boost headers for Windows). This argument for JSON Spirit was essentially the same argument you have for WxJSON.
3. There's a header-only version which avoids additional trouble for linking the library properly on all supported platforms.

Other libraries had different problems. One very small Library with just one header didn't support formatting of the output and put it all in one big line. Another library didn't support getting the type of values, only receiving them through templates. Many libraries didn't seem to be updated anymore, were only about a half year old or had many unanswered bugreports associated. Some had a few bad ratings talking about heap corruption and other things. One library was only in C and not C++.

I wouldn't want WxJSON if it's not getting updated anymore. Do you think the additional headers matter much? In this case if you have a suggestion for another library, I could have a look.

Tests are a good idea, but I'd probably wait until after the SpiderMonkey upgrade before implementing them. What I've done so far is loading a map, saving it again and comparing the files with a diff. They were the same.

[Yves]

@historic_bruno Is it right that you don't have a strong opinion against JSON Spirit or for another library? In this case I'd commit the patch at the current state.

[historic_bruno]

I don't have a strong opinion against JSON Spirit.

[Yves]

Thanks again for reviewing, I'll commit soon. :)

[Yves]

In 14780:

Adds the variant headers of boost which are needed by the JSON Spirit library.
Refs #2434

[Yves]

In 14781:

Adds the spirit headers of boost which are needed by the JSON Spirit library.
Refs #2434

[Yves]

In 14782:

Adds the math headers of boost which are needed by the JSON Spirit library.
Refs #2434

[Yves]

In 14783:

Removes the ScriptInterface and SpiderMonkey dependencies from Atlas.
Replaces the code for JSON parsing and writing with an implementation using the JSON Spirit library.

Fixes #2434
Refs #1886

[fabio]

It would be nice to be able to use system json-spirit, available, e.g., in Debian/Ubuntu: ​https://packages.debian.org/source/sid/json-spirit

[fabio]

Actually, since it has not a binary package, it won't change the resulting binaries, so this may not be really useful since the header are needed anyway in 0ad for the other systems...

[Yves]

Well, I'm using the template version which is easier. The other way would be using the object library version (not including the templated headers). ​http://www.codeproject.com/Articles/20027/JSON-Spirit-A-C-JSON-Parser-Generator-Implemented#comp

I thought the difference in binary size is too small to be worth the additional trouble.

[historic_bruno]

Replying to Yves:

The other way would be using the object library version (not including the templated headers). ​http://www.codeproject.com/Articles/20027/JSON-Spirit-A-C-JSON-Parser-Generator-Implemented#comp

If it has a C++/STL API, I would strongly advise not doing that, as it will only cause nightmares with compilers like MSVC that don't guarantee STL ABI compatibility between major releases (the same nightmare we had with gloox, and will have with tinygettext).

[Yves]

Oh yes, I didn't think about that. That's an even better reason that avoiding the additional efforts and the complexity of maintaining it as object library.

[Yves]

In 14797:

Should fix the Atlas build issues with the autobuilder.
For some reason the autobuilder does not know uint32_t but my Windows system with VC2010 and my Linux system both do.

Refs #2434

[Yves]

In 14827:

Fixes behaviour change in Atlas' JSON serialization after the switch to JSON Spirit (#2434).
This caused problems setting the starting camera.

Refs #2434