-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Added a new type of map object representing points #1799
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks great already!
You raise an interesting set of questions that have bothered me as well already since a while. I don't like how the rendering of all the shapes, and now also the pin, it tied up into the MapRenderer
interface. In fact I don't ever expect the objects to look a whole lot different, so I think rather their rendering should be shared and probably in the MapObjectItem
. Only the isometric map renderer does anything special, and I guess this particular transformation could be abstracted instead of abstracting the whole rendering.
But rather than changing this for all existing object rendering, maybe we could start with the pins. Especially since they aren't subject to transformations, they should be an easy case. So you could try to put the pin rendering in MapObjectItem
. The remaining problem is placing the selection marker, which I guess could be done by exposing the bounding rect calculation so that it can be used by objectBounds
.
The downside of putting the rendering in MapObjectItem
would be that the rendering code is no longer accessible by the tmxviewer
tool shipping with Tiled. Maybe MapObjectItem
should just call some rendering helper functions that we place in libtiled
.
src/tiled/objectselectiontool.cpp
Outdated
const bool hasResizableItem = std::any_of(objects.begin(), objects.end(), [](MapObject *object) { | ||
return canResize(object); | ||
}); | ||
const bool showHandles = hasSelection && hasResizableItem && (mAction == NoAction || mAction == Selecting); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the resize handles should also show as long as more than 1 objects are selected. Of course, resizing would only change the position of point objects (seems to be already working).
src/tiled/objectselectiontool.cpp
Outdated
@@ -858,7 +875,9 @@ void ObjectSelectionTool::updateHandles(bool resetOriginIndicator) | |||
return; | |||
|
|||
const QList<MapObject*> &objects = mapDocument()->selectedObjects(); | |||
const bool showHandles = objects.size() > 0; | |||
const bool showHandles = objects.size() > 0 && std::any_of(objects.begin(), objects.end(), [](MapObject *object) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess objects.size() > 1 || (objects.size() > 0 && ... )
src/tiled/objectselectiontool.cpp
Outdated
const bool showHandles = objects.size() > 0; | ||
const bool showHandles = objects.size() > 0 && std::any_of(objects.begin(), objects.end(), [](MapObject *object) { | ||
return canResize(object); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually canResize
could be passed into std::any_of
directly, right?
src/libtiled/orthogonalrenderer.cpp
Outdated
10 + extraSpace + 1); | ||
-10 - extraSpace, | ||
10 + extraSpace + 1, | ||
10 + extraSpace + 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please leave the indentation intact here and below.
src/libtiled/mapobject.h
Outdated
{ return mShape == Polygon || mShape == Polyline; } | ||
inline bool MapObject::hasDimensions() const | ||
{ | ||
switch(mShape) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coding style: please put a space after switch
.
One thing I forgot to mention, in order to fix the autobuilds (and compiling for me), please add new files also to the Qbs project (I'd also recommend using the Qbs project file, I'd like to get rid of the qmake one (#1484)). |
Last two commits clean up the duplicate functionality between renderers and bounding box functions. I ended up using MapRenderer as the home for the draw/shape utility functions, this makes them available to the renderer subclasses and also to the tmxrasterizer. |
Questions about the remaining items:
|
For localization I do a string-freeze about two weeks before the release, which is when the .ts files are re-generated and all translators get a message asking them to start updating the translation. You don't have to do anything.
You could definitely help updating the manual and you could do so as part of this PR. When I released Tiled 1.0 the manual unfortunately didn't really exist, which is why there is no 1.0-version of the manual hosted. Now the manual covers a lot of things, and many of them are already 1.1-specific. What I'll do upon release is to host both the 1.1 manual (which I could make the default) as well as master, so that manual updates can always go along with the changes without confusing readers using the Tiled releases. This is easy to do now that we are hosted by readthedocs.org. I should be able to get to another review on Tuesday. |
* When placing point objects, allow changing the position while holding the mouse button, like when placing tile objects and template instances. * Prevent changing rotation of point objects through the Properties view. * Prevent switching to rotation mode when clicking a single selected point object (it wouldn't show the handles, but it would toggle the origin indicator).
Great work, @agersant! I only found some minor issues and decided to just fix them instead of going through another review round. |
Opening the PR while working on this so implementation can be discussed.
One thing I am not happy with at the moment is the various boundingBox functions. I don't think I am implementing this right. In my latest commits, I implemented OrthogonalRenderer::drawMapObject to draw pins. I then proceeded to update the bounding box functions like this:
objectBounds
inobjectselectionitem.cpp
. As far as I can tell, this function is the one used to draw the animated selection rectangle when the item is highlighted. I put some magic numbers which match the pin drawing code I wrote in OrthogonalRenderer. I haven't looked at the other renderers yet but I'm worried they will need identical pin drawing functions too. Where should the pin drawing code and the constants live?OrthogonalRenderer::boundingRect
. This seems to be used to invalidate regions and repaint , so it needs a bit of padding (extraSpace
) around the actual bounding box. Also uses the constants from the drawing code. Is this also used to position the resize handles? The handles end up a little bit removed from where they should be because of the padding. This doesn't happen for other object types so I must be doing something bad.OrthogonalRenderer::shape
. As far as I could tell, this one is used for mouse interactions and should match the visuals closely. (ie. identical to objectBounds from objectselectionitem). Also needs the drawing constants.WIP screenshot:
Progress:
Tool name localization