ttencate / jfxr

A browser-based tool to create sound effects for games.
http://jfxr.frozenfractal.com/
429 stars 45 forks source link

Integrating jfxr in Gdevelop - a bit of help #35

Closed blurymind closed 5 years ago

blurymind commented 5 years ago

Hi, I built the web app and am now getting it embedded in gdevelop. Basically I need to get the json data of the sound effect and the base64 string of the wav file. Then I need to be able to load the json data when angular opens and it was saved back to gdevelop in the past. That is how it works for jsfx.

Because jfxr is an angular app, I am now wondering how do I access its controller and all of its methods in electron/nodejs? Is there a simple way to access the dom?

Sorry I dont have any previous Angular experience

EDIT: Pull ready to merge in gdevelop now https://github.com/4ian/GDevelop/pull/716 Much thanks to both @4ian and @ttencate for the help/advice along the way

blurymind commented 5 years ago

I can find the methods I need inside bundje.js , but accessing bundle.js from another js seems a bit tricky I am loading the app as an iframe btw

blurymind commented 5 years ago

Ok I managed to get to it with this ridiculous line: jfxr.angular.element(editorContentDocument.getElementsByClassName('ng-scope')[0]).scope().ctrl xD some progress

blurymind commented 5 years ago

now I can store the json data, however a bit stuck at writing the wav file. toWavBytes doesnt seem to work, or I am not triggering it correct

You store the data in float32array, so far I have been using base64 strings. Need to figure out how to write the file

blurymind commented 5 years ago

ok nevermind, I figured that out as well :) Might be able to do a pull some time this week :)

blurymind commented 5 years ago

I got this sorted now - will let you know when it gets merged :) closing this now, as I don't need help any more

ttencate commented 5 years ago

Hm, we're clearly not in the same time zone (I'm in Central European Time) :)

I didn't realise you wanted to integrate the entire app. I thought you just wanted the synthesis library. Glad you got it figured out!

FWIW, if I ever seriously continue development, I'll probably be porting the entire thing to Angular "2" or React or maybe Vue, which would break your embedding approach. At that point, we'll discuss adding some kind of proper API to the UI.

blurymind commented 5 years ago

Hi, i decided to embed the whole app in the end, as i figured it will be faster than recreating the already awesome ui. :) We might still use the library for the game engine side.

On upgrading- dont worry we will adopt to any changes you make to it. I will share with you my pull request once its ready, so you can see how I am interacting with the current interface. Gd will use the current version until adjustments are made for any major upgrades

On Tue, 23 Oct 2018 11:06 Thomas ten Cate, notifications@github.com wrote:

Hm, we're clearly not in the same time zone (I'm in Central European Time) :)

I didn't realise you wanted to integrate the entire app. I thought you just wanted the synthesis library. Glad you got it figured out!

FWIW, if I ever seriously continue development, I'll probably be porting the entire thing to Angular "2" or React or maybe Vue, which would break your embedding approach. At that point, we'll discuss adding some kind of proper API to the UI.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/ttencate/jfxr/issues/35#issuecomment-432184159, or mute the thread https://github.com/notifications/unsubscribe-auth/AGMbVZPMcMlcCD0AoJEcF-NuyFeYymqgks5unuotgaJpZM4X0DQX .

blurymind commented 5 years ago

Quick question - I am trying to decide on how to save jfxr sound resource data in gdevelop projects.

I have two already implemented approaches:

  1. an url append #string to append to the sound loaded jfxr.createLink();jfxr.link (pro: its shorther, con: harder to read)
  2. the original object, fetched via jfxr.getSound().serialize() (pro: clearer to read, con: longer) This here is one of the older pulls where I made the jump https://github.com/4ian/GDevelop/pull/716/commits/7cce6d1d97dbc4f0810552d8f38145f160c92a07

which of the two is more likely to be backwards compatible spec in future versions or better in your opinion to use? They both work

ttencate commented 5 years ago

It doesn't really matter I think; both go through the MainCtrl which is not a public API. Just use whichever is easiest for you.

blurymind commented 5 years ago

I went with storing the serialized object as a json sting :) it looks cleaner - and it was actually shorter when stored. Btw the analytics script you have attached to the app was causing issues with embedding it: https://github.com/4ian/GDevelop/pull/716#issuecomment-433731265

Basically the iframe's onload signal gets massively delayed 50% of the time when the app is inside an iframe, inside an electron window instance - due to the analytics script. One way I found around it is in using a timer instead of the onload: https://github.com/4ian/GDevelop/pull/716/commits/03c6e7cb7b9aa8e1828212c84d783663105f3908 @4ian also has analytics to track how many times different parts of gdevelop are used, so I understand this

