Add jsfx sound effects editor, refactor some of piskel's code...

[blurymind]

This pull adds a jsfx sound effects editor, refactors some of piskel's code make it usable by other editors.

It also splits the resource path editor into a separate js file that can later be used by piskel.
The path label shows a warning when a file exists at that path. See demo:

The jsfx editor can store metadata of the sound effect back to gdevelop. Then when edited again- it loads it. This makes sound effects in the game non destructive and easy to edit/tweak.

This metadata storing feature is thanks to @4ian:
af7f13c
#569

I will add layer storing to Piskel thanks to it, after this pull goes throug

Feature discussion here:
#693

[blurymind]

might have to do more cleanup and do flow checks tomorrow :)

[blurymind]

@4ian I might need some help with the flowtype. This will require a big change with the external editor flow - because you have made the external editor flow specific to only piskel. Other external editors will have different parameters, so I am not sure how to change it in a way that works best.

Anything I try seems to break the flow check on this further

[4ian]

I'm away today, will take a look tomorrow :)

…

On Sun, 14 Oct 2018, 11:23 Todor Imreorov, ***@***.***> wrote: @4ian <https://github.com/4ian> I might need some help with the flowtype. This will require a big change with the external editor flow - because you have made the external editor flow specific to only piskel. Other external editors will have different parameters, so I am not sure how to change it in a way that works for you as well — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#695 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABOIgg_NFOdCGcc4bjCfL6huBqvQTsQFks5ukxCRgaJpZM4Xa0yZ> .

[blurymind]

thank you :)

[4ian]

I've not had the time to review everything, I just let a few comments and will continue tomorrow.

It's unfortunate that we don't have JavaScript modules in code written in public/External. Maybe we should investigate the use of ES modules to have "import", otherwise everything is living in the global namespace and writing JavaScript like this is really the worst thing for future maintainability and robustness :)
At least we could "namespace" functions by putting them into an object (or an anonymous function) to avoid name clashing.

[blurymind]

@4ian applied all notes and actually ended up changing the way jsfx loads- so its more robust and simpler- much less global variables too.

Btw thank you for showing me this:
https://jakearchibald.com/2017/es-modules-in-browsers/

I think that it is fantastic that browsers now have this option. The code is much much easier to understand now

I am a bit afraid of touching the flow checking, as I feel like I might break it even more, but would be interested to know how you would approach it- so as to make it reusable for external editors

[4ian]

Added more comments. For Flow typing, let's forget about it for now. I'll help fix that later :)

[4ian]

Yes es module is really nice! :)

I am a bit afraid of touching the flow checking, as I feel like I might break it even more, but would be interested to know how you would approach it- so as to make it reusable for external editors

So basically I think we need to update ResourceExternalEditor.flow.js. In particular in this file, you can see the type ExternalEditorOpenOptions. It has a property called extraOptions, which is an object with name, isLooping, fps. We should:

1. Make them optional, by using a "?":

  extraOptions: {
    name?: string,
    isLooping?: boolean,
    fps?: number,
  },

this might create more errors as the Piskel bridge is using them directly, so we should always check if they are defined or cast them to a boolean/string (isLooping => !!isLooping, name => name || ""...)

2. then, we should add the option value taken by jsfx bridge:

  extraOptions: {
    name?: string,
    isLooping?: boolean,
    fps?: number,
    initialResourcePath?: string,
    initialResourceMetadata?: Object,
  },

I'll help if needed once we have the code that is cleaned up and re-architectured for jsfx :)

[blurymind]

extraOptions: { name?: string, isLooping?: boolean, fps?: number, initialResourcePath?: string, initialResourceMetadata?: Object, },

what about

Array<{ path: string, name: string, originalIndex: ?number }>, newName: string
  )

I can make that optional too? :)

[4ian]

I can make that optional too? :)

What do you want to make optional precisely?

The typing is here describing that you have a function called onChangesSaved, that takes two arguments: an array of objects and as second argument, a string.
The array is composed of objects containing path, name and originalIndex, optionally.

