Enable replacing an object with a template #1793

thabetx · 2017-10-21T16:44:36Z

Works exactly like the replace tile feature.

I had to move the selected template from the template creation tool into the tool manager so it can be used across all the tools, just like the tile.

bjorn

@thabetx Thank you so much for coming back to this! Overall your change looks very good. I've added a number of comments.

Usability wise, I think a minor improvement could be to mention the template name in the menu item, like Replace with "Rock" Template in the case of your demonstration above.

bjorn · 2017-10-23T10:18:35Z

src/tiled/abstractobjecttool.cpp

+{
+    mapDocument()->undoStack()->push(new ReplaceObjectsWithTemplate(mapDocument(),
+                                                                    mapDocument()->selectedObjects(),
+                                                                    {objectTemplate()->templateGroup(), objectTemplate()->id()}));


Please use the new templateRef method you added here. :-)

For some reason I didn't commit that 😅.

bjorn · 2017-10-23T10:20:15Z

src/tiled/abstracttool.cpp

@@ -22,6 +22,7 @@
 #include "abstracttool.h"

 #include "mapdocument.h"
+#include "templatesdock.h"


It doesn't look like you need this include here?

bjorn · 2017-10-23T10:21:08Z

src/tiled/changemapobject.cpp

@@ -204,6 +204,20 @@ void DetachObjects::undo()
    emit mMapDocument->mapObjectModel()->objectsChanged(mMapObjects);
 }

