Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds Autocrop feature #1574

Merged
merged 4 commits into from Jun 3, 2017
Merged

Adds Autocrop feature #1574

merged 4 commits into from Jun 3, 2017

Conversation

@ketanhwr
Copy link
Contributor

@ketanhwr ketanhwr commented May 21, 2017

This addresses #642. The option is available in the 'Map' menu and is available only when a Tile Layer is selected. The implementation is pretty straightforward:

  • First compute the (minX, minY) and (maxX, maxY) by checking all the cells of the current layer.
  • Resize the map using this region.
  • Works similar to Crop to Selection, except, the region is calculated using TileLayer::computeAutocropRegion()
Copy link
Contributor

@Ablu Ablu left a comment

added some comments for now

src/libtiled/tilelayer.cpp Outdated Show resolved Hide resolved
src/tiled/mainwindow.cpp Outdated Show resolved Hide resolved
if (bounds.isNull())
return;

resizeMap(bounds.size(), -bounds.topLeft(), true);

This comment has been minimized.

@Ablu

Ablu May 21, 2017
Contributor

Hm... The boundary check only considers tiles... @bjorn Is it fine to silently delete objects (the third parameter) which are not in the area here?

This comment has been minimized.

@ketanhwr

ketanhwr May 21, 2017
Author Contributor

The option of Crop to Selection behaves in a similar way. It also deletes the objects outside the selected region. I took that under consideration.

This comment has been minimized.

@bjorn

bjorn May 21, 2017
Owner

Yes, I think it's reasonable for this to be consistent with "Crop to Selection". There isn't really an opportunity to ask the user here, unless we'd introduce a popup, which I don't really want to.

src/tiled/mapdocument.cpp Outdated Show resolved Hide resolved
src/tiled/mapdocument.cpp Outdated Show resolved Hide resolved
src/tiled/mapdocument.cpp Outdated Show resolved Hide resolved
src/tiled/mapdocumentactionhandler.cpp Outdated Show resolved Hide resolved
}
} else {
mActionSelectNone->setEnabled(false);
mActionAutocrop->setEnabled(false);

This comment has been minimized.

@bjorn

bjorn May 21, 2017
Owner

It will be more readable to put one line below together with mActionCropToSelection like this:

mActionAutocrop->setEnabled(currentLayer && currentLayer->isTileLayer());

The mActionSelectNone needed all this special code only because the condition on whether it should be enabled or not varies depending on the type of the selected layer, but let's not "abuse" all those cases for setting the state of mActionAutocrop.

This comment has been minimized.

@ketanhwr

ketanhwr May 21, 2017
Author Contributor

Nice idea 🙂

src/tiled/mapdocumentactionhandler.h Outdated Show resolved Hide resolved
Copy link
Owner

@bjorn bjorn left a comment

Apart from two small nitpicks about whitespace, this change is ready to go in.

I will wait with merging it until I have created the 1.0 branch, since due to the string freeze this feature will go to Tiled 1.1.

Thanks!

src/tiled/mainwindow.cpp Outdated Show resolved Hide resolved
src/tiled/mapdocument.cpp Outdated Show resolved Hide resolved
@ketanhwr
Copy link
Contributor Author

@ketanhwr ketanhwr commented Jun 3, 2017

@bjorn I think you forgot this one 🙂

@bjorn bjorn merged commit ac307f1 into bjorn:master Jun 3, 2017
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjorn
Copy link
Owner

@bjorn bjorn commented Jun 3, 2017

@ketanhwr Indeed, I haven't created an 1.0 branch yet, but I implicitly ended the string freeze by merging #1580, so this one can go in now as well. Thanks for the reminder! :-)

thabetx added a commit to thabetx/tiled that referenced this pull request Jun 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.