Apply collision to multiple tiles, autodetect tile extents #1960
Conversation
robinmacharg
changed the title
There was a problem hiding this comment.
Thanks for working on this and sorry for getting to doing the review so late!
I wrote a lot of feedback on the change and I hope you will find time to make adjustments, since it would certainly be nice to have this functionality included in the next Tiled release.
Since I made some changes in the meantime that are relevant for this patch (even if there are no conflicts now), you should rebase your changes on top of my latest master branch before continuing work on the patch. At the same time you should try to get rid of those two strange merge commits.
Of course, your change is really two changes and I'd have preferred two separate PRs for them. But since both changes are relatively small and they affect the same area I'm fine with leaving that as it is for now.
|
Thanks for the detailed feedback; it's really appreciated. This is my
first time contributing to a C++ project so your comments will not only
help make Tiled better, but me as well. Work's hectic for the next week or
two (contracting, deadlines, etc. but nothing compared to an Always a
Deadline new child, I know this) but I'll work through the comments and get
back to you when I can.
Git remains a devil to me - amazingly powerful but with the potential for
idiot mistakes like mine; I'll try and fix this with the next PR.
Thanks,
Robin
…On Fri, 20 Jul 2018 at 17:05, Thorbjørn Lindeijer ***@***.***> wrote:
***@***.**** requested changes on this pull request.
Thanks for working on this and sorry for getting to doing the review so
late!
I wrote a lot of feedback on the change and I hope you will find time to
make adjustments, since it would certainly be nice to have this
functionality included in the next Tiled release.
Since I made some changes in the meantime that are relevant for this patch
(even if there are no conflicts now), you should rebase your changes on top
of my latest master branch before continuing work on the patch. At the
same time you should try to get rid of those two strange merge commits.
Of course, your change is really two changes and I'd have preferred two
separate PRs for them. But since both changes are relatively small and they
affect the same area I'm fine with leaving that as it is for now.
------------------------------
In src/tiled/tilecollisiondock.cpp
<#1960 (comment)>:
> @@ -127,6 +136,51 @@ TileCollisionDock::~TileCollisionDock()
setTile(nullptr);
}
+/**
+ * @brief TileCollisionDock::autoDetectMask
+ *
+ * Automatically detect the extents of the tile and append a simple rectanglular collision mask.
+ */
+void TileCollisionDock::autoDetectMask()
+{
+ // Iterate over the pixels, looking for empty rows
+ // sourced from: https://stackoverflow.com/a/3722160/2431627
+ QImage image = mTile->image().toImage();
+ int left = image.width(), right = 0, top = image.height(), bottom = 0;
This is really a rectangle, and could be initialized like:
QRect content = image.rect();
------------------------------
In src/tiled/tilecollisiondock.cpp
<#1960 (comment)>:
> + bool rowFilled = false;
+ for (int x = 0; x < image.width(); ++x) {
+ if (qAlpha(row[x])) {
+ rowFilled = true;
+ right = std::max(right, x);
+ if (left > x) {
+ left = x;
+ x = right; // shortcut to only search for new right bound from here
+ }
+ }
+ }
+ if (rowFilled) {
+ top = std::min(top, y);
+ bottom = y;
+ }
+ }
Though actually, after doing some digging in how the QGraphicsPixmapItem
determines its shape, the following would do the same:
QRect content = QRegion(mTile->image().createHeuristicMask()).boundingRect();
It does involve the allocation of a temporary QBitmap and the
construction of a QRegion, but I think it will not matter much for
performance and it keeps this code a lot shorter.
------------------------------
In src/tiled/tilecollisiondock.cpp
<#1960 (comment)>:
> + right = std::max(right, x);
+ if (left > x) {
+ left = x;
+ x = right; // shortcut to only search for new right bound from here
+ }
+ }
+ }
+ if (rowFilled) {
+ top = std::min(top, y);
+ bottom = y;
+ }
+ }
+
+ // Create a group for collision objects if none exists
+ if (!mTile->objectGroup()) {
+ mTile->setObjectGroup(new ObjectGroup);
You can't just directly modify the tile, since that way the operation
cannot be undone.
For the undo system to work in the tile collision editor, the undo stack
of the mDummyMapDocument should be used. This dummy map is guaranteed to
already have an object layer (see TileCollisionDock::setTile). You'd only
need to use something like the following to add the object:
auto objectGroup = static_cast<ObjectGroup*>(mDummyMapDocument->map()->layerAt(1));
auto command = new AddMapObjects(mDummyMapDocument, objectGroup, newObject);
mDummyMapDocument->undoStack()->push(command);
That way the UI will also update automatically, since that is triggered by
the undo command.
(Note that you'll need to rebase your changes, since AddMapObjects used
to be called AddMapObject and could only add one object, but I've
recently changed that.)
------------------------------
In src/tiled/abstractobjecttool.cpp
<#1960 (comment)>:
> @@ -454,6 +523,15 @@ void AbstractObjectTool::showContextMenu(MapObject *clickedObject,
QAction *removeAction = menu.addAction(tr("Remove %n Object(s)", "", selectedObjects.size()),
this, SLOT(removeObjects()));
+ // Allow the currently selected collision mask to be applied to all selected tiles in the tileset editor
+ if (auto document = DocumentManager::instance()->currentDocument()) {
+ if (auto tilesetDocument = qobject_cast<TilesetDocument*>(document)) {
+ if (tilesetDocument->selectedTiles().count() > 1) {
+ menu.addAction(tr("Apply Collision(s) to Selection"), this, SLOT(applyCollisionsToSelection()));
This is a very hidden location in the UI and somewhat of a strange place
also code-wise. Of course I understand the latter naturally followed from
the wish to put this action in the context menu.
I think I'd rather see this action as a button alongside this magic wand
button you've added to the tile collision editor. There it is easier to
discover and we don't need this somewhat hacky check to see if the user is
editing a tileset. Of course, it will be a nice challenge to consider a
good icon and probably we need to give it an elaborate tool tip to explain
the functionality.
------------------------------
In src/tiled/tilecollisiondock.cpp
<#1960 (comment)>:
> @@ -80,6 +81,13 @@ TileCollisionDock::TileCollisionDock(QWidget *parent)
CreateObjectTool *polygonObjectsTool = new CreatePolygonObjectTool(this);
CreateObjectTool *templatesTool = new CreateTemplateTool(this);
+ // Autodetection of tile extents
+ QIcon autoDetectMaskIcon(QLatin1String(":images/22x22/stock-tool-fuzzy-select-22.png"));
+ QAction *autoDetectMask = new QAction(this);
+ autoDetectMask->setText(tr("Detect Shape"));
This icon and text really suggests a more advanced functionality than
actually implemented. I think the icon should include a rectangle and the
text should say "Autodetect Collision Box".
------------------------------
In src/tiled/abstractobjecttool.cpp
<#1960 (comment)>:
> + if (Tile *currentTile = dynamic_cast<Tile *>(tilesetDocument->currentObject())) {
+
+ // Ask the user if they want to append or overwrite the collision mask
+ QMessageBox msgBox;
+ msgBox.setIcon(QMessageBox::Information);
+ msgBox.setWindowTitle(tr("Overwrite Collision Masks?"));
+ msgBox.setText(tr("Should Collision Masks be appended to, or replace, selected tiles' masks?"));
+ msgBox.addButton(tr("Append"), QMessageBox::ActionRole);
+ QPushButton *overwriteBtn = msgBox.addButton(tr("Replace"), QMessageBox::ActionRole);
+ QPushButton *abortButton = msgBox.addButton(QMessageBox::Cancel);
+ msgBox.setDefaultButton(overwriteBtn);
+ msgBox.exec();
+
+ if (msgBox.clickedButton() != abortButton) {
+
+ QList<Tile *>selectedTiles = tilesetDocument->selectedTiles();
Since we're not going to change the list of selected tiles, this can be:
const auto &selectedTiles = tilesetDocument->selectedTiles();
Or even just:
for (Tile *tile : tilesetDocument->selectedTiles()) {
------------------------------
In src/tiled/abstractobjecttool.cpp
<#1960 (comment)>:
> + msgBox.setText(tr("Should Collision Masks be appended to, or replace, selected tiles' masks?"));
+ msgBox.addButton(tr("Append"), QMessageBox::ActionRole);
+ QPushButton *overwriteBtn = msgBox.addButton(tr("Replace"), QMessageBox::ActionRole);
+ QPushButton *abortButton = msgBox.addButton(QMessageBox::Cancel);
+ msgBox.setDefaultButton(overwriteBtn);
+ msgBox.exec();
+
+ if (msgBox.clickedButton() != abortButton) {
+
+ QList<Tile *>selectedTiles = tilesetDocument->selectedTiles();
+
+ // Add each collision object to each selected tile, as long as the tile is not the current one
+ for (Tile* tile : selectedTiles) {
+
+ // Ignore the currently selected tile
+ if (tile != currentTile) {
To avoid writing a lot of code with large indentation, I prefer to this
this check as follows:
if (tile == currentTile)
continue;
------------------------------
In src/tiled/abstractobjecttool.cpp
<#1960 (comment)>:
> @@ -246,6 +247,74 @@ void AbstractObjectTool::removeObjects()
mapDocument()->removeObjects(mapDocument()->selectedObjects());
}
+/**
+ * @brief AbstractObjectTool::applyCollisionsToSelection
+ *
+ * Add the selected collision masks for the currently selected tile - the last selected tile - to
+ * all selected tiles.
+ */
+void AbstractObjectTool::applyCollisionsToSelection()
+{
+ // The selected Collision Masks
+ QList<MapObject*> selectedObjects = mapDocument()->selectedObjects();
+
+ if (auto document = DocumentManager::instance()->currentDocument()) {
+ if (auto tilesetDocument = qobject_cast<TilesetDocument*>(document)) {
+ if (Tile *currentTile = dynamic_cast<Tile *>(tilesetDocument->currentObject())) {
Avoid indenting the whole function by doing early-outs for these basic
sanity checks:
Tile *currentTile = dynamic_cast<Tile *>(tilesetDocument->currentObject());if (!currentTile)
return;
------------------------------
In src/tiled/abstractobjecttool.cpp
<#1960 (comment)>:
> @@ -246,6 +247,74 @@ void AbstractObjectTool::removeObjects()
mapDocument()->removeObjects(mapDocument()->selectedObjects());
}
+/**
+ * @brief AbstractObjectTool::applyCollisionsToSelection
+ *
+ * Add the selected collision masks for the currently selected tile - the last selected tile - to
+ * all selected tiles.
+ */
+void AbstractObjectTool::applyCollisionsToSelection()
+{
+ // The selected Collision Masks
+ QList<MapObject*> selectedObjects = mapDocument()->selectedObjects();
Since we're not going to modify the list of selected objects, we can avoid
the extra reference count (and detaching, as will happen when iterating the
non-const implicit copy later on):
const QList<MapObject*> &selectedObjects = mapDocument()->selectedObjects();
------------------------------
In src/tiled/abstractobjecttool.cpp
<#1960 (comment)>:
> +
+ QList<Tile *>selectedTiles = tilesetDocument->selectedTiles();
+
+ // Add each collision object to each selected tile, as long as the tile is not the current one
+ for (Tile* tile : selectedTiles) {
+
+ // Ignore the currently selected tile
+ if (tile != currentTile) {
+
+ // If we should overwrite, then remove all objects prior to adding the new ones
+ if (msgBox.clickedButton() == overwriteBtn) {
+ if (tile->objectGroup()) {
+ for (MapObject *object : tile->objectGroup()->objects()) {
+ tile->objectGroup()->removeObject(object);
+ }
+ tile->objectGroup()->resetObjectIds();
This is pointless after removing all objects. There's nothing left to
reset.
------------------------------
In src/tiled/abstractobjecttool.cpp
<#1960 (comment)>:
> + // Ignore the currently selected tile
+ if (tile != currentTile) {
+
+ // If we should overwrite, then remove all objects prior to adding the new ones
+ if (msgBox.clickedButton() == overwriteBtn) {
+ if (tile->objectGroup()) {
+ for (MapObject *object : tile->objectGroup()->objects()) {
+ tile->objectGroup()->removeObject(object);
+ }
+ tile->objectGroup()->resetObjectIds();
+ }
+ }
+
+ // Create a group for collision objects if none exists
+ if (!tile->objectGroup()) {
+ tile->setObjectGroup(new ObjectGroup);
We can't modify the tiles directly since that breaks the undo system. What
we need to do here is to clone the existing object group (or create a new
one, if there is none or if we should overwrite) and then push a
ChangeTileObjectGroup undo command onto the tilesetDocument->undoStack().
See TileCollisionDock::applyChanges for an example (except we don't need
to change mApplyingChanges).
------------------------------
In src/tiled/abstractobjecttool.cpp
<#1960 (comment)>:
> + }
+ tile->objectGroup()->resetObjectIds();
+ }
+ }
+
+ // Create a group for collision objects if none exists
+ if (!tile->objectGroup()) {
+ tile->setObjectGroup(new ObjectGroup);
+ }
+
+ // Copy across the selected collision mask shapes
+ for (MapObject* object : selectedObjects) {
+ MapObject* newObject = new MapObject;
+ newObject->copyPropertiesFrom(object);
+ QPointF position = QPointF(object->position().x(), object->position().y());
+ newObject->setPosition(position);
This should do the trick as well:
MapObject *newObject = object->clone();
newObject->resetId();
------------------------------
In src/tiled/abstractobjecttool.cpp
<#1960 (comment)>:
> + QList<MapObject*> selectedObjects = mapDocument()->selectedObjects();
+
+ if (auto document = DocumentManager::instance()->currentDocument()) {
+ if (auto tilesetDocument = qobject_cast<TilesetDocument*>(document)) {
+ if (Tile *currentTile = dynamic_cast<Tile *>(tilesetDocument->currentObject())) {
+
+ // Ask the user if they want to append or overwrite the collision mask
+ QMessageBox msgBox;
+ msgBox.setIcon(QMessageBox::Information);
+ msgBox.setWindowTitle(tr("Overwrite Collision Masks?"));
+ msgBox.setText(tr("Should Collision Masks be appended to, or replace, selected tiles' masks?"));
+ msgBox.addButton(tr("Append"), QMessageBox::ActionRole);
+ QPushButton *overwriteBtn = msgBox.addButton(tr("Replace"), QMessageBox::ActionRole);
+ QPushButton *abortButton = msgBox.addButton(QMessageBox::Cancel);
+ msgBox.setDefaultButton(overwriteBtn);
+ msgBox.exec();
This prompt is going to get annoying fast. At the least it should only
happen when any of the selected tiles already has some collision shapes.
But if this action is moved to a button on the tool bar in the collision
editor, then alternatively the button could also be turned into a forced
menu button using QToolButton::InstantPopup that presents both options
before triggering the action.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1960 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABrXM6zKOJrfXl6L038tx9T-AQL-7oalks5uIf_EgaJpZM4UrXXb>
.
|
|
I'm glad the detailed review is appreciated, makes it worth the time! :-)
Alright, there's no rush! I do hope to be able to release Tiled 1.2 in about three weeks, but this might be too optimistic anyway.
Ah, I remember giving up on Git when I tried it for the first time instead of Subversion. Only switched back to it a year later after also having used Mercurial, but since then I had never looked back and love its speed and flexibility. :-) Be sure to force-push into this PR (just keep using your |
|
This would be so very helpful! It's quite tedious adding the same collision shape to hundreds or thousands of tiles. |
While that is certainly true, this was really not the intended use-case for the tile collision editor. If you have some trivial collision shape (possibly just filling the whole tile with a rectangle), it would be much faster and convenient to set a custom property like "collides: true" on those tiles. If you select all the tiles you can set this custom property at once on all of them. |
|
That's certainly true. Good point. Silly me, I even used to set a collision boolean on tiles back like six years ago before Tiled had collision polygons -_-. |
|
@bjorn Apologies for leaving this hanging. I've gained some Git(hub)-fu in the intervening couple of years and might even have some time to implement the changes you requested. With the changes that have taken place in the last couple of years with Tiled is this proposed feature still valid and wanted/required? |
bjorn
force-pushed
the
ApplyCollisionToMultipleTiles
branch
from
29a9054 to
2a6f2bb
Compare
|
@robinmacharg No problem about leaving it hanging and glad to hear you're still considering to finish this change! Indeed it is still relevant since no one else addressed #1322 so far. To get you started I've just rebased your work onto the latest |
|
@bjorn Thanks. Glad to hear it's still potentially a useful addition, and thanks for the rebase. New job, new laptop, new Qt install etc. so may take a couple of days to get back up to speed. |
|
@bjorn Any estimate as to when this feature will be merged and released? This remains the most painful aspect of using Tiled for me. :( I have lots of tilesets with many tile variants and creating collision for them takes many hours of work... This feature would be such a timesaver! |
|
@tliron Since @robinmacharg appears to be busy elsewhere I can definitely consider picking this one up. I'll have a look on Tuesday to see how much work is remaining. |
You're very polite! Yet another new job started today, following a fairly intense 6 months in the last one. I can only apologise; I had the best of intentions but simply didn't have (and likely won't have), the time to follow this up. |
…elected tiles Issue mapeditor#1322
Correct copying of properties, position, rotation. Correct application of mask ID. Context menu entry only appears if >1 tile is selected.
bjorn
force-pushed
the
ApplyCollisionToMultipleTiles
branch
from
2a6f2bb to
c3bb14d
Compare
* Reduced code nesting * Always display "Apply Collision(s) to Selected Tiles" action for discoverability, but only enable when more than one tile is selected. * Use QPixmap::mask for auto-detecting a basic collision box.
|
I've rebased the changes to the latest
Apart from that, it would still be nice if you could just select multiple tiles and change all their collision shapes at once, but this is outside the scope of this change and adds the additional problem of handling the case where the selected tiles have different shapes set on them. I'll look into the above issues next. |
Also made sure the "Detect Bounding Box" button is disabled when no tile is selected.
Also, select the auto-detected bounding box object after adding it.
|
All done and will be available in today's development snapshot. It took me about 6 hours of work. Please considering sponsoring me if this feature is useful to you, to ensure I have the time to make similar improvements. Thanks! |
|
I wanted to test this and built from master, but am getting "corrupt data layer" errors for my project. I guess some formats have changed since the last release? |
|
@tliron Hmm, what version were you using before and which file format are you using? I think there actually isn't any change that would be expected to cause such errors. Can you still open the files fine in the previous version you were using? Note that you don't need to build it yourself. If you install the latest "snapshot" build on itch.io (see this news post) then you should see these new actions. |
|
@bjorn Thanks! The snapshot on itch.io works fine with my existing files and also has the new features. I'm not sure why my self-built version was different, possibly different libraries on my OS (Fedora 32). |
Adds two small collision mask editor features requested in issue #1322:
A context menu entry that applies the selected mask(s) in the current tile (i.e. the last selected one) to all selected tiles. Offers the user the choice of appending to existing masks or replacing existing ones.
An additional tool on the collision mask editor toolbar that autodetects the (rectangular) extents of the tile and creates a collision mask automatically. It uses the existing magic wand icon since the intent feels similar enough to the traditional image-related use.