Piskel Resize prompt for any new animations + ability to set name and subfolder of new animations! #522

blurymind · 2018-06-15T16:57:08Z

This makes the resize pop up show up whenever the user opens a brand new piskel animation

It also enables the ability to select the folder where you want piskel to create new frames, as well as the name of the files:

Note:

if you select a folder outside of your project- piskel will refuse to use it
On animations imported from gdevelop- the folder and the name can not be edited by design- so the user doesnt make the mistake of saving half their sequence in another folder
If there is no animation name used in gdevelop, piskel will set it upon saving - so the animation name used in piskel is consistent with the one used in gdevelop!

This also makes sure that the user doesnt put any symbols in the animation name that would lead to an invalid filepath

This makes the resize pop up show up whenever the user opens a brand new piskel animation

- this adds ability to set name of new piskel frames - it also paves the way to transfer the name set in piskel back to gdevelop

blurymind · 2018-06-15T19:33:17Z

Added ability to set name of animation - It only affects the name of frames created in piskel- it will not rename any imported files from gdevelop, but should help with organizing!

I also made it so the user cant use any input characters that would lead to an invalid basepath

Now I am trying to figure out how to use the name fetched from piskel to set the changed animation's optional name - @4ian should i somehow pass it to the onchangessaved function?
Wondering how to properly get it to set the optional name

4ian · 2018-06-17T17:45:52Z

should i somehow pass it to the onchangessaved function?

Yes I think you can pass it and use it to pass it to a function that will change the animation name.
Let me know when I can check the code :)

4ian · 2018-06-17T17:48:13Z

newIDE/app/package.json

@@ -21,6 +21,7 @@
    "classnames": "2.2.5",
    "create-react-context": "^0.1.6",
    "date-fns": "^1.29.0",
+    "electron": "^2.0.2",


