Animation Editor- Multiple Frame duration edit feature #1784

satu0king · 2017-10-15T05:31:18Z

Added a spinBox where the user can select a frame duration.
The Set Frame Time button has two purposes

If no Frame(s) are selected, then it sets the duration of all frames to the duration of the spinBox. It also resets the default Frame duration to that duration.
If frame(s) are selected, then it sets the duration of all those frames to the duration of the spinBox. Default Frame duration is not changed.

Issues: The undo in situation 2 works but not all at a time. Each frame update is registered separately instead of all at a once. I am not sure how to fix this. Apart from this minor bug, everything seems to be all right.

This is the first time I am contributing to open source. I am new to Tiled and QT,However I am not new to programming. Please guide me.

ketanhwr

Made some comments regarding the coding style.

ketanhwr · 2017-10-15T08:30:55Z

src/tiled/tileanimationeditor.cpp


    void addFrame(const Frame &frame);

    const Tileset *mTileset;
    QVector<Frame> mFrames;
 };

+int FrameListModel::DEFAULT_DURATION=100;


I guess you can initialise this in the constructor.

I dont think this can be done in a constructor as its a static variable ..

ketanhwr · 2017-10-15T08:31:36Z

src/tiled/tileanimationeditor.cpp

@@ -257,6 +259,13 @@ const QVector<Frame> &FrameListModel::frames() const
 {
    return mFrames;
 }
+void FrameListModel::setDefaultFrameTime(int duration)


There should be a gap between the above two lines.

Thanks, Change has been made.

ketanhwr · 2017-10-15T08:31:47Z

src/tiled/tileanimationeditor.cpp

@@ -257,6 +259,13 @@ const QVector<Frame> &FrameListModel::frames() const
 {
    return mFrames;
 }
+void FrameListModel::setDefaultFrameTime(int duration)
+{
+    DEFAULT_DURATION=duration;


Coding style: DEFAULT_DURATION = duration;

ketanhwr · 2017-10-15T08:32:47Z

src/tiled/tileanimationeditor.cpp

+void FrameListModel::setDefaultFrameTime(int duration)
+{
+    DEFAULT_DURATION=duration;
+    for(Frame &F:mFrames){


Coding style: for (Frame &F:mFrames) {

And since this is a single line statement, you can get rid of the braces.

Thanks, Change has been made.

ketanhwr · 2017-10-15T08:33:03Z

src/tiled/tileanimationeditor.cpp

+{
+    DEFAULT_DURATION=duration;
+    for(Frame &F:mFrames){
+        F.duration=duration;


Spaces around the =

Thanks, Change has been made.

ketanhwr · 2017-10-15T08:33:14Z

src/tiled/tileanimationeditor.cpp

@@ -391,6 +405,21 @@ void TileAnimationEditor::framesEdited()
                                            mFrameListModel->frames()));
    mApplyingChanges = false;
 }
+void TileAnimationEditor::setFrameTime()


A gap between the two lines.

Thanks, Change has been made.

ketanhwr · 2017-10-15T08:33:35Z

src/tiled/tileanimationeditor.cpp

+{
+    QItemSelectionModel *selectionModel = mUi->frameList->selectionModel();
+    QModelIndexList indexes = selectionModel->selectedIndexes();
+    if (indexes.isEmpty()){


Coding stye: if (indexes.isEmpty()) {

Thanks, Change has been made.

bjorn

This is the first time I am contributing to open source. I am new to Tiled and QT,However I am not new to programming. Please guide me.

Thanks for taking on this issue and welcome to open source! It's a great initiative and I sincerely hope our comments on the pull request won't put you off.

I've provided some more detailed feedback and also commented on the problem you have with the undo.

bjorn · 2017-10-17T14:53:31Z

src/.qmake.stash

@@ -0,0 +1,42 @@
+QMAKE_MAC_SDK.macosx.Path = /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk


Please don't add this file, since it's a build artifact. To help avoid this file getting added I've now pushed an explicit ignore rule for it in change 337670d.

bjorn · 2017-10-17T14:55:58Z

src/tiled/tileanimationeditor.cpp

+{
+    DEFAULT_DURATION = duration;
+    for(Frame &F:mFrames)
+        F.duration=duration;


Please put spaces according to the coding style:

for (Frame &frame : mFrames) frame.duration = duration;

(also, rename F to frame)

I find this side-effect a little strange for a function called setDefaultFrameTime. Please separate it into another function.

bjorn · 2017-10-17T14:56:39Z

src/tiled/tileanimationeditor.cpp

+            SLOT(setFrameTime()));
+
+
+


Please leave only one empty line.

bjorn · 2017-10-17T14:57:49Z

src/tiled/tileanimationeditor.cpp

+void TileAnimationEditor::setFrameTime()
+{
+    QItemSelectionModel *selectionModel = mUi->frameList->selectionModel();
+    QModelIndexList indexes = selectionModel->selectedIndexes();


Please declare as const QModelIndexList. Since you're not modifying the list this avoids potential detaching (deep-copy) of the data.

bjorn · 2017-10-17T14:58:27Z

src/tiled/tileanimationeditor.cpp

+        return;
+    }
+    for (const QModelIndex &index : indexes)
+        mFrameListModel->setData(index,mUi->frameTime->value(),Qt::EditRole);


Coding style: should have spaces after the commas.

Regarding the problem with the undo history, each call here emits dataChanged from the model, which is connected to framesEdited, which creates a ChangeTileAnimation undo command. To avoid this, you could introduce a boolean like mSuppressUndo and temporarily set it to true in this function, checking that in framesEdited and setting it to false again before calling framesEdited explicitly.

bjorn · 2017-10-17T15:01:16Z

src/tiled/tileanimationeditor.ui

+             <property name="value">
+              <number>100</number>
+             </property>
+         </widget>


You would generally use Qt Designer or the Design mode in Qt Creator for editing .ui files, but if you do it by hand, please do take care that they are formatted consistently (easiest should be to open the file in Qt Designer and save it again).

bjorn · 2017-10-17T15:01:54Z

src/tiled/tileanimationeditor.ui

+              <string>ms</string>
+             </property>
+             <property name="minimum">
+              <number>20</number>


Why is 20 ms the minimum frame time? I would just put it on 1 ms.

bjorn · 2017-10-17T15:03:04Z

src/tiled/tileanimationeditor.ui

+              <number>10000</number>
+             </property>
+             <property name="singleStep">
+              <number>20</number>


Let's use 10 ms here.

bjorn · 2017-10-17T15:16:35Z

src/tiled/tileanimationeditor.ui

@@ -20,6 +20,35 @@
   <item>
    <layout class="QHBoxLayout" name="horizontalLayout">
     <item>
+         <widget class="QSpinBox" name="frameTime">
+             <property name="suffix">
+              <string>ms</string>


This should be " ms" (starting with a space).

bjorn · 2017-10-17T15:33:22Z

src/tiled/tileanimationeditor.ui

+     <item>
+         <widget class="QPushButton" name="setFrameTimeButton">
+             <property name="text">
+              <string>Set Frame Time</string>


I think it would be nicer to use a label "Frame duration:", then the input box, and then the button with just "Apply" on it.

satu0king · 2017-12-25T13:40:48Z

Really sorry that I didn't make the changes earlier.
I fixed the undo issue the way @bjorn told to do and did all the corrections requested by @bjorn and @ketanhwr.

satu0king · 2017-12-30T13:58:00Z

Can you please check if my PR is alright, if not let me know the mistakes

bjorn

Thanks for continuing your work on this!

I've left a few more comments. Also, I think it would be good to make the default frame duration persistent (remembered across Tiled sessions). Check out how persistence is implemented in other dialogs, like the NewMapDialog.

bjorn · 2017-12-30T16:04:14Z

src/tiled/tileanimationeditor.ui

+     <item>
+      <widget class="QLabel" name="label">
+       <property name="text">
+        <string>Frame-Duration: </string>


Please change to "Frame duration:"

bjorn · 2017-12-30T16:08:05Z

src/tiled/tileanimationeditor.cpp

+    const QModelIndexList indexes = selectionModel->selectedIndexes();
+
+    if (indexes.isEmpty()) {
+        mFrameListModel->setDefaultFrameTime(mUi->frameTime->value());


I think the default animation time should change immediately when you change the value of the frameTime widget. And of course, without affecting the existing frames.

bjorn · 2017-12-30T16:09:06Z

src/tiled/tileanimationeditor.cpp

+{
+    DEFAULT_DURATION = duration;
+    for(Frame &frame : mFrames)
+        frame.duration = duration;


In my previous review I commented that I don't like this side-effect of setDefaultFrameTime. Please don't change the animation and just set the new default frame time (also this setter can be static then).

bjorn · 2017-12-30T16:11:39Z

src/tiled/tileanimationeditor.cpp

    const QVector<Frame> &frames() const;

 private:
-    static const int DEFAULT_DURATION = 100;
+    static int DEFAULT_DURATION;


This variable used by the in ALL_CAPS because it was a constant. Now that it can be changed, please rename to mDefaultDuration.

bjorn · 2017-12-30T16:21:15Z

src/tiled/tileanimationeditor.cpp

+
+    if (indexes.isEmpty()) {
+        mFrameListModel->setDefaultFrameTime(mUi->frameTime->value());
+        mUi->frameList->setFocus(); // This is necessary for the UI, It does not get updates without this


This is a hack, which works because it triggers a repaint. The reason the UI isn't updating is that setDefaultFrameTime is modifying the frame durations without emitting the dataChanged signal.

Please write a separate function like FrameListModel::applyFrameTime that sets the duration like currently setDefaultFrameTime is doing, but in addition it should emit dataChanged with appropriate indexes. Something like:

emit dataChanged(index(0), index(mFrames.length() - 1), { Qt::EditRole, Qt::DisplayRole });

(since this will only be valid when there is at least one frame, you need to early-out if mFrames.isEmpty())

I now understood why it wasn't working and why the hack worked. I am going to use the function that was written previously. It internally emits a data change signal mFrameListModel->setData()

satu0king · 2018-01-07T18:50:21Z

I think I got it right this time. I did the following things

Persistence of Frame Duration across Tiled sessions. (As implemented in NewMapDialog)
Removed side effect of setDefaultFrameTime() .
Change default time immediately on a change of frame-time widget.
Removed the hack and used data emit on changing frame time.
Name errors

@bjorn, please let me know if there are any mistakes.

bjorn · 2018-01-12T09:43:36Z

All looking good, thanks for your contribution!

I didn't close #1310 since it had an idea which I think could still be interesting to add, which would be to add the ability to add/subtract/multiply with the duration of the selected animation frames. I could see this being added by way of adding a menu to the "Apply" button with the additional actions, but in that case I think it's a little strange to combine it with the automatic change of default frame duration (maybe that should then become another item in this menu). In any case I'm a little undecided about this, but if you like you could try to fit in that functionality as well.

…ames (#1784)" This reverts commit 7971fa8. That change was not supposed to be done on the 1.1 branch, which is in string freeze.

Animation Editor- Multiple Frame duration edit feature

0fe6552

ketanhwr reviewed Oct 15, 2017

View reviewed changes

satu0king added 2 commits October 15, 2017 16:00

coding style changes

fafe356

Merge remote-tracking branch 'bjorn/master' into TileAnimationEditor

234fe64

bjorn requested changes Oct 17, 2017

View reviewed changes

satu0king added 3 commits December 17, 2017 19:40

Merge remote-tracking branch 'bjorn/master' into TileAnimationEditor

df40f3a

Merge remote-tracking branch 'bjorn/master' into TileAnimationEditor

f9f60ac

Multiple Frames Undo bug fixed

3687de3

Delete .qmake.stash

5d5ec41

bjorn requested changes Dec 30, 2017

View reviewed changes

bjorn mentioned this pull request Jan 4, 2018

#1310: Multiple Frame Duration Editing #1330

Closed

satu0king added 2 commits January 8, 2018 00:14

Removed side effect of default frame duration change

9534352

Persistence of Frame Duration

7c5a0d2

bjorn approved these changes Jan 12, 2018

View reviewed changes

bjorn merged commit 7971fa8 into mapeditor:master Jan 12, 2018

satu0king deleted the TileAnimationEditor branch January 12, 2018 15:28

bjorn added a commit that referenced this pull request Jan 30, 2018

Revert "Tile Animation Editor: Allow changing duration of multiple fr…

e5d041f

…ames (#1784)" This reverts commit 7971fa8. That change was not supposed to be done on the 1.1 branch, which is in string freeze.

		@@ -0,0 +1,42 @@
		QMAKE_MAC_SDK.macosx.Path = /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk

Animation Editor- Multiple Frame duration edit feature #1784

Animation Editor- Multiple Frame duration edit feature #1784

Conversation

satu0king commented Oct 15, 2017

ketanhwr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjorn Oct 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satu0king commented Dec 25, 2017 • edited

satu0king commented Dec 30, 2017

bjorn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satu0king Jan 7, 2018 • edited

Choose a reason for hiding this comment

satu0king commented Jan 7, 2018 • edited

bjorn commented Jan 12, 2018

bjorn Oct 17, 2017 •

edited

satu0king commented Dec 25, 2017 •

edited

satu0king Jan 7, 2018 •

edited

satu0king commented Jan 7, 2018 •

edited