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 tools to create and modify bezier objects #776

Open
wants to merge 14 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@zilluss

zilluss commented Sep 27, 2014

The following things are still missing:

  • Icons for the toolbar
  • Shortcuts for the "create bezier*" tools
  • Translations
  • Export to formats other than JSON
@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Sep 28, 2014

Owner

Translations

They will come with the string freeze for 0.11, no need for that here.

Export to formats other than JSON

It seems you've actually already implemented TMX as well. That only leaves the Lua format, which can be added later.

About the storage format though, I had based the polygon and polyline elements on the SVG specification and thought it may be good to do the same here. That would mean saving this to a path element, for example a simple cubic curve:

<path d="M0,0 C0,100 100,100 100,0"/>

This means, first move to position 0,0 (these coordinates would be relative to the object's position), then do a cubic curve to 100,0 using 0,100 and 100,100 as the two control points.

This format does not seem overly hard to parse to me nor should it be a problem to write it out. Of course, we do not need to support the full standard immediately but extending the support later would anyway be a possibility.

While trying out the patch I found some problems:

  • Clickable regions for selecting / moving bezier objects seemed to be not working properly.
  • The control points and their lines are not positioned correctly when objects are rotated.
  • Lines to the control points are not painted correctly when they extend beyond the bounding box of the object.
  • When moving bezier object nodes, I would expect their control points to move along with them.
  • Objects complete change shape when flipped along their X or Y axis (actions available in the context menu).

Overall though, this is a really promising feature and I think you've done a good job!

Owner

bjorn commented Sep 28, 2014

Translations

They will come with the string freeze for 0.11, no need for that here.

Export to formats other than JSON

It seems you've actually already implemented TMX as well. That only leaves the Lua format, which can be added later.

About the storage format though, I had based the polygon and polyline elements on the SVG specification and thought it may be good to do the same here. That would mean saving this to a path element, for example a simple cubic curve:

<path d="M0,0 C0,100 100,100 100,0"/>

This means, first move to position 0,0 (these coordinates would be relative to the object's position), then do a cubic curve to 100,0 using 0,100 and 100,100 as the two control points.

This format does not seem overly hard to parse to me nor should it be a problem to write it out. Of course, we do not need to support the full standard immediately but extending the support later would anyway be a possibility.

While trying out the patch I found some problems:

  • Clickable regions for selecting / moving bezier objects seemed to be not working properly.
  • The control points and their lines are not positioned correctly when objects are rotated.
  • Lines to the control points are not painted correctly when they extend beyond the bounding box of the object.
  • When moving bezier object nodes, I would expect their control points to move along with them.
  • Objects complete change shape when flipped along their X or Y axis (actions available in the context menu).

Overall though, this is a really promising feature and I think you've done a good job!

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Sep 28, 2014

Owner

Icons for the toolbar

I've created the others and can add some in similar style for the new tools. It's basically just taking that arrow and adding the shape as it appears in Tiled itself.

Owner

bjorn commented Sep 28, 2014

Icons for the toolbar

I've created the others and can add some in similar style for the new tools. It's basically just taking that arrow and adding the shape as it appears in Tiled itself.

@zilluss

This comment has been minimized.

Show comment
Hide comment
@zilluss

zilluss Sep 28, 2014

I was expecting that a few things might not work correctly, and some problems were introduced when I added the undo/split/delete/join functionalities. I'll fix the problems you've mentioned.

zilluss commented Sep 28, 2014

I was expecting that a few things might not work correctly, and some problems were introduced when I added the undo/split/delete/join functionalities. I'll fix the problems you've mentioned.

@zilluss

This comment has been minimized.

Show comment
Hide comment
@zilluss

zilluss Oct 6, 2014

The previous problems have been fixed. I would appreciate it if someone could test the new version if he or she spots any new problems I didn't catch.

zilluss commented Oct 6, 2014

The previous problems have been fixed. I would appreciate it if someone could test the new version if he or she spots any new problems I didn't catch.

Show outdated Hide outdated src/libtiled/mapobject.cpp Outdated
@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Oct 6, 2014

Indentation is wrong starting from this line (missing a space).

bjorn commented on src/tiled/editpolygontool.cpp in 50d50de Oct 6, 2014

Indentation is wrong starting from this line (missing a space).

Show outdated Hide outdated src/libtiled/isometricrenderer.cpp Outdated
Show outdated Hide outdated src/libtiled/isometricrenderer.cpp Outdated
Show outdated Hide outdated src/libtiled/orthogonalrenderer.cpp Outdated
Show outdated Hide outdated src/tiled/editpolygontool.cpp Outdated
Show outdated Hide outdated src/tiled/editpolygontool.cpp Outdated
Show outdated Hide outdated src/tiled/editpolygontool.cpp Outdated
Show outdated Hide outdated src/tiled/editpolygontool.cpp Outdated
@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Oct 6, 2014

Owner

Thanks for the fixes! These two problems I wrote before remain:

  • Clickable regions for selecting / moving bezier objects seemed to be not working properly.
  • Lines to the control points are not painted correctly when they extend beyond the bounding box of the object.
Owner

bjorn commented Oct 6, 2014

Thanks for the fixes! These two problems I wrote before remain:

  • Clickable regions for selecting / moving bezier objects seemed to be not working properly.
  • Lines to the control points are not painted correctly when they extend beyond the bounding box of the object.
@zilluss

This comment has been minimized.

Show comment
Hide comment
@zilluss

zilluss Oct 11, 2014

  • This problem seems to occur when the bezier segment is straight. In that case, the mMapScene->items(pos) call returns no object. The problem can be alleviated by using a rectangle instead of a point to find the items under the mouse, but this affects all the other object too.
  • I didn't notice the problem with the control point lines, because it only happens when OpenGL acceleration is turned off. I'm currently investigating the issue.

zilluss commented Oct 11, 2014

  • This problem seems to occur when the bezier segment is straight. In that case, the mMapScene->items(pos) call returns no object. The problem can be alleviated by using a rectangle instead of a point to find the items under the mouse, but this affects all the other object too.
  • I didn't notice the problem with the control point lines, because it only happens when OpenGL acceleration is turned off. I'm currently investigating the issue.
@zilluss

This comment has been minimized.

Show comment
Hide comment
@zilluss

zilluss Oct 13, 2014

Unfortunately, the commit was immediately pushed.

zilluss commented Oct 13, 2014

Unfortunately, the commit was immediately pushed.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Oct 13, 2014

Owner

Unfortunately, the commit was immediately pushed.

Well, you can still squash the commit locally into the previous one and force-push the new version to your beziertool branch. If you're on your local beziertool branch, you would do git rebase -i HEAD^^, change the second line to have the word fixup instead of pick at the start, save and close the editor. Of course, if you're new to this you should be extra careful to understand what you're doing.

Owner

bjorn commented Oct 13, 2014

Unfortunately, the commit was immediately pushed.

Well, you can still squash the commit locally into the previous one and force-push the new version to your beziertool branch. If you're on your local beziertool branch, you would do git rebase -i HEAD^^, change the second line to have the word fixup instead of pick at the start, save and close the editor. Of course, if you're new to this you should be extra careful to understand what you're doing.

@Darky-Lucera

This comment has been minimized.

Show comment
Hide comment
@Darky-Lucera

Darky-Lucera Dec 3, 2014

Sure, we can do it because we can draw a line, and we can aproximate any continuous curve drawing little lines. In this case using moveTo and lineTo.

Furthermore we can draw closed spline curves (is similar to draw polygons instead of polylines).

Darky-Lucera commented Dec 3, 2014

Sure, we can do it because we can draw a line, and we can aproximate any continuous curve drawing little lines. In this case using moveTo and lineTo.

Furthermore we can draw closed spline curves (is similar to draw polygons instead of polylines).

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Dec 3, 2014

Owner

@Darky-Lucera Ok, I worry a little about the performance and the quality in that case, but sure it could be done if you manage to find the right number of lines to draw. Anyway, it's a different issue so let's not further pollute this pull request. :-)

The support for bezier curves is not actually in yet, but I am having trouble finding time to get around to another round of testing and code review. Also, I'm not that happy with the way the data is stored, on the C++ side as well as in the TMX format (see my first comment about that). So either @zilluss or me shall have to consider alternatives and possibly implement them. Due to these concerns I'm also leaning towards first releasing Tiled 0.11 with the hexagonal map feature before merging this in.

Owner

bjorn commented Dec 3, 2014

@Darky-Lucera Ok, I worry a little about the performance and the quality in that case, but sure it could be done if you manage to find the right number of lines to draw. Anyway, it's a different issue so let's not further pollute this pull request. :-)