+void copyProperties(const MapObject *source, MapObject *target) {


Coding style: please place the { on its own line for function definitions.

Please mark this function static.

Could this potentially be implemented as MapObject::operator=(const MapObject &other), so it could be done like *target = *source? Or does this have different semantics than assignment? At least having it in MapObject would make it easier not to forget adding more things to it later.

Using the = operator looks like a good idea, it copies everything except id and position so I think i's okay but 'm not sure.

Hmm, I guess operator= could be expected to copy those as well, so probably better to leave this as a special function.

bjorn · 2017-10-23T11:03:31Z

src/tiled/changemapobject.cpp

+    for (int i = 0; i < mMapObjects.size(); ++i) {
+        MapObject *current = mMapObjects.at(i);
+        MapObject *old = mOldMapObjects.at(i);
+        copyProperties(old, current);;


Please remove second ;

bjorn · 2017-10-23T11:04:37Z

src/tiled/changemapobject.cpp

+
+
+ReplaceObjectsWithTemplate::ReplaceObjectsWithTemplate(MapDocument *mapDocument,
+                                                      const QList<MapObject *> &mapObjects,


Indentation missing a space.

bjorn · 2017-10-23T11:06:19Z

src/tiled/changemapobject.cpp

    }

    emit mMapDocument->objectsChanged(mMapObjects);
    emit mMapDocument->selectedObjectsChanged();
 }
+
+ReplaceObjectsWithTemplate::~ReplaceObjectsWithTemplate()


Please define the destructor right after the constructor (matching the order in the header file).

bjorn · 2017-10-23T11:09:55Z

src/tiled/mapdocument.cpp

@@ -139,7 +139,7 @@ void MapDocument::saveSelectedObject(const QString &name, int groupIndex)

    if (ObjectTemplate *objectTemplate = model->saveObjectToDocument(object, name, groupIndex)) {
        // Convert the saved object into an instance and clear the changed properties flags
-        object->setTemplateRef({objectTemplate->templateGroup(), objectTemplate->id()});
+        object->setTemplateRef(objectTemplate->templateRef());
        object->setChangedProperties(0);


Actually these lines should be covered by an undo command, right? Now that you've written ReplaceObjectsWithTemplate, you should probably use it here.

Hmm, currently this would be a partial undo, as removing from the template group is not implemented. But at least it would reset the object to being a normal object.

Yeah, I think partial undo would be fine here.

thabetx · 2017-10-28T12:30:29Z

I like the option to add the name of the template in the context menu. It makes things clearer.

bjorn · 2017-10-31T10:34:07Z

src/tiled/changemapobject.cpp

+    target->setShape(source->shape());
+    target->setCell(source->cell());
+    target->setRotation(source->rotation());
+    target->setVisible(source->VisibleProperty);


I just noticed this little issue. I won't change it now on master to avoid conflicts, so please fix this to source->isVisible() as part of this PR.

thabetx · 2017-11-04T14:37:24Z

I cleaned the code and made some fixes and changes.

Replacing was enabled for tile objects only, so I fixed that as well, but now when replacing a non-tile object e.g. a polygon with a template, Tiled will crash. I think the reason is that the two objects have different sets of properties and when the properties dock is asked to update it can't set the properties of the new object.

So I will be looking into it and will do some more testing.

bjorn · 2017-11-04T21:13:35Z

Replacing was enabled for tile objects only, so I fixed that as well, but now when replacing a non-tile object e.g. a polygon with a template, Tiled will crash. I think the reason is that the two objects have different sets of properties and when the properties dock is asked to update it can't set the properties of the new object.

Ah, you're right. This new Replace by Template action is the first action that allows changing the actual type of an object, and some properties are only created for certain objects (like flipping for tile objects, and width/height only for rectangles, tiles and ellipses). So the PropertyBrowser will need some changes for this.

bjorn · 2017-11-04T21:18:41Z

src/tiled/abstractobjecttool.cpp

+    auto replaceTemplateAction = menu.addAction(tr("Replace With Template"), this, SLOT(replaceObjectsWithTemplate()));
+
+    if (selectedTemplate)
+        replaceTemplateAction->setText(tr("Replace With \"") + selectedTemplate->name() + tr("\" Template"));


This is not nice for the translator. You should instead use a parameter, like so:

tr("Replace With \"%1\" Template").arg(selectedTemplate->name())

Conflicts: src/libtiled/objecttemplate.h src/tiled/createtemplatetool.cpp src/tiled/mapdocument.cpp src/tiled/mapscene.cpp

bjorn · 2017-11-21T14:50:43Z

Alright, I've merged master and in the process fixed that translation string and corrected ReplaceObjectsWithTemplate::redo.

The remaining issue is the crashing when changing the kind of object through replacing it with a template.

By replacing an object with a template, it is possible to change whether it has dimensions, or whether it is a tile or a text object. This affects which properties should be listed. The easiest way to adjust the list of properties is to refresh it entirely when necessary.

thabetx · 2017-11-29T19:33:24Z

Hello bjron, thank you for the updates, I was stuck at updating the properties and the custom properties. I tested it and it's working great.

I will try working on #1823 .

Again thank you :).

bjorn · 2017-11-29T19:35:51Z

Hello bjorn, thank you for the updates, I was stuck at updating the properties and the custom properties. I tested it and it's working great.

No problem, I just needed this to be finished since I'm trying to move forward with a 1.1 release, which means finishing up the loose ends.

I will try working on #1823.

Ah, I was about to look into it myself too, especially since I introduced this one recently, but feel free to have a look into it. It's probably not a hard one to fix.

thabetx · 2017-11-29T19:48:51Z

Okay you can continue looking into it, I will try improving broken links.

Undoing after fixing doesn't reset and I will also look into fixing broken tileset link inside the template #1732 .

thabetx added 2 commits October 21, 2017 13:22

Add helper function to create TemplateRef from ObjectTemplate

7b17bbe

Enable replacing an object with a template

f19a5cc

thabetx force-pushed the replace-template branch from e9fef58 to f19a5cc Compare October 22, 2017 18:15

bjorn requested changes Oct 23, 2017

View reviewed changes

bjorn reviewed Oct 31, 2017

View reviewed changes

Fixes and cleanup

747e0ab

bjorn reviewed Nov 4, 2017

View reviewed changes

bjorn mentioned this pull request Nov 8, 2017

Refer to templates directly (by file name) #1813

Merged

Merge branch 'master' into thabetx-replace-template

dbd7240

Conflicts: src/libtiled/objecttemplate.h src/tiled/createtemplatetool.cpp src/tiled/mapdocument.cpp src/tiled/mapscene.cpp

bjorn merged commit 6404b3d into mapeditor:master Nov 29, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable replacing an object with a template #1793

Enable replacing an object with a template #1793

thabetx commented Oct 21, 2017

bjorn left a comment

bjorn Oct 23, 2017

thabetx Oct 27, 2017

bjorn Oct 23, 2017

bjorn Oct 23, 2017

thabetx Oct 28, 2017

bjorn Oct 28, 2017

bjorn Oct 23, 2017

bjorn Oct 23, 2017

bjorn Oct 23, 2017

bjorn Oct 23, 2017

thabetx Oct 27, 2017

bjorn Oct 31, 2017

thabetx commented Oct 28, 2017

bjorn Oct 31, 2017

thabetx commented Nov 4, 2017

bjorn commented Nov 4, 2017

bjorn Nov 4, 2017

thabetx Nov 4, 2017

bjorn commented Nov 21, 2017

thabetx commented Nov 29, 2017

bjorn commented Nov 29, 2017

thabetx commented Nov 29, 2017



		ReplaceObjectsWithTemplate::ReplaceObjectsWithTemplate(MapDocument *mapDocument,
		const QList<MapObject *> &mapObjects,

Enable replacing an object with a template #1793

Enable replacing an object with a template #1793

Conversation

thabetx commented Oct 21, 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

Choose a reason for hiding this comment

thabetx commented Oct 28, 2017

Choose a reason for hiding this comment

thabetx commented Nov 4, 2017

bjorn commented Nov 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn commented Nov 21, 2017

thabetx commented Nov 29, 2017

bjorn commented Nov 29, 2017

thabetx commented Nov 29, 2017