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

Automapping Tile Objects #890

Closed
wants to merge 3 commits into from
Closed

Automapping Tile Objects #890

wants to merge 3 commits into from

Conversation

Seanba
Copy link
Contributor

@Seanba Seanba commented Feb 18, 2015

Tile Objects can be added to Object Layers through the automapping
mechanism.
Also, fixed bug in "DeleteTiles" functionality with respect to Objects
(tiled or otherwise).

Tile Objects can be added to Object Layers through the automapping
mechanism.
Also, fixed bug in "DeleteTiles" functionality with respect to Objects
(tiled or otherwise).
@bjorn
Copy link
Member

bjorn commented Feb 18, 2015

@stefanbeller Do you have time to review this?


// Note: intersects returns false in some conditions so check if corners are contained
const QRect& objRect = obj->bounds().toAlignedRect();
if (where.intersects(objRect) || where.contains(objRect.topLeft()) || where.contains(objRect.bottomLeft()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, intersects should only return false if the intersection is empty. This happens also when either rectangle is empty, even though one is contained in the other. So, it should be enough to check only one corner (in addition to the intersects).

@bjorn bjorn added 1 - Ready and removed 1 - Ready labels Feb 22, 2015
Map Objects are removed from automapping when "DeleteTiles" is used.
Code was already there for this but was comparing tile space with map
space.
Tile Objects can be added to Object Layers through automapping.

Tile Objects having a bottom-left origin and zero width and height was
problematic for automapping.
I've added a convenience function MapObject::boundsUseTile() that will
use the tile, if present, to build the boundary for a given Tile Object.
(Perhaps that function could be better named?)
Note: it's possible that the added functionaliy of
MapObject::boundsUseTile() could just be rolled into
MapObject::bounds(), but I didn't want to (potentially) break legacy
code that may have relied on Tile Objects having an empty bounds. For
instance, do we want Tile Objects to be written out to TMX with non-zero
width and height now?
@Seanba
Copy link
Contributor Author

Seanba commented Feb 22, 2015

I am putting together another pull request. Thanks for the feedback.

@Seanba Seanba closed this Feb 22, 2015
@Seanba
Copy link
Contributor Author

Seanba commented Feb 22, 2015

I don't know what I'm doing with Github. :/
Re-opening this pull request with modified files.

@Seanba Seanba reopened this Feb 22, 2015
@Seanba
Copy link
Contributor Author

Seanba commented Feb 24, 2015

Not sure what's with that build failure. Is it because I had briefly closed the pull request?

@bjorn
Copy link
Member

bjorn commented Feb 24, 2015

Not sure what's with that build failure. Is it because I had briefly closed the pull request?

That's what the AppVeyor page indicates, yes. Nothing to worry about.

@@ -42,7 +43,22 @@ void eraseRegionObjectGroup(MapDocument *mapDocument,
// tile objects. polygons and polylines are not covered correctly by this
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does the comment here need updating?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, it does not look like it to me, because it already says the correct thing is done for tile objects. The patch seems to make the comment more correct than before.

@bjorn
Copy link
Member

bjorn commented Mar 29, 2015

I've pushed this fix as commit 521a40a, thanks!

@bjorn bjorn closed this Mar 29, 2015
@Seanba
Copy link
Contributor Author

Seanba commented Mar 30, 2015

Awesome. Thanks, Bjorn.

On Sun, Mar 29, 2015 at 9:27 AM, Thorbjørn Lindeijer <
notifications@github.com> wrote:

I've pushed this fix as commit 521a40a
521a40a,
thanks!


Reply to this email directly or view it on GitHub
#890 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants