Template instances #1623

thabetx · 2017-06-23T16:22:25Z

This is the third checkpoint in the reusable object templates project, it will be mainly concerned with reading template instance in a map.

The format will look like this

 <templategroup firsttid="1" source="group1.ttx"/>
 <templategroup firsttid="11" source="group2.ttx"/>

 <object id="66" tid="2" x="191" y="342"/>

The instance in the map file would be the same as an object except that it would have an extra attribute similar to the gid called the tid standing for template id. Template ids would work like tileset ids, the map assigns the firsttid to each template group and the template manager would resolve those ids.

If a template object has properties they will override the template.

Create TemplatesManager to aid in mapping.
Create the tid mapping functionality.
Handle reading errors.
Write template group references inside the map.
Write template instances inside the map.
Reduce redundant data when writing.

Currently the pull request is focused on reading an instance but may be expanded to also cover creation.

Also add fileName member to the TemplateGroup

thabetx · 2017-06-26T12:12:50Z

The code can now read template instances and load them into the map. I will do more testing and handle possible errors.

Will also handle some reading errors.

Also added a nextTemplateId property to the template to be used when saving or adding a new template.

bjorn

Looks good overall, but I found some issues.

bjorn · 2017-06-30T19:21:18Z

src/libtiled/mapobject.h

+
+        CellProperty             = 1 << 8,
+        SizeProperty             = 1 << 9,
+        shapeProperty            = 1 << 10


Naming: ShapeProperty

bjorn · 2017-06-30T19:22:57Z

src/libtiled/mapwriter.cpp

    }

-    if (id != 0)
+    if (id != 0 && !isTemplateInstance)


Template instances do still need their ID.

bjorn · 2017-07-03T13:29:50Z

src/libtiled/tidmapper.cpp

+      if (it.value() == templateGroup)
+          return it.key();
+    }
+    return 0;


This function can be written as:

return mFirstTidToTemplateGroup.key(templateGroup);

It should also be marked const.

bjorn · 2017-07-03T13:31:20Z

src/libtiled/tidmapper.cpp

+
+ObjectTemplate *TidMapper::tidToTemplate(unsigned tid, bool &ok) const
+{
+    ObjectTemplate *objectTemplate;


Should be initialized to nullptr, to ensure a consistent return value in case of failure in addition to setting ok to false.

bjorn · 2017-07-03T13:31:54Z

src/libtiled/tidmapper.cpp

+            objectTemplate = templateGroup->templateAt(index);
+        } else {
+            ok = false;
+            objectTemplate = nullptr;


When properly initialized, we don't need to set it to nullptr here.

bjorn · 2017-07-03T14:27:31Z

src/libtiled/mapwriter.cpp

+        w.writeAttribute(QLatin1String("tid"), QString::number(tid));
+    }
+
+    if (id != 0 && !isTemplateInstance)


Template instances need to write their ID as well.

bjorn · 2017-07-03T14:32:58Z

src/libtiled/mapwriter.cpp

        w.writeAttribute(QLatin1String("visible"), QLatin1String("0"));

-    writeProperties(w, mapObject.properties());
+    if (!isTemplateInstance)


Properties should always be written, since this will only include overridden values and any properties only added to the instance.

bjorn · 2017-07-03T14:50:17Z

src/tiled/objecttemplatemodel.h

-inline void ObjectTemplateModel::setTemplateDocuments(const TemplateDocuments &templateDocuments)
-{ mTemplateDocuments = templateDocuments; }
+inline void ObjectTemplateModel::setTemplateDocuments(TemplateDocuments &templateDocuments)
+{ mTemplateDocuments = &templateDocuments; }


mTemplateDocuments should not be a pointer. Rather, just store the list here by value and when you need it in the TemplatesDock, have it call ObjectTemplateModel::templateDocuments and remove TemplatesDock::mTemplateDocuments. The function should take const TemplateDocuments &templateDocuments.

Also, since this affects the model, it should call beginResetModel and endResetModel, even though currently the function may only be called while the model is not yet used by a view, this could change later on.

It should probably also own the documents, so it should call qDeleteAll(mTemplateDocuments) before the assignment, and this should be added to the destructor (instead of doing it in TemplatesDock).

bjorn · 2017-07-03T14:51:09Z

src/tiled/objecttemplatemodel.h


 inline const TemplateDocuments ObjectTemplateModel::templateDocuments() const
-{ return mTemplateDocuments; }
+{ return *mTemplateDocuments; }


This function should return const TemplateDocuments &.

bjorn · 2017-07-03T14:53:43Z

src/tiled/templatesdock.cpp

+    manager->setTemplateGroups(templateGroups);
+}
+
+TemplatesDock::~TemplatesDock() {


Coding style: { should be on the next line.

bjorn · 2017-07-04T13:12:33Z

src/tiled/templatesdock.h

@@ -49,6 +52,7 @@ private slots:
    void retranslateUi();

    TemplatesView *mTemplatesView;
+    TemplateDocuments mTemplateDocuments;


It seems you forgot to remove this member, and the above include?

bjorn

Some comments.

bjorn · 2017-07-04T20:30:24Z

src/tiled/flipmapobjects.cpp

+        mMapObjects[i]->flip(mFlipDirection, mObjectsCenter);
+
+        mMapObjects[i]->setPropertyChanged(MapObject::CellProperty, mNewCellStates[i]);
+        mMapObjects[i]->setPropertyChanged(MapObject::RotationProperty, mNewCellStates[i]);


mNewCellStates should be mNewRotationStates

bjorn · 2017-07-04T20:30:51Z

src/libtiled/map.cpp

+        }
+    }
+
+    delete oldTemplateGroup;


Maybe somebody should delete the old template group, but it's not the map.

bjorn · 2017-07-04T20:32:19Z

src/tiled/brokenlinks.cpp

+    case TilesetTileImageSource:
+        return nullptr;
+    case TemplateGroupReference:
+        return _templateGroup;


Well, that is a lot of lines for writing:

return type == TemplateGroupReference ? _templateGroup : nullptr;

:-)

bjorn · 2017-07-04T20:33:28Z

src/tiled/brokenlinks.cpp

+                    link._templateGroup = templateGroup;
+                    mBrokenLinks.append(link);
+                } else {
+                    processTemplateGroup(templateGroup);


This else makes no sense, because it does essentially the same as the code above, just without the check on the file name being empty (which should be there, I guess).

bjorn · 2017-07-04T20:41:03Z

src/tiled/brokenlinks.cpp

@@ -404,6 +460,7 @@ void BrokenLinksWidget::selectionChanged()
    mLocateButton->setEnabled(!selection.isEmpty());

    bool isTileset = qobject_cast<TilesetDocument*>(mBrokenLinksModel->document()) != nullptr;
+    bool isTemplateGroup = qobject_cast<TemplateGroupDocument*>(mBrokenLinksModel->document()) != nullptr;


The broken links model document can never be a template group document, since we don't currently have a template group editor.

bjorn · 2017-07-04T20:41:30Z

src/tiled/brokenlinks.cpp

+            if (isTemplateGroup)
+                mLocateButton->setText(tr("Locate File..."));
+            else
+                mLocateButton->setText(tr("Open Template Group..."));


The button should always read "Locate File..." at this point, since we can't open template groups directly.

bjorn · 2017-07-06T08:48:24Z

@thabetx Looks like your last commit should go to the 1.0 branch. :-)

thabetx · 2017-07-06T11:43:51Z

Okay, I will test it on master.

bjorn

Numerous small comments but overal I think it's looking pretty solid. Let's aim for merging this as soon as possible so we can have a new PR for the actual instantiation.

bjorn · 2017-07-10T13:19:00Z

src/libtiled/map.cpp

+    return true;
+}
+
+QList<Tiled::MapObject *> Map::replaceTemplateGroup(TemplateGroup *oldTemplateGroup, TemplateGroup *newTemplateGroup)


MapObject does not need to be fully qualified here.

bjorn · 2017-07-10T13:32:22Z

src/libtiled/mapobject.cpp