You should not commit any changes that you made to package.json or package-lock.json. Can you try to remove these?
You may want to read about git add (if you're using command line) (and git add -p, which is super useful to choose the part to add in your commit). If you're using a graphical client you should also be able to choose the files to include in your commits.

In general, only commit the changes related to your feature - otherwise I'll have to edit your commits and remove these :)

ah sorry, i thought it was in the gitignore

The gitignore contains the file that are not in the git repository at all.
It's a bit a pain at the beginning but once you know how to choose the files to commit, you can make much cleaner commits :) (by not selecting files like package.json or package-lock.json, or files with irrelevant changes)

guess the subfolder via the path of the first loaded resource

blurymind · 2018-06-17T19:21:54Z

@4ian can you help me in changing it (the animation name)? I am not sure how to pass it through the onchangessaved function. I got piskel to send it back to gdevelop, but can't figure out how to tell gdevelop to change it.
So if the user hasn't entered a name in gdevelop yet- make gdevelop automatically take the one set in piskel. It would be a nice touch imo :)

I also want to send the name to piskel too - so piskel uses the animation name if the user has entered a name- instead of the object name. The object name is the backup option
Edit: Oh wait- it looks like you are already passing the animation name to piskel :D

4ian · 2018-06-17T19:35:38Z

First, let's pass the name from Piskel back to GD:

(Piskel) When calling ipcRenderer.send('piskel-changes-saved', outputResources); in Piskel, add the extra argument that you want to pass: ipcRenderer.send('piskel-changes-saved', outputResources, name); (or a more complex object instead of name if necessary).
(Electron "bridge") Then add the argument in ipcMain.on('piskel-changes-saved', (event, imageResources) => { and mainWindow.webContents.send('piskel-changes-saved', imageResources);. Relaunch electron as you've here modified some electron related files.
(App) Then, add it to ipcRenderer.on('piskel-changes-saved', (event, outputResources) => { (in LocalPiskelBridge.js). Add it also to the call to onChangesSaved to have the name passed back to the SpritesList.

EDIT: In fact you've already done what I've said above ;) Here is the more interesting part then:

Now, you have the name (or whatever more complex object) in GDevelop SpritesList.
The responsibility of SpritesList though is only to display the list. To get the direction, you have to add a prop to SpritesList which is called for example "onChangeAnimationName". Take example on onReplaceByDirection and replaceDirection. You must do the same with a prop called onChangeAnimationName and a method called changeAnimationName (taking animationId and the new name as arguments) in SpriteEditor/index.js.
In fact this function already exists! Here it is: https://github.com/4ian/GD/blob/af4cdcd48542fcda671704fe5800f517ab02ca83/newIDE/app/src/ObjectEditor/Editors/SpriteEditor/index.js#L268

The job is mainly to be able to use React "props" to pass the function down to the SpritesList, and call it when you want to update the name.

EDIT : (If you're unfamiliar with props, maybe you can take a look at this React doc: https://reactjs.org/docs/handling-events.html)

...but not both. This will make more sense when the animation name on new animations is set in piskel and piskel sends it back to gdevelop

blurymind · 2018-06-17T20:29:52Z

@4ian Thank you for the explanation :)
This is indeed very interesting. I will add this final part tomorrow hopefully and will be ready for review.
I think trying to tackle saving layers needs to have its own pull request - once this one is good to go.

Btw I love the way you have designed gdevelop's new editor, it's a beautiful thing 👍

4ian · 2018-06-17T20:36:52Z

Cool, let me know how it goes!

You can see that changeAnimationName is passed as a prop to SortableAnimationsList as onChangeAnimationName. Then onChangeAnimationName is passed from SortableAnimationsList to SortableAnimation as a prop named onChangeName. Finally, onChangeName is passed to the text field containing the animation name. SpritesList is also there so you can reuse this props and pass it to SpritesList, and finally call it at the right moment in SpritesList.
Should not be too difficult in fact!

Btw I love the way you have designed gdevelop's new editor, it's a beautiful thing 👍

Thanks! It's the way how everything is architectured in the IDE with React: components are handling some part of the interface and use props to pass or get information to their children or parent components - where the real work is done. Need a bit of reading to find where to add new code, but the result is clean and really modular.

blurymind · 2018-06-19T08:56:51Z

@4ian how would one export the function changeAnimationName from a class that is not exported in index.js?

I tried everything,including importing and using onChangeAnimationName, and it still says that it is not a function

Do I need to edit multiple files to get access to that function in spriteList.js?
I must be missing something. Do I need to also export it in the file where it is established

Can you show me just in this instance how one would import it in the best way?

blurymind · 2018-06-19T09:23:09Z

@4ian I figured it out!!
I wasn't exporting it indeed.
I exported onchangeName in index.js and using it in SpriteList.js
Thank you @4ian , git comit coming later today :)

4ian · 2018-06-19T09:24:34Z

So you need to call onChangeAnimationName in SpritesList. First thing to do is to check if it's present in the props of SpritesList, here: https://github.com/4ian/GD/blob/af4cdcd48542fcda671704fe5800f517ab02ca83/newIDE/app/src/ObjectEditor/Editors/SpriteEditor/index.js#L134

The answer is no, there is no such props that we can use that is passed by Animation component to SpritesList component. So let's see if we can find a prop or a function doing the job in Animation. The answer is yes:

https://github.com/4ian/GD/blob/af4cdcd48542fcda671704fe5800f517ab02ca83/newIDE/app/src/ObjectEditor/Editors/SpriteEditor/index.js#L107

You can see being passed to another component here: https://github.com/4ian/GD/blob/af4cdcd48542fcda671704fe5800f517ab02ca83/newIDE/app/src/ObjectEditor/Editors/SpriteEditor/index.js#L122

Seems that its usage is quite simple: call it why the new name of the function as argument. Typing is helping us to ensure that this is the case:

https://github.com/4ian/GD/blob/af4cdcd48542fcda671704fe5800f517ab02ca83/newIDE/app/src/ObjectEditor/Editors/SpriteEditor/index.js#L88

So let's pass it to SpritesList! At the end of SpritesList props, pass it by adding onChangeName={onChangeName} (which means that we just pass to SpritesList the function):
https://github.com/4ian/GD/blob/af4cdcd48542fcda671704fe5800f517ab02ca83/newIDE/app/src/ObjectEditor/Editors/SpriteEditor/index.js#L149

Once you've added this props, it becames available in SpritesList. You can use it anywhere in the SpritesList component by using this.props.onChangeName. For us, we'll want to use it in onChangesSaved, at the end of the function: https://github.com/4ian/GD/blob/af4cdcd48542fcda671704fe5800f517ab02ca83/newIDE/app/src/ObjectEditor/Editors/SpriteEditor/SpritesList.js#L265

Something like this: this.props.onChangeName(newAnimationName).
(You can start by trying to call yourself this.props.onChangeName("Test123") somewhere else in the function).

When you call this function in SpritesList, the original function changeAnimationName from SpriteEditor/index.js is used:

https://github.com/4ian/GD/blob/af4cdcd48542fcda671704fe5800f517ab02ca83/newIDE/app/src/ObjectEditor/Editors/SpriteEditor/index.js#L268

React will update any children component using the animation name so that the change is reflected everywhere.

4ian · 2018-06-19T09:27:25Z

Following my previous explanation, the last thing you need to do is to "register" the new prop onChangeName by adding it to the Props "typing" after this line: https://github.com/4ian/GD/blob/af4cdcd48542fcda671704fe5800f517ab02ca83/newIDE/app/src/ObjectEditor/Editors/SpriteEditor/SpritesList.js#L153

Add your new prop: onChangeName: string => void (meaning that it's a function that takes a string and returns nothing).
This help the type checker "flowtype" to verify that you're properly using the function when calling it or passing it to the component. This avoid error when you forget to pass a function. Especially useful when you reuse your components everywhere in the codebase (which is not the case here, but still helpful to catch errors).

blurymind · 2018-06-19T09:27:31Z

@4ian I did it by adding this line to spriteList's return function (index.js):
onChangeName={onChangeName}

That allowed me to use it like this in SpriteList.js

        if (animationName.length === 0) {
          onChangeName(newAnimationName)
        };

but I also did as you said- registering it like this

onChangeName: string => void

So if the user hasnt set any optional animation name- piskel will automatically set it in gdevelop, depending on what was set in piskel when the animation was created

blurymind · 2018-06-19T09:29:00Z

Btw thank you for explaining this. Passing functions between modules confused me at first

4ian · 2018-06-19T09:30:21Z

That allowed me to use it like this in SpriteList.js

Yes that looks correct! :)
Don't forget to update the flow typing. You can check for errors by running npm run flow (or yarn flow) and/or installing the Flowtype extension for VSCode if you use it.

Passing functions in "props" is the React way to "export" functions to other components. It is what makes all component reusable because you can easily plug any function to any props and all will behave properly. Much like assembling bricks and plugging them together :)

...if there is no animation name used

blurymind · 2018-06-19T17:18:17Z

@4ian ready for review now :)
I updated the description of the pull too- for better documentation

We can now:

Use subfolders for animations made in piskel
Specify name for the image sequence and animation name when creating them
Autoresize automatically shows up on new animations

For support of saving layers -i need to do that in another pull- when we figure how to approach that

4ian · 2018-06-20T08:40:51Z

Great! Will try to check this today, posting comments as usual and we'll merge this when it is ready then. Thanks for your work so far 👍

4ian

I've left various comments about different things, that will help for clarity and maintainability. Thanks again for your work so far :)
Let me know if you have any question!

4ian · 2018-06-20T08:57:46Z

newIDE/app/public/External/Piskel/piskel-main.js

@@ -4,12 +4,52 @@ const path = require('path');
 const fs = require('fs');
 const async = require('async');
 const remote = electron.remote;
+const { dialog } = require('electron').remote;


nitpicking: You can do const { dialog } = remote, as remote is defined on the previous line.

4ian · 2018-06-20T08:59:22Z

newIDE/app/public/External/Piskel/piskel-main.js

+  if (!projectBasePath) {
+    projectBasePath = piskelOptions.projectPath;
+  }
+  var selectedDir = dialog.showOpenDialog(remote.getCurrentWindow(), {


nitpicking: can you use const instead of var? :)

4ian · 2018-06-20T09:04:09Z

newIDE/app/public/External/Piskel/piskel-main.js

+    return;
+  }
+  piskelOptions.projectPath = selectedDir;
+  alert('New frames  will be saved in:\n' + piskelOptions.projectPath);


Do you think this alert is necessary? I'm thinking it can be a bit redundant. If the user clicked on the folder and chose a new folder, the UI will be updated with the new path, I think it's clear enough.

I will remove it. You are right - it might get annoying after the first time :)

