Reusable object templates #1587

thabetx · 2017-06-01T12:37:00Z

This pull request is about the reusable object templates project in GSoC 2017, addresses #70 .

The entry point for testing is selecting some objects and choosing "Save as a template group", the selected objects will be saved to a file, then the file will be immediately read and the sizes of the saved templates will be printed, this is just for testing purposes.

When saving tile objects, the tileset info will be saved as well, this encourages having standalone template groups that can be used across multiple objects, although for now, when reading the file, the tilesets are ignored.

The next step will be creating a template group document that will allow loading the template group into tiled.

Code wise, I've followed tileset format and map format for creating the new classes, the template group is till now similar to object group, maybe I will need to create a new class for a template instead of using the object class.

Reading and writing reuses some of the available code, I had to extract some of the object reading and writing code (the part that handles different types of objects), otherwise things are pretty similar to writing object groups albeit with some changes like not writing the coordinates of the object or its ID as they won't be relevant when creating new copies of the template.

So currently this hypothetical use case is simple and some parts of the code are not complete, I think having the template group ready and loading it into Tiled will require doing more checking and will reveal more cases that require better handling.

bjorn

You've made a good start! To avoid building up a too large pull request, I think we should agree on certain "check points" and set the target branch to the wip/templates branch that I just created. This way, we can merge pull requests there until the whole feature is ready to be merged into master.

I would suggest the first checkpoint to be roughly the functionality you've started with here, in addition to the following bits:

Finish the saving and loading of tileset references.
Finish the implementations of the TemplateGroupDocument methods.
Using a temporary TemplateGroupDocument in MapDocument::saveSelectedObjectsAsTemplateGroup for testing its functionality.
Working on the feedback I put in comments (probably including the addition of a Template class, which I think should have a "name" and "id" independently of the MapObject it refers to).
Adding the license header to new files.

Then, I think we have a good base to continue to the next steps like implementing the Templates dock.

bjorn · 2017-06-03T17:32:35Z

src/libtiled/mapreader.cpp

+
+    MapObject *object = new MapObject(name, type, {0, 0}, size);
+
+    readObjectData(object);


Why did you decide to not use the following format?

<template> <object ...> ... </object> </template>

Such that, you could re-use the entire readObject function within the readTemplate function and avoid the need for readObjectData? It would also allow a separation of concerns, like a template.name attribute could be used as name of the template, whereas object.name would be the name of the instantiated object.

As an extension of this, I think you need to add a Template class, which points to its object using a member. TemplateGroup::addObject then becomes TemplateGroup::addTemplate. This may also provide a good place to keep back-references to instances of the template, in order to know what to update when the object is changed.

Alright, I didn't understand what you meant, I thought you referred to the template group as template.

So just to be clear the format will look like this

<templategroup ...> <template ...> <object ...> ... </object ...> </template ...> <template ...> <object ...> ... </object ...> </template> </templategroup>

This looks more effective, I will update it and the template class makes more sense now.

And for writing, is there a better way other than creating a new function just to prevent writing the id and the coordinates? Does sending a parameter to tell it whether to write them or not be appropriate?

I think it would be fine to prevent the writing of the ID by just making sure it is 0 and adding a check for this in MapWriterPrivate::writeObject. For now, I would not bother with preventing the writing of the position.

Okay that would allow more code reuse, but what do you mean by setting id to 0, wouldn't that change the original object?

Well, only the object owned by the template, which should be a copy, would have its ID set to 0.

Okay great, thanks!

template is a keyword in C++, would objectTemplate as the name of the class be fine?

bjorn · 2017-06-03T17:34:01Z

src/libtiled/mapreader.cpp