The support for bezier curves is not actually in yet, but I am having trouble finding time to get around to another round of testing and code review. Also, I'm not that happy with the way the data is stored, on the C++ side as well as in the TMX format (see my first comment about that). So either @zilluss or me shall have to consider alternatives and possibly implement them. Due to these concerns I'm also leaning towards first releasing Tiled 0.11 with the hexagonal map feature before merging this in.

@Darky-Lucera

This comment has been minimized.

Show comment
Hide comment
@Darky-Lucera

Darky-Lucera Dec 4, 2014

It saddens me to hear this but you are the project manager ;)

I think tiled is not a CAD/CAM program, it's just a wonderful tool to make maps for our games, so the quality of the curve will be enough (we must adjust this quality in our games, here we need to see an aproximation about how it will look), and nowadays you will not worry about the performance of drawing some tens (or hundreds, or thousands) of lines to aproximate a curve.

With respect to the format, I would prefer something like that:

<object name="cell" type="membrane" x="3104" y="896">
  <path type="bezier" points="0,0 1,1 ..." controlPoints="0,1 1,2 ..." />
  or
  <path type="cardinal" points="0,0 1,1 ..." tensions="0,1,..." />
  or
  <path type="tcb" points="0,0 1,1 ..." tensions="0,1,..." continuities="..." biases="..." />