If you don't need everything for the Jsfx onChangesSaved, you can:

1. Still provide them! Even if they are not used, at least the code is consistent with Piskel
2. Make them optional. For example, I made path optional:

onChangesSaved: (
    Array<{ path?: string, name: string, originalIndex: ?number }>, newName: string
  ) => void,

Option 1) is in fact easier because you don't change the types. For, option 2) you might have to go in the onChangesSaved used by Piskel, and add condition to check if path is defined (because you said to flow that it is optional, so Flow will warn you against using it).

In both case, you need to return an array of objects (and not just the resourceName which is a string), because otherwise we're gonna have to totally incompatible onChangesSaved, which is error prone, hard to type, hard to reason about ("is this argument an array or a string???").
You also need to update your onChangesSaved function to expect an array of an object and not a string. It's fine to assume that only one element is in the array.

[blurymind]

@4ian so when I have 1 resource and metadata, I call it like this to be consistent with piskel??
onChangesSaved([{}],resourceName, metadata);
xD

this seems like creating confusing code in order to please flow

[4ian]

onChangesSaved([{path:resourceName}], newMetadata) means "here are the changes: a single resource with path being resourceName and this new metadata for the object name"
=> is that normal that you affect "resourceName" to a "path"?
=> you are not specifiying the name, is that normal?
=> Also, why are you affecting "newMetadata" to the argument called "newName"? You should add metadata as part of the objects that are in the array if you want to pass something else.

If newName is not needed, we'll make it optional too.

onChangesSaved: (
    Array<{ path?: string, name: string, originalIndex: ?number, newMetadata?: string }>, newName?: string
  ) => void,

[blurymind]

In this case all we need is a newname and the metadata. Just wondering how to make this easy and clean

For piskel I do not like the idea of storing the metadata in every single frame, so I am not sure if I want it inside the array. It seems like adding lots of copies of the same data when it is needed in only one place.

I was thinking more like this
Array<{ path: string, name: string, originalIndex: ?number }>, newName: string, initialResourceMetadata?: Object

I think that Piskel files store frames on a layer as a single tileset of the frames turned into a base64 string
In theory a resource with 50 frames will save the same tileset string 50 times in the project.Or maybe we can place it only in the first frame?

but this is now making me wonder how to approach this

Do you want me to change the parameter's structure of onChangesSaved or the flow file?

so fsfx will be called like this, I guess:
onChangesSaved ([{metadata:theMetadata}],newName)

and then function will be onchangsSaved(resourceData,newName) {
resourceData[0].theMetadata -> metadata
newName ->newName
}

[blurymind]

I gave this a crack and applied the review notes. The flow is still failing, but I think it's getting closer?

[blurymind]

the path editor is now using private variables, I refactored it so we only need one method to initialize it and use it - it is much easier to hook up now

[4ian]

renderXXX is a good function name.
Just for your interest:

Image that your function is called a "component"
Imagine that you call headerObject.saveOptions the "state"
Imagine that you can call other similar renderXXX functions.
Imagine that instead of manually mutating the DOM, you have a library that do it for you according to your render method.
Finally, imagine that you have a nice syntax to write your "render" method using a HTML like syntax <like>this</like>, but that is translated to JavaScript.

Picture it? That's React.js :)

[blurymind]

hahah yes, I was having a strange deja vu when refactoring it like that xD
Reactjs is fantastic and GD5 is an excellent example of why 😸

I can rename it to just render if you like, it is now a purely internal function with no need to export anyway

[4ian]

Looks way better! 👍The "renderXXX" functions and using objects make things cleaner and easier to understand.

I've still added a few comments but we're almost there. Let me know when you're done with it and I'll update myself your branch to fix any minor things/do cleanup or refactoring before merging.

I think there is only one flow error left: I explained how to fix it. If it's not working, I'll do it myself.

I'm almost thinking that we would need React here (see a few of my comments), but I guess it's "overkill" (we would need to add a transformer like babel for JSX) and with good naming and good separation of functions, we can get something that is sufficient and clean :)