4ian · 2018-06-20T09:07:43Z

newIDE/app/public/External/Piskel/piskel-main.js


 const editorContentWindow = document.getElementById('piskel-frame')
  .contentWindow;
 let baseExportPath;
 let piskelOptions; // The options received from GDevelop

+let saveFolderLabel,


nitpicking: you're set piskelAnimationNameInput to an empty object, while later your making it a reference to a DOM input element (as the name properly implies).
It's best to then avoid using an object (which confuses the source code reader) and set it explicitly to null, null meaning that "this input is not yet created". You may have to update one or two other places to ensure that piskelAnimationNameInput is not null then before doing a change.

Objects are more to store data.

4ian · 2018-06-20T09:11:27Z

newIDE/app/public/External/Piskel/piskel-main.js

+  ); // Don't allow the user to enter any characters that would lead to an invalid path
+  piskelOptions.name = piskelAnimationNameInput.value;
+  baseExportPath = piskelOptions.projectPath + '/' + piskelOptions.name;
+  saveFolderLabel.innerHTML = piskelOptions.projectPath + '\\';


Avoid innerHTML as much as possible, consider using .textContent. It will first avoid you to have to escape characters, and above all it avoid XSS injections attacks that are possible when using innerHTML.
(All of this is not needed with a UI framework like React - which one of the many reason why it's great :))

More interesting information available here: https://stackoverflow.com/questions/24427621/innertext-vs-innerhtml-vs-label-vs-text-vs-textcontent-vs-outertext

4ian · 2018-06-20T09:18:11Z

newIDE/app/src/ObjectEditor/Editors/SpriteEditor/SpritesList.js

      objectName,
-      animationName,
+      animationName, 


nitpicking: there is a useless extra space at the end of the line ;)