</object>

Do we create a new feature request to add support for splines?

Darky-Lucera commented Dec 4, 2014

It saddens me to hear this but you are the project manager ;)

I think tiled is not a CAD/CAM program, it's just a wonderful tool to make maps for our games, so the quality of the curve will be enough (we must adjust this quality in our games, here we need to see an aproximation about how it will look), and nowadays you will not worry about the performance of drawing some tens (or hundreds, or thousands) of lines to aproximate a curve.

With respect to the format, I would prefer something like that:

<object name="cell" type="membrane" x="3104" y="896">
  <path type="bezier" points="0,0 1,1 ..." controlPoints="0,1 1,2 ..." />
  or
  <path type="cardinal" points="0,0 1,1 ..." tensions="0,1,..." />
  or
  <path type="tcb" points="0,0 1,1 ..." tensions="0,1,..." continuities="..." biases="..." />
</object>

Do we create a new feature request to add support for splines?

@zilluss

This comment has been minimized.

Show comment
Hide comment
@zilluss

zilluss Dec 4, 2014

@Darky-Lucera I haven't had QT experience before this PR either 😄. Having looked at the other kind of splines, they would definitely be a nice addition and easier to implement than beziers, because without control points you can basically reuse most of the polygon code. As for the format, Thorbjørn wants to stick with the SVG standard which probably makes the more sense for most users.

@bjorn I've had a quick glance over the QT source for the beziers and as far as I can tell, QT splits the bezier into lines as well. Also, during debugging I noticed that QT is rather lazy when it comes to redrawing so I think they probably use some kind of offscreen buffer for the paths.
I will change the TMX representation when I find the time. I'd schedule the change of the internal representation for a later refactoring, but that's up to you to decide.

zilluss commented Dec 4, 2014

@Darky-Lucera I haven't had QT experience before this PR either 😄. Having looked at the other kind of splines, they would definitely be a nice addition and easier to implement than beziers, because without control points you can basically reuse most of the polygon code. As for the format, Thorbjørn wants to stick with the SVG standard which probably makes the more sense for most users.

@bjorn I've had a quick glance over the QT source for the beziers and as far as I can tell, QT splits the bezier into lines as well. Also, during debugging I noticed that QT is rather lazy when it comes to redrawing so I think they probably use some kind of offscreen buffer for the paths.
I will change the TMX representation when I find the time. I'd schedule the change of the internal representation for a later refactoring, but that's up to you to decide.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Dec 13, 2014

Owner

Internal and external data storage aside, I can't merge this pull request mainly because there are still click area issues. Especially for the bezierloop I'm having trouble finding the clickable areas when trying to drag around the objects. This is a problem that needs to be looked into. Also I still see repainting issues when dragging around the control point handles, but that is a more minor issue.

Owner

bjorn commented Dec 13, 2014

Internal and external data storage aside, I can't merge this pull request mainly because there are still click area issues. Especially for the bezierloop I'm having trouble finding the clickable areas when trying to drag around the objects. This is a problem that needs to be looked into. Also I still see repainting issues when dragging around the control point handles, but that is a more minor issue.