[4ian]

I wonder why toString is useful here?

[blurymind]

the reason was that the method returns an object, not a string - I need a string there :)

[blurymind]

@4ian once again thank you for the excellent feedback and help. I managed to apply the review changes and slightly refactored jsfx-bridge's code to be more consistent with piskel's

It is now also passing the flow test at last. Thanks to your help I am starting to understand how the flow check works and why it is so important to have it.

Commit coming later today when I get home :)

[4ian]

This is looking good :) I'll fetch your branch locally to do tests this week-end and do any refactoring that I can think of to be sure that it's ready. Can you double check that it's working in all use cases?

Super glad to have this new lightweight 3rd party editor integrated. Will speed up creation (notably in game jams!). Also have to mention it on the website.

If there are no issues that we discover I think it could be part of the next version (otherwise it's not a big deal, I'll release another one with it a few days later when it's ready).

[blurymind]

@4ian thank you. Hope you find it as fun to use as I did. 😄
I tested and tested and it seems to work file. Hopefully my last commit will make loading seem smoother.

I hope that this will encourage the whole team in a game jam to use gdevelop to work on the game and cut down time spent on something as trivial as adding a sound effect. No need to search for it, download,import and so on. You can even tweak it. The idea for this came from remembering a gamejam- where we had 1 artist (me), one musician, one generalist and 2 programmers. My friend- the musician spent time on the music, but had to stop working on it on the second day- in order to look for basic sound effects. He lost some time also setting the software for making music (renoise). I thought- if we knew about this generator back then- it would have given him more time for the game music.

I noticed that a lot of games didn't have good basic sound effects because they ran out of time, but those that did- the sound effects made a big difference on their play 'feeling'. I think that gdevelop with these editors, the event sheet and the ability to reuse code is going to gradually become even more of a killer app for game prototyping and game jam events. The event sheet encourages artists without programming knowledge to try to make games, the pixelart and music editors should invite them to gd even more. The IDE will give them a complete and very intuitive dev environment that is both effective and fun to use - like a fantasy console without limitations.

[4ian]

This looks good to merge. I still think we can rework a bit the way pathEditor.js and jsfx-main.js are working. Notably to separate what is doing dom rendering (updateXXX/renderXXX methods) and the rest. => I'll take a look at doing this tomorrow.

Thanks a lot for working on this. It's a great addition to GDevelop and I'm happy that we are able to connect external editors to make the whole game making experience better and faster :)

Note that it's important that we keep having external editors like this well separated (in terms of code architecture) from the rest of GDevelop. Mainly because in the future it must be easy to update (if a new version is available), remove (if the tool is broken/too old) or replace the externals editors.
jsfx is starting to get "old" so if later there is a better option for sound effect generation, we should swap it :)

Merging this now, and will double check tomorrow if there is more refactoring to do!

[blurymind]

@4ian thank you 😄
With these additions to the editor and especially with the new reusable functions that you have added, the next release is going to be absolutely fantastic!

[blurymind]

@4ian I will talk with jfxr's developer on the possibility of using jfxr instead in the future.
The only problem atm is its dependency on angluar and not having a node module,but it is updated more often and gives slightly better results imo.

[4ian]

Looks like the developer pushed it as a npm module :)
Note that the dependency on angular could be ok if the editor is stand-alone by itself. Rather than adding the npm module to the package.json of GDevelop, we could npm install it in a folder (in external/jfxr) by itself. Then run it as usual (i.e, in a modal BrowserWindow, then add a bit of glue code to load a sound/get the .wav).

[blurymind]

I think of giving this a try and once its functional remove jsfx :)
A lot of the code we have now for jsfx can be reused- notably the save path editor

[4ian]

Yes the idea would be to keep path-editor.js (adapt it if needed), it should be reusable. Potentially it could be used in Piskel too (this would allow to have a single code to handle this, could make improvements faster if the header is updated/redesigned at some point)

[blurymind]

Yes indeed, I was planning to refactor piskel a bit to use path-editor.js, when adding the ability to store the layers data