Add JSON support for reusable object templates #1667

thabetx · 2017-07-30T07:17:58Z

This PR adds JSON support for the reusable object templates, Tiled should be able to:

Write template instances.
Read template instances.
Write template groups.
Read template groups.

The groups file is used internally by Tiled, so I'm not sure if it's necessary to save it in JSON.

bjorn · 2017-07-30T08:23:06Z

The groups file is used internally by Tiled, so I'm not sure if it's necessary to save it in JSON.

Yeah, I initially thought the same of the objecttypes.xml file, but then people who used the JSON format for maps/tilesets had a reason to parse that file as well and asked for it in JSON format. I guess we'll run into a similar situation for the templates, since game engines will need to load them to understand what the objects are.

Sometimes it's nice to give people options but it also becomes quite a burden sometimes and it causes complexity that often also affects people not using those options. :-/

bjorn · 2017-07-30T08:42:50Z

src/libtiled/maptovariantconverter.cpp

 {
    QVariantMap templateGroupVariant;

-    templateGroupVariant[QLatin1String("firsttid")] = firstTid;
+    if (firstTid > 0) {


Since you have essentially two entirely different implementations based on whether a firstTid is passed or not, I think it would be better to have two separate overloads for this. In fact the version that doesn't take a firstTid can be inlined in MapToVariantConverter::toVariant(const TemplateGroup &templateGroup, QDir &directory).

This is different for the function for tilesets, since those can be embedded.

bjorn · 2017-07-30T08:43:15Z

src/libtiled/maptovariantconverter.cpp

 {
    QVariantMap templateGroupVariant;

-    templateGroupVariant[QLatin1String("firsttid")] = firstTid;
+    if (firstTid > 0) {
+        templateGroupVariant[QLatin1String("type")] = QLatin1String("templategroup");


Actually the type should be set when no firstTid is passed, and doesn't need to be set when it is.

thabetx · 2017-07-30T13:57:04Z

Okay, the groups file contains just a list of file paths for template groups to be loaded on startup, so it should be simple to use JSON, although I'm not entirely convinced that it might be useful to read them as the path of the group will be also included in the map :).

Either way, I'm not sure how to integrate both formats in the UI. Currently the groups XML file is updated automatically with a new path whenever a new group is added or created with no action required from the user. I think it could be done with an export option offered by a button in the dock that offers saving in any of the two formats. What do you think?

bjorn · 2017-07-30T14:05:13Z

Okay, the groups file contains just a list of file paths for template groups to be loaded on startup, so it should be simple to use JSON, although I'm not entirely convinced that it might be useful to read them as the path of the group will be also included in the map :).

Ah, right, this file is really not very useful and can just be maintained by Tiled in whatever format it wants, so XML is fine.

thabetx · 2017-07-30T14:11:59Z

Okay 😄 I will support the the template group file for sure.

thabetx · 2017-07-30T18:14:23Z

Sorry for not being clear in the first description.

thabetx · 2017-07-31T07:11:21Z

I think this PR is now ready. Now all actions are supported on maps and template groups.

thabetx · 2017-07-31T07:13:25Z

XML format was hard coded as it was the only format, now the enhancements PR should be updated to allow opening JSON template groups as well, this could be done when both PRs are merged and the template format singleton can be removed altogether.

bjorn · 2017-07-31T08:15:11Z

src/libtiled/varianttomapconverter.cpp

    }

+    if (templateGroup)


This check is superfluous since the above code always makes sure there is a template group.

bjorn · 2017-07-31T08:18:25Z

src/plugins/json/jsonplugin.cpp

+
+    if (!file.open(QIODevice::ReadOnly | QIODevice::Text)) {
+        mError = tr("Could not open file for reading.");
+        return new Tiled::TemplateGroup();


Why not return nullptr? (and on the line below)

Yeah that's wrong, I used tileset JSON reader as reference, where the returned shared pointer is actually a nullptr.

thabetx · 2017-07-31T16:32:21Z

I applied the last two review comments,The hardest things while doing this PR was connecting everything together, like finding out that format instances should be added to the plugin manager. at first I thought it would pick them up automatically :).

bjorn · 2017-07-31T21:09:01Z

Can you rebase this change onto the latest wip/templates now that #1666 is merged?

The hardest things while doing this PR was connecting everything together, like finding out that format instances should be added to the plugin manager. at first I thought it would pick them up automatically :).

Ah yeah, this is where some documentation of the overall design could help...

Also removed the template format singleton.

thabetx · 2017-08-01T07:52:50Z

I updated the dialogs to support JSON. The new template group is duplicated from the templates dock in the new template dialog as they both offer creating new groups. Is there a way to reuse the newTemplateGroup dialog directly from the templates dock without duplication?

bjorn · 2017-08-14T09:38:09Z

The new template group is duplicated from the templates dock in the new template dialog as they both offer creating new groups. Is there a way to reuse the newTemplateGroup dialog directly from the templates dock without duplication?

Maybe you could create a public static helper function in the NewTemplateDialog that takes the needed parameters, so that you can call this from the TemplatesDock as well. For the rest the PR looks good so I'll merge it now.

thabetx changed the title ~~Json templates~~ Add JSON support for reusable object templates Jul 30, 2017

bjorn requested changes Jul 30, 2017

View reviewed changes

thabetx force-pushed the JSON-templates branch 2 times, most recently from ff4ba6c to 4d5579b Compare July 31, 2017 07:03

bjorn requested changes Jul 31, 2017

View reviewed changes

thabetx force-pushed the JSON-templates branch from 4d5579b to 51ac644 Compare July 31, 2017 11:36

thabetx added 2 commits August 1, 2017 08:27

Support writing template instances in JSON format

39beef7

Support reading templates in JSON format

497b2fe

thabetx force-pushed the JSON-templates branch from 51ac644 to fad148e Compare August 1, 2017 06:32

thabetx added 3 commits August 1, 2017 09:18

Support writing TemplateGroups in JSON format

88db52f

Support reading TemplateGroups in JSON format

26a2f6e

Add JSON to the new template dialog

a6fb85c

Also removed the template format singleton.

thabetx force-pushed the JSON-templates branch 2 times, most recently from 8d91e1f to a6fb85c Compare August 1, 2017 07:46

bjorn merged commit a6fb85c into mapeditor:wip/templates Aug 14, 2017

Uh oh!

Add JSON support for reusable object templates #1667

Add JSON support for reusable object templates #1667

Uh oh!

Conversation

thabetx commented Jul 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn commented Jul 30, 2017

Uh oh!

bjorn Jul 30, 2017

Choose a reason for hiding this comment

Uh oh!

bjorn Jul 30, 2017

Choose a reason for hiding this comment

Uh oh!

thabetx commented Jul 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn commented Jul 30, 2017

Uh oh!

thabetx commented Jul 30, 2017

Uh oh!

thabetx commented Jul 30, 2017

Uh oh!

thabetx commented Jul 31, 2017

Uh oh!

thabetx commented Jul 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bjorn Jul 31, 2017

Choose a reason for hiding this comment

Uh oh!

bjorn Jul 31, 2017

Choose a reason for hiding this comment

Uh oh!

thabetx Jul 31, 2017

Choose a reason for hiding this comment

Uh oh!

thabetx commented Jul 31, 2017

Uh oh!

bjorn commented Jul 31, 2017

Uh oh!

thabetx commented Aug 1, 2017

Uh oh!

bjorn commented Aug 14, 2017

Uh oh!

Uh oh!

thabetx commented Jul 30, 2017 •

edited

Loading

thabetx commented Jul 30, 2017 •

edited

Loading

thabetx commented Jul 31, 2017 •

edited

Loading