@Darky-Lucera

This comment has been minimized.

Show comment
Hide comment
@Darky-Lucera

Darky-Lucera Dec 14, 2014

If you generate a polygon for the curve, that I think will be faster to draw, you can easily solve the click area issue. This algorithm is known for many years:
http://www.ecse.rpi.edu/Homepages/wrf/Research/Short_Notes/pnpoly.html

Here is the code, for reference. Excluding lines with only braces, there are only 7 lines of code.

int pnpoly(int nvert, float *vertx, float *verty, float testx, float testy)
{
  int i, j, c = 0;
  for (i = 0, j = nvert-1; i < nvert; j = i++) {
    if ( ((verty[i]>testy) != (verty[j]>testy)) &&
     (testx < (vertx[j]-vertx[i]) * (testy-verty[i]) / (verty[j]-verty[i]) + vertx[i]) )
       c = !c;
  }
  return c;
}
  • W. Randolph Franklin

Enjoy it!

Darky-Lucera commented Dec 14, 2014

If you generate a polygon for the curve, that I think will be faster to draw, you can easily solve the click area issue. This algorithm is known for many years:
http://www.ecse.rpi.edu/Homepages/wrf/Research/Short_Notes/pnpoly.html

Here is the code, for reference. Excluding lines with only braces, there are only 7 lines of code.

int pnpoly(int nvert, float *vertx, float *verty, float testx, float testy)
{
  int i, j, c = 0;
  for (i = 0, j = nvert-1; i < nvert; j = i++) {
    if ( ((verty[i]>testy) != (verty[j]>testy)) &&
     (testx < (vertx[j]-vertx[i]) * (testy-verty[i]) / (verty[j]-verty[i]) + vertx[i]) )
       c = !c;
  }
  return c;
}
  • W. Randolph Franklin

Enjoy it!

@bjorn bjorn added this to the 0.12.0 milestone Dec 30, 2014

void mouseMovedWhileCreatingObject(const QPointF &pos,
Qt::KeyboardModifiers modifiers, bool snapToGrid, bool snapToFineGrid);
void mousePressedWhileCreatingObject(QGraphicsSceneMouseEvent *event, bool snapToGrid, bool snapToFineGrid);
void mouseReleasedWhileCreatingObject(QGraphicsSceneMouseEvent *event, bool snapToGrid, bool snapToFineGrid);

This comment has been minimized.

@bjorn

bjorn Feb 8, 2015

Owner

Even though there are no merge conflicts, I broke this tool because I changed these function signatures in commit cef4129. Sorry about that.

@bjorn

bjorn Feb 8, 2015

Owner

Even though there are no merge conflicts, I broke this tool because I changed these function signatures in commit cef4129. Sorry about that.

@zilluss

This comment has been minimized.

Show comment
Hide comment
@zilluss

zilluss Feb 9, 2015

No problem, I'll fix it when I take care of the click area issue. I hope I'll get to fix it soon.

zilluss commented Feb 9, 2015

No problem, I'll fix it when I take care of the click area issue. I hope I'll get to fix it soon.

@bjorn bjorn added 1 - Ready and removed 1 - Ready labels Feb 22, 2015

@bjorn bjorn modified the milestones: 0.12.0, 0.13.0 May 4, 2015

@bjorn bjorn modified the milestones: Tiled 0.13, Tiled 0.14 Jun 4, 2015

@bjorn bjorn modified the milestone: Tiled 0.14 Aug 10, 2015

@Guard13007

This comment has been minimized.

Show comment
Hide comment
@Guard13007

Guard13007 Apr 28, 2017

So how close or not close to being added to Tiled is this? I didn't realize this wasn't a feature already and need it for a project I was considering Tiled for. :/

Guard13007 commented Apr 28, 2017

So how close or not close to being added to Tiled is this? I didn't realize this wasn't a feature already and need it for a project I was considering Tiled for. :/

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Apr 28, 2017

Owner

So how close or not close to being added to Tiled is this? I didn't realize this wasn't a feature already and need it for a project I was considering Tiled for. :/

Two years ago I planned to get around this, but in the end I wanted to get Tiled 1.0 out and this feature wasn't part of that plan. This pull request was never in a finished state and I think substantial work is still required to get it done.