4ian · 2018-06-20T09:18:38Z

newIDE/app/src/ObjectEditor/Editors/SpriteEditor/SpritesList.js

        isLooping: direction.isLooping(),
      },
-      onChangesSaved: resources => {
+      onChangesSaved: (resources,newAnimationName) => {


nitpicking: missing a space before the comma, be sure to run prettier to fix this

4ian · 2018-06-20T09:19:47Z

newIDE/app/src/ResourcesList/LocalPiskelBridge.js

@@ -54,8 +54,7 @@ export const openPiskel = ({
      resourcesManager.addResource(imageResource);
      imageResource.delete();
    });
-
-    onChangesSaved(outputResources);
+    onChangesSaved(outputResources,newAnimationName);


nitpicking: missing space after the comma

ah sorry, I will run format document on all the files again

4ian · 2018-06-20T09:20:06Z

newIDE/app/src/ResourcesList/ResourceExternalEditor.flow.js

@@ -11,7 +11,7 @@ export type ExternalEditorOpenOptions = {|
  singleFrame: boolean, // If set to true, edition should be limited to a single frame
  resourceNames: Array<string>,
  onChangesSaved: (
-    Array<{ path: string, name: string, originalIndex: ?number }>
+    Array<{ path: string, name: string, originalIndex: ?number }>, newName: string


👍 great, thanks for updating flow!

4ian · 2018-06-20T09:21:26Z

newIDE/app/public/External/Piskel/piskel-main.js

+      );
+      updatePiskelBasePath(); //update the path label
+      // Disable changing path and naming convention by user - on animations imported from gdevelop
+      saveFolderLabel.removeEventListener('click', selectBaseFolderPath);


Maybe move this part (all the logic to disable folder choosing) in another function? Will help to achieve clarity by avoiding mixing the loading logic with the UI updating logic. :)

blurymind · 2018-06-20T10:23:07Z

Applied all requested changes :) Comit coming later today 👍

blurymind · 2018-06-20T16:33:26Z

@4ian pull submited =)

4ian · 2018-06-21T17:33:38Z

Will double check this this evening :) Thanks!

4ian · 2018-06-22T13:34:27Z

Looks ok like this. Will merge this week-end!
Can you still try to remove the package-lock.json from this PR? (easier to track changes in the history for later)

blurymind · 2018-06-22T19:04:57Z

@4ian 'm sorry, I tried to remove it, but gitkraken wont let me unstage it, because it is in previous commits :(
I have no idea why it wasnt in the gitignore to begin with.. should have been more careful

Lizard-13 · 2018-06-22T22:25:24Z

This answer says that you have to reset the file in your pull request branch and make a new commit: https://stackoverflow.com/questions/39459467/remove-a-modified-file-from-pull-request

This reverts commit f316b37.

blurymind · 2018-06-22T23:45:29Z

@Lizard-13 @4ian I solved it by reverting the comit in which it was introduced

Thank you for the help :)

4ian · 2018-06-23T10:21:41Z

Good job with reverting the commit :)
Merging now. Thanks for working on it! 👍

blurymind · 2018-06-23T10:40:21Z

@4ian thank you for reviewing this and the patience! Now the piskel implementation in gdevelop is almost feature complete. Only thing missing is the ability to preserve layer data