+{
+    QFile file(fileName);
+    if (!d->openFile(&file)) {
+        return new TemplateGroup();


Coding style: please leave out () when constructing classes without passing any constructor arguments.

In addition, it seems weird to return an empty constructed template group when the file could not be opened. Consider if you can just return nullptr.

bjorn · 2017-06-03T17:37:56Z

src/libtiled/mapwriter.cpp

+    writer.writeStartElement(QLatin1String("templategroup"));
+
+    if (mDtdEnabled) {
+        writer.writeDTD(QLatin1String("<!DOCTYPE map SYSTEM \""


The map part of the DTD should match the root element, so it should be templategroup. Of course, eventually we'd have to update the DTD as well then. I'm not sure if DTD validation is really useful though.

bjorn · 2017-06-03T17:39:07Z

src/libtiled/mapwriter.h

+    void writeTemplateGroup(const QList<MapObject *> &mapObjects, QIODevice *device,
+                       const QString &path = QString());
+
+    bool writeTemplateGroup(const QList<MapObject *> &mapObjects, const QString &fileName);


Since you already have a TemplateGroup class, I would suggest to use that here instead of a plain list of objects.

bjorn · 2017-06-03T17:41:18Z

src/tiled/abstractobjecttool.cpp

@@ -259,6 +264,8 @@ void AbstractObjectTool::showContextMenu(MapObjectItem *clickedObjectItem,
                                                    isResizedTileObject));
    }

+    menu.addAction(tr("Save As A Template Group"), this, SLOT(saveSelectedObjectsAsATemplateGroup()));


It's grammatically fine and easier to read when you leave out the "A" here (also in the function name).

bjorn · 2017-06-03T17:43:10Z

src/tiled/mapdocument.h

@@ -43,6 +43,7 @@ class Map;
 class MapObject;
 class MapRenderer;
 class MapFormat;
+class TemplateGroupFormat;


This forward declaration seems not needed here (maybe not anymore).

bjorn · 2017-06-03T17:45:50Z

src/tiled/templategroupdocument.cpp

+using namespace Tiled;
+using namespace Tiled::Internal;
+
+TemplateGroupDocument::TemplateGroupDocument(TemplateGroup templateGroup, const QString &fileName)


Probably the template group should be passed in as a pointer instead.

thabetx · 2017-06-04T13:47:51Z

I've updated the format, added the tileset references to the templategroup and fixed some review points.

I'm working on changing the writing function to work directly on a template group instead of an object list, and will add more information to the template group and the templates.

thabetx · 2017-06-04T23:38:33Z

I updated the saving function and will start working on the TemplateGroupDocument.

I added a name to templateGroup and a name and ID to objectTemplate, not sure yet how the ID will be used.

To prevent writing the ID and the coordinates of the mapObject in the objectTemplate, I used the suggestion of setting the ID of the cloned mapObject to 0 and used it as a check.

Extracted some code from readObject function

Reduce code duplication in writing objects and templates

- Remove double included QDebug - Fix some warnings - Remove writeObject function from TmxMapFormat - Print the size of the read objects for testing - Formatting - Rename the saving function - Remove some comments - Change the name of the saved file

- Rename TtxTemplateFormat to TtxTemplateGroupFormat - Review related updates - Return nullptr when a template group file can't be opened - Remove wrong DTD link - Rename saving function from save as a template group to save as template group - Use template group pointer in temlate group document constructer - Remove unneeded forward declaration

Added objectTemplate class, and used the object reading and writing functions.

Also added extra attributes to the template group and the object template and updated saving, reading and testing accordingly.

thabetx · 2017-06-07T07:53:15Z

I implemented the TemplateGroupDocument functions and now it's used for testing. I've also squashed some of the commits, testing is still done by selecting some objects then choosing "save as template group".

bjorn

Only some minor comments, then I will merge this. I've tested it shortly and it seemed to work well!

bjorn · 2017-06-07T21:22:34Z

src/tiled/templategroupdocument.cpp

+{
+    QString displayName = mTemplateGroup->name();
+
+    if(displayName.isEmpty())


Coding style: please put a space after if.

bjorn · 2017-06-07T21:23:45Z

src/tiled/tmxmapformat.cpp

+    mError.clear();
+
+    MapReader reader;
+    TemplateGroup *templateGroup =  reader.readTemplateGroup(fileName);


Coding style: one space too many after =.

bjorn · 2017-06-07T21:26:04Z

src/tiled/mapdocument.cpp

+/**
+ * Currently, this is the entry point for testing templates,
+ * it creates a templateGroup from the selected objects except the last object,
+ * then creates a templatGroupDocument from the templateGroup,


Typo: templatGroupDocument (also, I think it would be better to match the casing with the class name).

bjorn · 2017-06-07T21:33:01Z

src/tiled/mapdocument.cpp

+
+    QString fileName = QLatin1String("templateGroup.ttx");
+
+    TemplateGroupDocument *templateGroupDocument = new TemplateGroupDocument(templateGroup, fileName);


No change required, but just to note, that to have less repetition I am often using auto for such lines these days, since the type is completely obvious:

auto templateGroupDocument = new TemplateGroupDocument(templateGroup, fileName);

thabetx force-pushed the reusable-object-templates branch from 5bb77d2 to 408c31e Compare June 3, 2017 08:30

bjorn requested changes Jun 3, 2017

View reviewed changes

bjorn changed the base branch from master to wip/templates June 3, 2017 18:29

thabetx force-pushed the reusable-object-templates branch from cbf91a9 to d004f38 Compare June 4, 2017 09:45

thabetx added 15 commits June 5, 2017 17:36

Use TmxMapFormat to write an object in a separate file

c60183d

Create a new function for saving template data

4853363

Create the TemplateFormat class

1ac04ed

Use template instead of object in function names

39ab2ca

move template format to libtiled

c3ea4b0

Temporarily allow saving multiple templates at once

1daa645

Make tilesets shared between all templates

6e5ccab

Create the templateGroup class

56f04a8

WIP Add reading templates functionality

2c7ff00

Add read function in the format files

2ce4b97

Add temporary testing through context menu

d01cbb0

Write object properties in the template

37a7e28

Renaming

d0f140a

Read the saved template group

8d9d9fa

Write and read template name and properties

2b2bcc5

Extracted some code from readObject function

thabetx force-pushed the reusable-object-templates branch 3 times, most recently from c8dabf0 to 69a2353 Compare June 7, 2017 07:20

thabetx added 6 commits June 7, 2017 09:30

Extract writeObjectData function

c5acf77

Reduce code duplication in writing objects and templates

Code cleanup

d32cced

- Remove double included QDebug - Fix some warnings - Remove writeObject function from TmxMapFormat - Print the size of the read objects for testing - Formatting - Rename the saving function - Remove some comments - Change the name of the saved file

Add format setter and getter for templateGroup

1ab0613

Create skeleton for TemplateGroupDocument

498d8b4

Rename TemplateFormat to TemplateGroupFormat

7594515

Add setting error string when reading a template group

6ccfa52

thabetx added 6 commits June 7, 2017 09:31

Update the template group format

c922eee

Added objectTemplate class, and used the object reading and writing functions.

Add tileset references to the template group

942847c

Update saving to work directly with the template group

389ebdc

Also added extra attributes to the template group and the object template and updated saving, reading and testing accordingly.

Implement TemplateGroupDocument and use it in testing

48cb7cb

Add license header

7bdcec9

thabetx force-pushed the reusable-object-templates branch from 69a2353 to 7bdcec9 Compare June 7, 2017 07:34

bjorn requested changes Jun 7, 2017

View reviewed changes

Style fixes

060b9d8

bjorn merged commit 1d3befb into mapeditor:wip/templates Jun 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reusable object templates #1587

Reusable object templates #1587

thabetx commented Jun 1, 2017

bjorn left a comment

bjorn Jun 3, 2017

thabetx Jun 3, 2017

bjorn Jun 3, 2017

thabetx Jun 3, 2017

bjorn Jun 3, 2017

thabetx Jun 3, 2017 •

edited

Loading

thabetx Jun 4, 2017

bjorn Jun 3, 2017

bjorn Jun 3, 2017

bjorn Jun 3, 2017

bjorn Jun 3, 2017

bjorn Jun 3, 2017

bjorn Jun 3, 2017

thabetx commented Jun 4, 2017 •

edited

Loading

thabetx commented Jun 4, 2017

thabetx commented Jun 7, 2017

bjorn left a comment

bjorn Jun 7, 2017

bjorn Jun 7, 2017

bjorn Jun 7, 2017

bjorn Jun 7, 2017


		MapObject *object = new MapObject(name, type, {0, 0}, size);

		readObjectData(object);


		QString fileName = QLatin1String("templateGroup.ttx");

		TemplateGroupDocument *templateGroupDocument = new TemplateGroupDocument(templateGroup, fileName);

Reusable object templates #1587

Reusable object templates #1587

Conversation

thabetx commented Jun 1, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thabetx Jun 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thabetx commented Jun 4, 2017 • edited Loading

thabetx commented Jun 4, 2017

thabetx commented Jun 7, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thabetx Jun 3, 2017 •

edited

Loading

thabetx commented Jun 4, 2017 •

edited

Loading