@@ -33,6 +33,7 @@
 #include "map.h"
 #include "objectgroup.h"
 #include "tile.h"
+#include "templatemanager.h"


I think this can include templategroup.h instead of templatemanager.h, since the manager isn't actually used.

bjorn · 2017-07-10T13:41:26Z

src/libtiled/mapreader.cpp

@@ -921,41 +963,74 @@ MapObject *MapReaderPrivate::readObject()
    const QPointF pos(x, y);
    const QSizeF size(width, height);

-    MapObject *object = new MapObject(name, type, pos, size);
+    MapObject *object = new MapObject;


Maybe you could still use the previous constructor, and then just initialize the flags as follows?

object->setPropertyChanged(MapObject::NameProperty, !name.isEmpty()); object->setPropertyChanged(MapObject::TypeProperty, !type.isEmpty()); object->setPropertyChanged(MapObject::SizeProperty, !size.isEmpty());

Would just be a bit shorter.

bjorn · 2017-07-10T13:51:38Z

src/libtiled/tidmapper.cpp

+            ok = true;
+
+        if (!templateGroup->loaded())
+            templateGroup->updateMaxId(id);


Can include this in the else of the previous condition.

bjorn · 2017-07-10T13:52:00Z

src/libtiled/tidmapper.cpp

+    QMapIterator<unsigned, TemplateGroup*> it(mFirstTidToTemplateGroup);
+
+    while (it.hasNext()) {
+      it.next();


Indentation is off by 2.

bjorn · 2017-07-10T14:19:05Z

src/tiled/flipmapobjects.cpp

+    for (int i = 0; i < mMapObjects.size(); ++i) {
+        mMapObjects[i]->flip(mFlipDirection, mObjectsCenter);
+
+        mMapObjects[i]->setPropertyChanged(MapObject::CellProperty, mNewCellStates[i]);


I'm a little concerned about this. I think flipping some tile object is not actually expected to override the referenced tile, but just its flipped state. I think it's not very important to deal with this now, but it's something to keep in mind.

bjorn · 2017-07-10T14:20:38Z

src/tiled/mainwindow.cpp

+        MapDocument *mapDocument =  MapDocument::load(fileName, mapFormat, &error);
+        document = mapDocument;
+
+        bool hasNonEmbeddedTemplateGroups;


Variable is missing an initialization. Not that it matters, because the variable isn't really used. Remove?

bjorn · 2017-07-10T14:28:14Z

src/tiled/mapdocument.cpp

+    auto changedObjects = mMap->replaceTemplateGroup(oldTemplateGroup, templateGroup);
+
+    // Update the objects in the map scene
+    emit(objectsChanged(changedObjects));


Coding style: please use the empty emit macro as follows:

emit objectsChanged(changedObjects);

bjorn · 2017-07-10T14:45:48Z

src/tiled/templategroupdocument.cpp

@@ -45,7 +45,7 @@ TemplateGroupDocument::TemplateGroupDocument(TemplateGroup *templateGroup, const
    : Document(TemplateGroupDocumentType, fileName)


Should this fileName also be changed to mTemplateGroup->fileName()?

I think so. The template group should always contain the file name, so the constructor arguments can be the template group only.

bjorn · 2017-07-10T14:50:13Z

src/libtiled/mapobject.h

+        CellProperty             = 1 << 8,
+        SizeProperty             = 1 << 9,
+        ShapeProperty            = 1 << 10,
+        RotationProperty         = 1 << 11


Since you've added several entries here, I'm getting warnings in MapObject::setMapObjectProperty and MapObject::mapObjectProperty. I think size and rotation could simply be handled, but "shape" and "cell" are too special and it should probably catch those with a Q_ASSERT(false);.

Also created the sync function which synchronizes the properties of a template instance with the base template.

thabetx added 2 commits June 23, 2017 16:38

Move loaded template documents to the templates dock

af24005

Create template manager

86de94f

Also add fileName member to the TemplateGroup

thabetx force-pushed the template-instances branch 2 times, most recently from 805e8a1 to e5d790a Compare June 26, 2017 12:06

thabetx force-pushed the template-instances branch from e5d790a to 0d54859 Compare June 27, 2017 01:05

Read template instances in the map as objects

ca549f4

Will also handle some reading errors.

thabetx force-pushed the template-instances branch 3 times, most recently from 7a1fa2a to 812d0aa Compare June 27, 2017 23:23

Write used template groups when saving a map

d3fc1a5

Also added a nextTemplateId property to the template to be used when saving or adding a new template.

thabetx force-pushed the template-instances branch from 812d0aa to ee489b0 Compare June 28, 2017 08:15

Write template instances

2a45ded

thabetx force-pushed the template-instances branch 4 times, most recently from e1bf477 to 89b8aeb Compare July 1, 2017 15:37

bjorn requested changes Jul 3, 2017

View reviewed changes

thabetx force-pushed the template-instances branch from 89b8aeb to 2b26d24 Compare July 4, 2017 12:34

bjorn reviewed Jul 4, 2017

View reviewed changes

thabetx force-pushed the template-instances branch 2 times, most recently from e1f5039 to 6e4f3df Compare July 4, 2017 13:28

bjorn requested changes Jul 4, 2017

View reviewed changes

thabetx force-pushed the template-instances branch 2 times, most recently from 6f185c8 to 5172938 Compare July 5, 2017 15:17

thabetx force-pushed the template-instances branch from 2220c71 to 419444b Compare July 6, 2017 12:41

bjorn requested changes Jul 10, 2017

View reviewed changes

thabetx force-pushed the template-instances branch from a65dbd2 to eff51e6 Compare July 12, 2017 02:44

thabetx added 3 commits July 12, 2017 06:04

Introduce change flags to mark changed properties

a62a7b4

Also created the sync function which synchronizes the properties of a template instance with the base template.

Update handling of template IDs

f407e29

Move template documents ownership to the model

3a599e3

thabetx force-pushed the template-instances branch from eff51e6 to 92d06ca Compare July 12, 2017 04:24

thabetx added 3 commits July 12, 2017 13:51

Add basic broken links support for template groups

72724fc

Allow embedding new template groups on loading a map

e7ce205

Code cleanup

f6251ae

thabetx force-pushed the template-instances branch from 92d06ca to f6251ae Compare July 12, 2017 11:53

bjorn merged commit f6251ae into mapeditor:wip/templates Jul 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Template instances #1623

Template instances #1623

thabetx commented Jun 23, 2017 •

edited

thabetx commented Jun 26, 2017

bjorn left a comment

bjorn Jun 30, 2017

bjorn Jun 30, 2017

bjorn Jul 3, 2017

bjorn Jul 3, 2017

bjorn Jul 3, 2017

bjorn Jul 3, 2017

bjorn Jul 3, 2017

bjorn Jul 3, 2017

bjorn Jul 3, 2017

bjorn Jul 3, 2017

bjorn Jul 4, 2017

bjorn left a comment

bjorn Jul 4, 2017

bjorn Jul 4, 2017

bjorn Jul 4, 2017

bjorn Jul 4, 2017

bjorn Jul 4, 2017

bjorn Jul 4, 2017

bjorn commented Jul 6, 2017

thabetx commented Jul 6, 2017

bjorn left a comment

bjorn Jul 10, 2017

bjorn Jul 10, 2017

bjorn Jul 10, 2017

bjorn Jul 10, 2017

bjorn Jul 10, 2017

bjorn Jul 10, 2017

bjorn Jul 10, 2017

bjorn Jul 10, 2017

bjorn Jul 10, 2017

thabetx Jul 11, 2017

bjorn Jul 10, 2017

		@@ -45,7 +45,7 @@ TemplateGroupDocument::TemplateGroupDocument(TemplateGroup *templateGroup, const
		: Document(TemplateGroupDocumentType, fileName)

Template instances #1623

Template instances #1623

Conversation

thabetx commented Jun 23, 2017 • edited

thabetx commented Jun 26, 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

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

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

Choose a reason for hiding this comment

bjorn commented Jul 6, 2017

thabetx commented Jul 6, 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

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 23, 2017 •

edited