Feature/jfxr sound effects #716
Conversation
this is a redo pull of #675
Jfxr yields better results and has a better interface
|
I noticed a bug in gdevelop with this, not yet sure whats causing it. If you go in the resource manager and remove the first resource 'jump.wav', when playtest - 'player\jump.wav' will start to work! alternatively renaming 'player\jump.wav' to 'player\jump2.wav' also fixes it why is 'player\jump.wav' being affected by 'jump.wav'? and the other way around? Something with the way resourcesmanager loads new ones or the way I am using it in this particular case? |
There was a problem hiding this comment.
I've added a few comments but this looks generally good :)
Thanks for your work on this, that was super fast ;)
I still have a concern about having the code of jfxr included in the repository of GDevelop. Note that it's a big deal, but I wonder if we should not go the Piskel way and have a script to import it from an archive (I can host it on GitHub).
It's almost the same but as the code is not part of the codebase we keep a better separation of what is GDevelop vs what are the 3rd party editors.
Also can be in a way more respectful of the author as we don't commit the files of his editor directly in GDevelop (so stats/lines of code are not affected).
Lastly having a separate archive means that we can add it to the .gitignore, which also mean that most text editors won't show jfxr sources when doing search on all the folders - which saves time and is less frustrating than when you're trying to move around files by typing a name and that you end up with the files of a 3rd party project :)
|
I'll be traveling at React Conf until Saturday to present a talk about GDevelop. I might be less available but will still try to continue reviewing. |
Thank you for reviewing :) I still have a few things to figure out before this is ready, so this gives me some time. Hope you have a nice trip and React Conf goes well. Silly question: with piskel's implementation - where do you keep the zip file that contains piskel? It's not here: |
It's on GitHub. See https://github.com/4ian/GDevelop/blob/master/newIDE/app/scripts/import-piskel-editor.js#L26 If you attach here the link to a zip file containing jfxr, we can do the same - I'll give you the link :) The advantage of having it on GDevelop GitHub is that we're sure it will always work as long as GDevelop github is up.
I think it's because when you call addResource, if the resource already exists with the same name it won't be updated (the old resource will stay untouched). |
|
Ah I see, it's getting it from the releases page. Clever 😄 On the resource - I will give this a try. Perhaps in the future we could make a generalized function to add/replace resources that does that? I wonder if piskel is affected by this too. I am getting a strange deja vu from when I worked a bit on the resource manager - with the scan for new resources function |
|
I think this is an issue with the game engine, it still happens with audio resources if I add the resources by opening them rather than using the editor. To replicate:
btw in the past I made a commit that makes it use relative paths as names, see here: I can try to tackle it here if its something with the editor - or try in another pull and file a separate issue I tried replacing the My guess is that it is something that has to do with |
- fixes on path editor - moved some of the code from jfxr bridge as a step to make some of it reusable with piskel later on - changed metadata storage approach to a url query string instead of object - simpler to save/load and also uses less data from jfxr author - fix on sound autoplay ttencate/jfxr#37 - fix on url loading of data ttencate/jfxr#36
.. so we can later also use it for jfxr, once jfxr is zipped and released at github. Update readmes
|
Can you attach here the zip file? Will be faster for me to upload as I have little time right now :) For the issue, is there any error in the console while previewing/exporting? |
No Problem :) I am attaching it to here On the resource issue: Edit: I opened an issue here #720 Getting back to this pull, there is still a problem with this with the .onload event not triggering by the iframe some times on the built electron version - I need to figure out why that happens and a way around it |
|
I dont know why exactly, but it works ok when in the development environment, but when I build an electron release and run that- the path editor or the jfxr data is some times taking absolute ages to load when you run it , other times - it loads immediately. in general jfxr seems really slow at loading on the build, and ok at loading in dev environment, im trying to find out whats causing it |
|
I've uploaded the zipped editor here: https://github.com/4ian/GDevelop/releases/download/v5.0.0-beta55/jfxr-editor.zip
I also observed this before with jsfx which is why I refactored to use "load" rather than DOMContentLoaded. I think there is a "race" condition, like code running before the page is fully loaded. I'm not sure why. Does jfxr have a function that we could use to be notified when the editor is ready? Would be more robust than relying on DOM events. Thanks for opening the other issue. Don't have time to look at it now but I'll try to see what's going on in the next days |
use link provided by @4ian to download jfxr
|
@4ian I updated the script in order to fetch jfxr as well, however I couldn't fix the loading problem on the build. I have no idea why it gets detected as loaded so late some times and immediately other times. It works consistently well when in development mode though - so perhaps it has something to do with the release version of electron? |
|
Release version should work pretty similar... Is it loading though or sometimes stuck without showing anything? |
|
yes indeed google will not get personal data, yet the author of jfxr can at least know how many gd users are using his tool :) |
|
@4ian actually I wonder if running it from electron would give him any stats whatsoever. If it fails to connect and doesnt work, is there any point of even having it? I guess the only way to find out if it works is from him :) |
|
@blurymind With the configuration that you've added, ad specific features will be disabled but I believe basic stats (notably the number of users) will still be there. Electron is based on Chromium so this will work (and in his stats, he will actually see the browser being reported as "Electron" if I'm not mistaken). I think it's fair to keep this as basic analytics will be useful for Jfxr author while avoiding any personal data to leak, both because we're in Electron so outside the user browser and because AdFeatures were disallowed. If there are no other concerns I'll merge this later tomorrow (when I finally get a bit of time!) |
|
yes I agree that he should have stats as they are a good motivation and I
have no concerns whatsoever. As noted before the analytics script doesbt
really affect the app, it just delays the onload iframe signal. since I
worked a way around that, it is not an issue any more
…On Wed, 31 Oct 2018 00:49 Florian Rival ***@***.*** wrote:
@blurymind <https://github.com/blurymind> With the configuration that
you've added, ad specific features will be disabled but I believe basic
stats (notably the number of users) will still be there. Electron is based
on Chromium so this will work (and in his stats, he will actually see the
browser being reported as "Electron" if I'm not mistaken).
I think it's fair to keep this as basic analytics will be useful for Jfxr
author while avoiding any personal data to leak, both because we're in
Electron so outside the user browser and because AdFeatures were disallowed.
If there are no other concerns I'll merge this later tomorrow (when I
finally get a bit of time!)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#716 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AGMbVZa6J5Ao6eOkxFwvhWZEt3s5axZcks5uqPOegaJpZM4X2lfa>
.
|
|
Neither of these bother me but I was wondering if either of these might require extra app permissions in android or ios which might cause some devs problems with moaning users?! |
|
As far as I understood the analytics scripts are only present in the IDE. The games you create shouldn't contain any analytics code. |
This is correct, games are not concerned by Analytics so no extra permission is required. ;)
Analytics are just an API that is called to report usage of features, so the only "permission" needed is an internet connection :) There is no reading of anything from the device. It's also optional so if internet connection is not working, it does not affect the rest of the app. |
|
I gave this a try via the signal jfxr author added, it doesnt work as well as the timer yet. Half the time it fails to receive the signal. I think it would be better to merge this with the timer approach- as that works 100% of the time both in dev and in release - to the end user that is all that matters- they will not care how we do it, only how well it works. Once we have a signal approach that is also as solid- replace it in another pull The timer may look a bit hacky, but it seems to do the job more reliably than any signals. I am actually starting to think of using it on piskel too. In some rare cases piskel has the same problem of a signal not being fired at the right time when initiated btw. |
The string methods are not the right approach
It seems to work solidly with no workarounds now! Much thanks to @ttencate for the awesome help in getting this to be easier to embed
|
@4ian I am attaching the latest version of jfxr here: @ttencate I disabled the jfxr button and the twitter buttons - as they were causing the iframe embedding to break |
| @@ -1,7 +1,7 @@ | |||
| This folder contains sources to embed Piskel editor (https://www.piskelapp.com/) so that it can | |||
| be used directly from GDevelop to edit images. | |||
|
|
|||
| Piskel sources are downloaded by `import-piskel-editor.js` script. They are raw, unchanged sources | |||
| of the Piskel editor. Sources will be stored in `piskel-editor` folder. | |||
| Piskel sources are downloaded by `import-zipped-editor.js` script. They are raw, unchanged sources | |||
| initialResourcePath.length | ||
| ), | ||
| projectBasePath: projectPath, | ||
| folderPath: path.dirname(initialResourcePath), |
There was a problem hiding this comment.
thank you for catching this btw
I've updated it :) Looks like we are very close! It's great that loading is now always successful, I think the approach is very clean (1) register the event listener, 2) Trigger the loading). My only concern is, can you check by building GDevelop if all of this is still working? I don't see a reason why it would not but let's double check. Look at my comments and if the editor loading is bullet proof, let's merge this :) |
|
I've tested it and it works well :) Also found the root cause of #720, see my comment here (it's not related so do not prevent this from being merged). Thanks a lot for working on this integration! Will definitely be a good improvement to GDevelop. Thanks @ttencate for making this amazing editor too! :) We should update the website to mention that Jfxr is embedded in next version 😄 |
|
Thanks all, loving this!
…On Sat, Nov 3, 2018, 22:40 Florian Rival ***@***.***> wrote:
Merged #716 <#716> into master.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#716 (comment)>, or mute the
thread
<https://github.com/notifications/unsubscribe-auth/AAFjMnil0rvpjbr_9J_e9BXhcaxPNYg5ks5urg1CgaJpZM4X2lfa>
.
|
This pull removes jsfx and replaces it with jfxr.
It also addresses some bugs with the path editor and the modalWindow