Call Resize prompt on Piskel for any new animations

f316b37

This makes the resize pop up show up whenever the user opens a brand new piskel animation

blurymind mentioned this pull request Jun 15, 2018

Some suggestions for Piskel #518

Closed

Add ability to set piskel animation name

290e4e0

- this adds ability to set name of new piskel frames - it also paves the way to transfer the name set in piskel back to gdevelop

blurymind changed the title ~~Call Resize prompt on Piskel for any new animations~~ Call Resize prompt on Piskel for any new animations + ability to set name of animation - affecting name of new files Jun 15, 2018

blurymind added 7 commits June 15, 2018 22:21

add nicer styling to name input

038c690

run prettify

5330103

match colors with piskel's

6d8fc7c

experimenting with header styling

6d347e6

header styling-more tweaks

5fc42ca

give the new piskel basepath function a more meaningful name

227dff9

enable ability to select subfolder to save new piskel frames

195beb8

4ian reviewed Jun 17, 2018

View reviewed changes

reuse subfolder when resources have been saved to it

76ba9d0

guess the subfolder via the path of the first loaded resource

blurymind added 4 commits June 17, 2018 20:45

use animation name from gdevelop when available, if not use object name

915bc7c

...but not both. This will make more sense when the animation name on new animations is set in piskel and piskel sends it back to gdevelop

Update package.json

f4e601a

add a tooltip hint on piskel path label

8d6e4b7

reduce font size on path label

2ab1729

improve piskel header css+allow piskel to set animation name...

c3e39e2

...if there is no animation name used

blurymind changed the title ~~Call Resize prompt on Piskel for any new animations + ability to set name of animation - affecting name of new files~~ Piskel Resize prompt for any new animations + ability to set name and subfolder of new animations! Jun 19, 2018

blurymind added 3 commits June 19, 2018 19:00

fix flow checks

d5e1ebd

fix flow checks

90f9af6

add right margin to save button

9baa9f7

4ian reviewed Jun 20, 2018

View reviewed changes

add changes requested by @4ian

c2de2f7

Revert "Call Resize prompt on Piskel for any new animations"

66a3d61

This reverts commit f316b37.

4ian merged commit 6cdee3b into 4ian:master Jun 23, 2018

Piskel Resize prompt for any new animations + ability to set name and subfolder of new animations! #522

Piskel Resize prompt for any new animations + ability to set name and subfolder of new animations! #522

Conversation

blurymind commented Jun 15, 2018 • edited Loading

blurymind commented Jun 15, 2018 • edited Loading

4ian commented Jun 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blurymind commented Jun 17, 2018 • edited Loading

4ian commented Jun 17, 2018 • edited Loading

blurymind commented Jun 17, 2018

4ian commented Jun 17, 2018

blurymind commented Jun 19, 2018 • edited Loading

blurymind commented Jun 19, 2018

4ian commented Jun 19, 2018

4ian commented Jun 19, 2018

blurymind commented Jun 19, 2018 • edited Loading

blurymind commented Jun 19, 2018

4ian commented Jun 19, 2018

blurymind commented Jun 19, 2018 • edited Loading

4ian commented Jun 20, 2018

4ian 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

blurymind Jun 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blurymind commented Jun 20, 2018 • edited Loading

blurymind commented Jun 20, 2018

4ian commented Jun 21, 2018

4ian commented Jun 22, 2018

blurymind commented Jun 22, 2018 • edited Loading

Lizard-13 commented Jun 22, 2018

blurymind commented Jun 22, 2018 • edited Loading

4ian commented Jun 23, 2018

blurymind commented Jun 23, 2018 • edited Loading

blurymind commented Jun 15, 2018 •

edited

Loading

blurymind commented Jun 15, 2018 •

edited

Loading

blurymind commented Jun 17, 2018 •

edited

Loading

4ian commented Jun 17, 2018 •

edited

Loading

blurymind commented Jun 19, 2018 •

edited

Loading

blurymind commented Jun 19, 2018 •

edited

Loading

blurymind commented Jun 19, 2018 •

edited

Loading

blurymind Jun 20, 2018 •

edited

Loading

blurymind commented Jun 20, 2018 •

edited

Loading

blurymind commented Jun 22, 2018 •

edited

Loading

blurymind commented Jun 22, 2018 •

edited

Loading

blurymind commented Jun 23, 2018 •

edited

Loading