Would it be possible perhaps - if we have this script, to also have a signal emited when the ctrl element is loaded? That way we will be able to get it in a cleaner way- on the frame it happens :)

In any case,its almost ready now to be merged. I kept it vanilla, the paypal button and everything :) Only thing I had to remove on load was really the github link, as it couldnt get loaded in an external window from the electron one for some reason

ttencate commented 5 years ago

That's odd. The Google Analytics script is loaded async so it shouldn't be delaying anything:

  (function(i,s,o,g,r,a,m){i['GoogleAnalyticsObject']=r;i[r]=i[r]||function(){
  (i[r].q=i[r].q||[]).push(arguments)},i[r].l=1*new Date();a=s.createElement(o),
  m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
  })(window,document,'script','//www.google-analytics.com/analytics.js','ga');
  ga('create','UA-19568648-1','auto');ga('send','pageview');

I'll add an event that you can listen to, which will be fired after initialization of the main controller, like this:

document.addEventListener('jfxrReady', (e) => { console.log(e.mainCtrl); });

It will be fired from a DOMReady event handler, so it isn't any faster than using DOMReady yourself, but it avoids depending on the order in which event handlers are called. And it beats polling ;)

4ian commented 5 years ago

I'll add an event that you can listen to, which will be fired after initialization of the main controller, like this:

Looks great! Thanks for adding this, should make things more robust. @blurymind let me know if you have time to try it :)

blurymind commented 5 years ago

I can do it after work in a few hours, if its added to jfxr :)

On Wed, 31 Oct 2018 13:28 Florian Rival <notifications@github.com wrote:

I'll add an event that you can listen to, which will be fired after initialization of the main controller, like this:

Looks great! Thanks for adding this, should make things more robust. @blurymind https://github.com/blurymind let me know if you have time to try it :)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ttencate/jfxr/issues/35#issuecomment-434686471, or mute the thread https://github.com/notifications/unsubscribe-auth/AGMbVRRwAPDO36bPJkF57mnbP8RdRVJuks5uqaWDgaJpZM4X0DQX .

blurymind commented 5 years ago

Thank you for doing that btw. We are getting really close to a merge now

blurymind commented 5 years ago

Ok I gave this a try and using the signal like this: document.addEventListener('jfxrReady', (e) => { console.log(e.mainCtrl); }); nothing prints out. Using it like this

document.getElementById('jfxr-frame').contentWindow.addEventListener('jfxrReady', (e) => {
  console.log(e.mainCtrl);
  console.log('received'); 
});

60% of the time it doesnt run the console.logs in dev mode. Not sure why

I even tried

const editorFrameEl = document.getElementById('jfxr-frame')
editorFrameEl.onload= ()=>{
  document.getElementById('jfxr-frame').contentWindow.addEventListener('jfxrReady', (e) => {
    console.log(e.mainCtrl);
    console.log('received'); 
  });
}

Just to be clear I am not adding these to the index file of jfxr. I chose to always keep the software we embed in gdevelop vanilla, so thats why all access is through an iframe

Is it possible the event listener gets added after the event signal is fired some times?

@ttencate you can give this a try if you like to btw :) the code is in https://github.com/4ian/GDevelop/pull/716/files#diff-d557c40d0ec4a7e0c1bda09bd4f98766R47

ttencate commented 5 years ago

Indeed my example using document was never meant to work from outside the iframe itself, sorry. I didn't want to risk writing an example that I couldn't test. But your second snippet looks good to me.

The jfxrReady event fires around the time the DOM inside the iframe is ready (DOMContentLoaded, I think... it's the ubiquitous $(function() { ... }) function from jQuery). This means that some of its resources (CSS, images) might not have been loaded yet, so it comes before the onload event on the iframe itself.

Like you said, there's probably some race condition here. At what point do you attach your own event listeners? If that's done synchronously while the page is loading, and the <script> comes before the <iframe>, you should always be on time. However, at that point the iframe might not yet exist in the DOM, so you can't attach any listeners to it... and as soon as the iframe does exist, it might already have been loaded too, and you're too late.

So instead, you could try the following: don't set the src attribute of the iframe in the HTML directly, but first attach the event listener through JavaScript, and then set the src through JavaScript as well. That way you'll always be on time, at the expense of some loading time perhaps.

In your last snippet, you are listening for onload, which only fires after all dependent resources have finished loading. This is too late to catch jfxrReady. And it would probably include even async scripts like Google Analytics, which explains your earlier slowdowns.

blurymind commented 5 years ago

@ttencate thank you for taking the time to explain this,

This is too late to catch jfxrReady. And it would probably include even async scripts like Google Analytics, which explains your earlier slowdowns.

Yes indeed that was what I tried using before the timer solution in my last commit and 50% of the time it got delayed by 10-20 seconds on the electron build - due to the analytics process.

Using iframes in general is not ideal, but at least allows us to keep the external editors' code vanilla :)

I tried having the script before the iframe yesterday and still had the signal not fire half the time. But I haven't tried removing the src from the iframe tag in the html and instead setting it in the script after adding the listener. I will try it after work tonight and do another commit.

blurymind commented 5 years ago

@ttencate trying that now the signal never actually fires :( Inside jfxr-main.js, add this:

document.getElementById('jfxr-frame').contentWindow.addEventListener('jfxrReady', (e) => {
  console.log(e.mainCtrl);
  console.log('received'); 
});
document.getElementById('jfxr-frame').src = 'jfxr-editor/index.html'

nothing prints out. Inside the index file, you can try moving the script before the dom, after the dom - just doesn't fire.

If it matters the js script is embedded in the dom as a module, <script type="module"...

I suggested to @4ian to just use the timer approach for now, as it is more reliable than anything else - if it's ok by you as well. Even with your signal this is turning out to be tricky with the current implementation

4ian commented 5 years ago

I suggested to @4ian to just use the timer approach for now, as it is more reliable than anything else - if it's ok by you as well. Even with your signal this is turning out to be tricky with the current implementation

I'm ok for using the timer/retry approach as it's not noticeable. Will merge as is tomorrow/this week-end. Would still be interesting to push a bit more to search if we can reach a good solution without it. If we can't that's fine, the solution with setInterval will do the job and don't have a real impact other than being not as "clean" as it could (in theory).

blurymind commented 5 years ago

To the end user it loads really fast even with polling - most importantly with 100% success :) The timer approach seems more solid even when the signal does work most of the times- because if it fails the first time- it will retry instead of just spilling an error or stopping. for example,I used a signal on piskel and some times- even rarely - when you load it from gdevelop- it doesnt load the sprites - most probably because of the same reason. But if you close it and reopen it- it loads them. It's almost the same number of lines of code, so its really not making it harder to maintain or understand imo. To me end user experience is more important than details of implementation for this case. This kind of bug can be hard to catch because it may not occur often (like its rare in piskel) and not behave the same on all platforms (almost never occur in dev mode and occur on builds- making testing iteration time painfully slow - I have to rebuild the electron version to test- 5-10 minutes between tests).

Since this is something that seems to happen to a different degree in embedded software I've added so far, I wonder if the retry approach is something that would help solidify loading these apps in one way that is reliable in most of them instead of trying to hit a corner case on each one and waste their developer's time trying to figure out the order of events.

So dealing with this is kind of problem on piskel, jsfx and now here is making me change my opinion about using a timed retry . It is not an ugly workaround, but a solid way to gain access without affecting the user experience in a noticeable way.

ttencate commented 5 years ago

Hmm, probably the contentWindow is replaced by a new object when the iframe is loaded, so your event handler is lost. I didn't think of that.

The <script type="module"> construct is new to me, but I suppose it means the script won't be executed synchronously while the page is being parsed, so it wouldn't matter where you place it.

I have now changed the event to fire on the parent window instead (window.parent from within the iframe). Combine that with delayed setting of the src attribute, and it should Pretty Much Always Work™ like this:

window.addEventListener('jfxrReady', (e) => {
  console.log(e.mainCtrl);
  console.log('received'); 
});
document.getElementById('jfxr-frame').src = 'jfxr-editor/index.html'

But if you want to stick with polling, I totally understand :)

blurymind commented 5 years ago

@ttencate I will try this again tonight with the new version and test extensively. In any case I would like to thank you for taking the time to explain how this works - I learned a few things about signals and async loading just from this exercise. In the process you also fixed a number of bugs in jfxr very quickly and made it much easier to embed- all of these improvements will go into what is bundled with gdevelop. Hopefully having it included with gdevelop will also attract more users to both projects :) I think that it is hands down the best javascript sound effects generator atm and your dedication to it is what makes it so great

blurymind commented 5 years ago

I tried it with the signal after the latest commits to jfxr and it works flawlessly now. I made the gdevelop embedding use the signal now. https://github.com/4ian/GDevelop/pull/716/commits/24eb7428f0fc4afdec9342f9248be8fd53cda1f5 If people report any issues with loading we can always return to polling - or use it on other software that needs it

@ttencate fantastic work! @4ian we are ready for review :) 👍