Right now, Tiled 1.0 is really around the corner (I plan to release it towards the end of next month) and after that it is time to think about how to prioritize feature development for 1.1, 1.2, etc. This is certainly one of the features I'd like to get around to, but so are many others and I just have 2 full days per week for Tiled development (which aren't fully funded yet). In addition, in June-August we'll be mentoring several (hopefully) students working on Tiled as part of the Google Summer of Code, but there is no project related to this feature specifically.

So short answer is, normally this feature could be somewhere between 4-8 months away. At best you could become a major patron and I could prioritize it right after the Tiled 1.0 release. But I could imagine a better alternative for you could be to rely on another tool, like Inkscape.

Owner

bjorn commented Apr 28, 2017

So how close or not close to being added to Tiled is this? I didn't realize this wasn't a feature already and need it for a project I was considering Tiled for. :/

Two years ago I planned to get around this, but in the end I wanted to get Tiled 1.0 out and this feature wasn't part of that plan. This pull request was never in a finished state and I think substantial work is still required to get it done.

Right now, Tiled 1.0 is really around the corner (I plan to release it towards the end of next month) and after that it is time to think about how to prioritize feature development for 1.1, 1.2, etc. This is certainly one of the features I'd like to get around to, but so are many others and I just have 2 full days per week for Tiled development (which aren't fully funded yet). In addition, in June-August we'll be mentoring several (hopefully) students working on Tiled as part of the Google Summer of Code, but there is no project related to this feature specifically.

So short answer is, normally this feature could be somewhere between 4-8 months away. At best you could become a major patron and I could prioritize it right after the Tiled 1.0 release. But I could imagine a better alternative for you could be to rely on another tool, like Inkscape.

@Guard13007

This comment has been minimized.

Show comment
Hide comment
@Guard13007

Guard13007 Apr 29, 2017

Inkscape doesn't do what I need. I'll be making my own map editor to solve the problem (I needed an excuse to work on my GUI lib anyhow). I'd love to fund the development, but I'm a poor student haha.

Thank you for the detailed reply and explanation!

Guard13007 commented Apr 29, 2017

Inkscape doesn't do what I need. I'll be making my own map editor to solve the problem (I needed an excuse to work on my GUI lib anyhow). I'd love to fund the development, but I'm a poor student haha.

Thank you for the detailed reply and explanation!

@IoriBranford

This comment has been minimized.

Show comment
Hide comment
@IoriBranford

IoriBranford Feb 22, 2018

Do you guys remember what's missing on this? Maybe a checklist so more of us could help out.

IoriBranford commented Feb 22, 2018

Do you guys remember what's missing on this? Maybe a checklist so more of us could help out.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Feb 22, 2018

Owner

Do you guys remember what's missing on this? Maybe a checklist so more of us could help out.

Please read up on the discussion above. I think there are still clickability issues and the storage format comments also still apply. Besides that, the changes should be rebased onto the latest master branch to resolve the conflicts.

Owner

bjorn commented Feb 22, 2018

Do you guys remember what's missing on this? Maybe a checklist so more of us could help out.

Please read up on the discussion above. I think there are still clickability issues and the storage format comments also still apply. Besides that, the changes should be rebased onto the latest master branch to resolve the conflicts.

@zilluss

This comment has been minimized.

Show comment
Hide comment
@zilluss

zilluss Feb 25, 2018

I will have a look at it this week.

zilluss commented Feb 25, 2018

I will have a look at it this week.

@bjorn

This comment has been minimized.

Show comment
Hide comment
@bjorn

bjorn Mar 7, 2018

Owner

I will have a look at it this week.

I've afraid this code has changed quite a lot since you made this pull request, but it's great that you'd like to update it! Feel free to ask questions if you're confused by some of the conflicts.

Owner

bjorn commented Mar 7, 2018

I will have a look at it this week.

I've afraid this code has changed quite a lot since you made this pull request, but it's great that you'd like to update it! Feel free to ask questions if you're confused by some of the conflicts.

@zilluss

This comment has been minimized.

Show comment
Hide comment
@zilluss

zilluss Mar 22, 2018

Yeah I tried a merge, but it's simpler to just look at the current master and the changes I made and re-do the changes on top of the current master. It's surprisingly little code anyway.

zilluss commented Mar 22, 2018

Yeah I tried a merge, but it's simpler to just look at the current master and the changes I made and re-do the changes on top of the current master. It's surprisingly little code anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment