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

Shape Fill Tool #1689

Merged
merged 4 commits into from
Aug 29, 2017
Merged

Shape Fill Tool #1689

merged 4 commits into from
Aug 29, 2017

Conversation

Bdtrotte
Copy link
Contributor

Allows users to click and drag to fill an area with a selected fill method and polygon.

@Bdtrotte
Copy link
Contributor Author

Still needs some graphics updates. Generally at this stage I'm looking for feed back on the general flow of the tool.

@bjorn
Copy link
Member

bjorn commented Aug 21, 2017

Please don't merge master into your branch, but rather rebase your branch on top of master.

@bjorn
Copy link
Member

bjorn commented Aug 21, 2017

I've tried this out and while I think it's interesting, I am having trouble thinking of the use-case for the custom polygon shape filling. Where did you get this idea from? I personally think supporting rectangle and circle shapes should be enough. It's a bit unfortunate since I can see a huge amount of effort has been put into it, but I had no idea you were planning to implement this.

I found it a bit surprising that the shapes are stored in the map file. What was the thought behind this? I would probably put them in the Tiled settings.

The ellipse seems to be of somewhat low quality, or at the very least it isn't symmetrical (I wonder how QRegion::Ellipse is implemented...). Check out the pointsOnEllipse function in geometry.cpp for a function generating a nicely shaped ellipse, though it returns a list of points. It could probably be copied and adapted to return a region.

For the rectangular filling mode, I think it would be interesting to add another option for doing a "border fill". Often when painting walls, carpets or other rectangular stuff with tiles, you have 4 corner tiles and 4 edge tiles and then one or more tiles to fill the center. It would be cool to have the option to tile the stamp this way.

Copy link
Member

@bjorn bjorn left a comment

Choose a reason for hiding this comment

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

Just two small remarks.

src/tiled/shapefilltool.cpp Outdated Show resolved Hide resolved
src/tiled/geometry.cpp Outdated Show resolved Hide resolved
@bjorn
Copy link
Member

bjorn commented Aug 28, 2017

I think the circle drawing is still acting strange, but I couldn't get it to play nice after playing around with it for a while. In the end I had this:

    case Circle: {
        int radiusX = boundingRect.width() / 2;
        int radiusY = boundingRect.height() / 2;
        mFillRegion = ellipseRegion(boundingRect.left() + radiusX,
                                    boundingRect.top() + radiusY,
                                    boundingRect.left() + radiusX * 2,
                                    boundingRect.top() + radiusY * 2);
        break;
    }

Which at least stopped it from wiggling on the left and top sides, but the weird thing is that this would refuse to draw square circles, though in fact the Stamp Brush has no problems drawing square circles. Maybe you have an idea what's going on?

@Bdtrotte
Copy link
Contributor Author

Strange... When I added in the code it still draws square circles. Also I think the wiggling has just moved to the right and bottom sides (which is noticeable when dragging to the left or above the first corner). We could just make the center point the point where you click so it acts like the stamp tool.

mFillRegion = ellipseRegion(mStartCorner.x(),
                            mStartCorner.y(),
                            mStartCorner.x() + width,
                            mStartCorner.y() + height);

@bjorn
Copy link
Member

bjorn commented Aug 29, 2017

We could just make the center point the point where you click so it acts like the stamp tool.

I think that's a reasonable solution. I've pushed that change and rebased on latest master, to resolve the conflict in the tiled.qrc file.

@bjorn bjorn merged commit dcacd23 into mapeditor:master Aug 29, 2017
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.

2 participants