4ian / GDevelop

:video_game: Open-source, cross-platform game engine designed to be used by everyone.
https://gdevelop.io
Other
6.56k stars 722 forks source link

Embed yarn editor in GD #1185

Closed blurymind closed 4 years ago

blurymind commented 4 years ago

This bundles Yarn - dialogue tree editor with gdevelop - making it possible to create and edit dialogue trees directly inside gdevelop. yarnGD

It's worth noting that Yarn's devs gave me full access to do commits to it. If there is anything required to fix it- I can fix it :)

This design bundles a new version of the electron branch of yarn, which I did some refactoring to in order to make it also work as a standalone web app. I am attaching it as a zip here, for those who want to test. Here is a build of https://github.com/InfiniteAmmoInc/Yarn/tree/electron

updated yarn package: https://github.com/YarnSpinnerTool/YarnEditor/commit/a5fb76b771481c92b2ec4af47d4da460c4b56aa8 yarn-editor.zip (a5fb76b )

This pr also gives the path editor a new feature- a [>] button to toggle it, as well as css cleanup - to make the style easier to read and carry over to other editors.

Not sure if I should do the bbs parser next or ability to read mod music files :P

updated the example: dialogueExample.zip Here is a screenshot example of how to set up an entire npc system with the extension: 2019-08-18_15-00 2019-08-18_15-17

One with animated avatars and conditional branches

trello https://trello.com/c/vcRqzoGk/74-bundle-yarn-a-dialogue-editor-for-interactive-stories

4ian commented 4 years ago

Looks pretty nice! :smile:

I will get these new changes merged to yarn's electron branch soon too.

Ok cool, would be great if the yarn-editor.zip could be generated from a well known branch/commit so that it's easily reproducible in case me or someone else has to make a fix in Yarn and re-bundle it.

Thanks for taking the time to refactor things related to the header, I'll give a more in depth look at the code.

Would it be possible to have a very very simple example file: a game, with a simple dialogue json file, a text object and just a few events to kickstart the dialog and maybe a key to press (or click) to continue. Context:

In a way, consider the example(s) as your guarantee that I won't break your extension behind your back when you're not looking at it ;)

blurymind commented 4 years ago

Thank you πŸ˜„ Yes I plan to do it this week some time. Just want to make sure everything is Rock solid with it in GD, as an electron app and as a webapp. The webapp support is going to be nice, since people will be able to use it straight in the browser from a smartphone. It will also be a step towards having yarn behave really well with mobile screens

On Wed, Aug 7, 2019, 10:17 AM Florian Rival notifications@github.com wrote:

Looks pretty nice! πŸ˜„

I will get these new changes merged to yarn's electron branch soon too.

Ok cool, would be great if the yarn-editor.zip could be generated from a well known branch/commit so that it's easily reproducible in case me or someone else has to make a fix in Yarn and re-bundle it.

Thanks for taking the time to refactor things related to the header, I'll give a more in depth look at the code.

Would it be possible to have a very very simple example file: a game, with a simple dialogue json file, a text object and just a few events to kickstart the dialog and maybe a key to press (or click) to continue. Context:

  • This will allow people to quickly see how to use this extension and try it.
  • For me that don't know well how Yarn or the extension work, it's a way to quickly test that the extension still work after I do a change/refactoring to the extension or Yarn.

In a way, consider the example(s) as your guarantee that I won't break your extension behind your back when you're not looking at it ;)

β€” You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/1185?email_source=notifications&email_token=ABRRWVJF773V4IGCGREQQN3QDKHI3A5CNFSM4IJZRQ32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3XYTUQ#issuecomment-519014866, or mute the thread https://github.com/notifications/unsubscribe-auth/ABRRWVIKOZSAYFSPRBAVONDQDKHI3ANCNFSM4IJZRQ3Q .

blurymind commented 4 years ago

I applied the review notes and did some cleanup. Also committed the latest changes to yarn's git, so you can see that the currently attached build is based on Latest commit Here is a build of https://github.com/InfiniteAmmoInc/Yarn/tree/electron (7aab340) yarn-editor.zip

4ian commented 4 years ago

Cool :) I have to take a look at testing this. How stable do you think this is?

blurymind commented 4 years ago

Cool :) I have to take a look at testing this. How stable do you think this is?

I am confident it is pretty stable in terms of editing/saving :) I am further testing the runtime (extension) in a more elaborate demo project atm.

I was wondering if it would be useful to add the name of the editor as metadata of the resource. It currently is not adding metadata

4ian commented 4 years ago

I was wondering if it would be useful to add the name of the editor as metadata of the resource. It currently is not adding metadata

Yes please :) Will be useful when we start having more than one editor using json resources.

blurymind commented 4 years ago

@4ian I found a bug in my dialogue extension. Will you be ok if I include it in this pr? I discovered it while making a more elaborate example of the feature :)

4ian commented 4 years ago

Yup it's fine as related :)

On Tue, 13 Aug 2019, 13:28 Todor Imreorov, notifications@github.com wrote:

@4ian https://github.com/4ian I found a bug in my dialogue extension. Will you be ok if I include it in this pr?

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/1185?email_source=notifications&email_token=AAJYRAQCC3PKTDXMOKJXS53QEMKONA5CNFSM4IJZRQ32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4G4K2Q#issuecomment-520996202, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJYRASSZGGNWWVGPDN4JOLQEMKONANCNFSM4IJZRQ3Q .

blurymind commented 4 years ago

@4ian so far so good, the demo is almost ready. Before more people start using it, I want to try to make a more reusable example sheet and see if this needs more work to be reusable.

Before it gets out, I wan't to see if we can get some of the names changed and logic simplified 2019-08-14_21-06

1- "is running" - what is running? The only hint is the icon - but what if some people arent that good at reading the icon and what if at some point we get another extension with a similar icon. suggested change: rename to "Dialogue is running"

2, 3- I am not a big fan of the "repeat" for loop in the example for a couple of reasons:

4- The use of "has" in some conditions at the beginning of the expression seems good for a javascript variable naming convention, but for plain english it seems a bit odd to put the words in that order. suggested change: rename to "Clipped text has completed scrolling" and "Selected option has changed"

Anyway, would appreciate getting your thoughts on this :) I want to get this right before way more people start using it

blurymind commented 4 years ago

I noticed a way to crash bondagejs :D, so now I think of tackling it in either yarn or bondage https://github.com/hylyh/bondage.js/issues/50

then update either/both so gdevelop users never encounter it

4ian commented 4 years ago

suggested change: rename to "Dialogue is running" rename to "Clipped text has completed scrolling" and "Selected option has changed"

Yes agree for all. GDevelop sentences in the events sheet should include the "subject" they are dealing with.

The use of "has" in some conditions at the beginning of the expression seems good for a javascript variable naming convention, but for plain english it seems a bit odd to put the words in that order.

Yes, same remark, GDevelop sentences must be complete.

  • I am not a big fan of the "repeat" for loop in the example for a couple of reasons:

Not sure about this one, I think I don't have a better solution. Unless avoiding the need for a loop entirely but not sure how. EDIT: a convenient action or expression that returns a pre-formatted text maybe?

blurymind commented 4 years ago

@4ian thank you. That is actually a great idea :) I will give it a try tonight. The simpler it is to use the better

blurymind commented 4 years ago

An error happened when trying to access the dialogue branch:

Also, in which case does this happen? Can we prevent this?

it's whenever bondagejs tries to access a dialogue branch that does not exist in the tree data or contains illegal characters. The library itself doesn't catch these errors yet

4ian commented 4 years ago

In this case, can we add a check on the extension side before going further? I would rather have explicit error checking rather than catching exception because this can hide other problems.

blurymind commented 4 years ago

In this case, can we add a check on the extension side before going further? I would rather have explicit error checking rather than catching exception because this can hide other problems.

unfortunately in this case it won't be trivial. I can add some custom logic to catch the most common case, which is not existing - for the illegal characters one it is something that might be fixed on bondagejs or yarn can be made not to allow them

blurymind commented 4 years ago

Should you add a return?

I will have to revert that code as it's not working properly. Due to the way yarn structures its data, this is not trivial to do there. Let me see if there is a clean way. The problem is that the node name is different from the text in the rendered option

The node name !== the option text. And bondage js has no easy way to get them together

4ian commented 4 years ago

Ok, in all cases would be great to validate/handle as much potential errors as possible and have the catch for the other cases, but that should be exceptional cases and if there is no other way to verify errors before.

blurymind commented 4 years ago

these are kind of rare and you have to do some elaborate steps to cause them. For example to trigger the non existent branch name access, I had to manually delete the branch node, leaving a reference to it and save the file.

Ideally I want to add a warning about that in yarn itself, so the user will never be allowed to cause the error without knowingly saving a file with an error in it.

The try/catch are only used where I want to be 100% sure bondagejs itself has no way of crashing the game - so accessing any methods it has that may not have good catching of errors. Either in it parsing data with issues with it, or calling a method with illegal parameters.

Since bondagejs is very lean as a library - it doesn't have much in the way of utility functions yet. A lot of the work has been put towards unit tests in testing it and is quite stable though

4ian commented 4 years ago

Ok fair enough then! :)

blurymind commented 4 years ago

having yarn embedded now with bondage is allowing me to catch errors much more rapidly, as the iteration time is much faster. You can really feel a huge difference when the editor is under one roof with bondagejs

blurymind commented 4 years ago

btw I added a new expression that renders the options with the cursor in a single event: 2019-08-15_21-06

You can pass as parameter what you want to use as selection cursor- although you can not pass utf characters/emojis ➑️ there- as it will crash gd. In case of more ellaborate implementation of that- there are the old methods still in place

no need for complicated repeat loop and manual index variable. There is something to wet the apetite of the first time user and something to keep the more advanced user happy

Should I keep both in the example file?

4ian commented 4 years ago

btw I added an action that renders the options with the cursor in a single event:

Looks great! :)

although you can not pass utf characters/emojis ➑️ there- as it will crash gd

Is this crashing GD or the game/bondage.js?

Should I keep both in the example file?

Let's keep only the simple one using the new expression. People that want something more complex will be able to dig and figure out by themselves how to use a repeat event I think :)

blurymind commented 4 years ago

The emoji characters - when loading them from the yarn json file - they seem to work flawlessly. 2019-08-15_21-17

However if you try to enter emojis in the event sheet and save your project,close it, reopen it and play the game, gdevelop.js starts to misbehave. I can attach an example project on a new issue if you are interested? :) I assumed that entering symbols and characters as string parameters was never ok with gdevelop.js

4ian commented 4 years ago

Would be interested yeah, I'll take a look :)

Bouh commented 4 years ago

Issue / improvement needed :

About Yarn system :

It's normal Yarn stopping at each lines ?

I can only see the first line in GD, i need press Z for next lines.

This mean i can't write in one time multiple line with \n. I need "call next line" with Yarn for each lines :( Maybe we need a new action for write all text from node. not just one line.

Maybe relative to example :

Im waiting documentation for create something from scratch and understand each events.

blurymind commented 4 years ago

Yes that is correct. New lines are basically a way for yarn or in this case bondagejs to interpret when to start printing the next message. So if you want to have one huge message printed at once, you can do that with the commands available too, it's just not in the example. :) The new line pauses are intended by design.

The editor locking gdevelop is just how gdevelop uses external editors at the moment- as modal windows- intended design again. I can change that for yarn or even the other editors if @4ian oks it :) i can understand how it can be a bit inconvenient when it locks gdevelop while being used

On Thu, Aug 15, 2019, 10:32 PM Bouh notifications@github.com wrote:

Issue / improvement needed :

  • When Yarn editor is open i can't use GDevelop. Yarn editor is over GD on block him. It's counter productive for multiples monitor.

About Yarn system :

It's normal Yarn stopping at each lines ?

  • When i talk to "enemy2" he say in json file : "Welcome to the dialogue tree demo project. πŸ€“\nPress Up/Z to interact with the tree critters, or alternatively jump on their heads"

I can only see the first line in GD, i need press Z for next lines.

This mean i can't write in one time multiple line with \n. I need "call next line" with Yarn for each lines :( Maybe we need a new action for write all text from node. not just one line.

Im waiting documentation for create something from scratch and understand each events.

β€” You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/1185?email_source=notifications&email_token=ABRRWVLPWETSG2QKZ3FYOCTQEXDO5A5CNFSM4IJZRQ32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4NB3VA#issuecomment-521805268, or mute the thread https://github.com/notifications/unsubscribe-auth/ABRRWVPZ3WTZQMQJOZIPJL3QEXDO5ANCNFSM4IJZRQ3Q .

blurymind commented 4 years ago

@Bouh thank you for the feedback.

I will look into addressing these points:

Please note that the example project i shared so far is just for testing - it's not presentable yet. If you want to you can play with it, change the level layout or stuff in the event sheet. My focus is still on how the extension works and yarn

4ian commented 4 years ago

make the yarn editor window non modal (needs @4ian to ok it :) ) (include those things in the example)

So I would be ok if we think carefully about what this implies:

This might be quite a bit of careful work, so maybe let's do it in another PR or if done here, let's plan carefully to avoid forgetting tons of edge cases :)

blurymind commented 4 years ago

@4ian not sure why its not letting me use a boolean there 2019-08-16_20-38 its not setting the default value to true either..

I used the same code that is used elsewhere, but not for expressions, so it appears I have discovered a bug?

this is the intended design 2019-08-16_21-09

4ian commented 4 years ago

Boolean are not supported in expressions for now - sorry about that! You can work around this by making another expression with a different name OptionsTextWithNewLine? Can actually make the whole thing clearer.

On Fri, Aug 16, 2019 at 12:39 PM Todor Imreorov notifications@github.com wrote:

@4ian https://github.com/4ian not sure why its not letting me use a boolean there [image: 2019-08-16_20-38] https://user-images.githubusercontent.com/6495061/63193640-e48a2780-c065-11e9-8c78-d989da504476.png

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/4ian/GDevelop/pull/1185?email_source=notifications&email_token=AAJYRAVOVFPVKHQLJURQ5ELQE366BA5CNFSM4IJZRQ32YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD4PQSVQ#issuecomment-522127702, or mute the thread https://github.com/notifications/unsubscribe-auth/AAJYRAXZ3TJJWWWC22DFDSDQE366BANCNFSM4IJZRQ3Q .

blurymind commented 4 years ago

horizontalOptionsList and verticalOptionsList?

or horizontalOptionsText and verticalOptionsText?

blurymind commented 4 years ago

just to be clear, the method .setDefaultValue(...) only works for numbers? Is there a way to do it for strings? Wish I could give it a default cursor when the user forgets to put one

blurymind commented 4 years ago

@Bouh for getting the raw text of a branch (newlines included), there is an expression for that already:

        'BranchText',
        _('Get the raw text of the current branch'),

the downside to it is that you cant hide <<commands>> or [[options]]in that branch, also new lines are not treated as logical separators - everything will print and is treated as raw text when you use that expression. I recommend using that only for big sign text messages - not for dynamic dialogue logic, where new lines are treated as part of the rendering logic or the player needs to make choices. This is how bondage.js and yarn are designed, so I respect that, since more often than not I use the newlines as intended - i like that part since it makes it super easy to control the text message pauses.

Later on I can add a bbcode tag to insert new lines anywhere you like [br]1[/br] but without the limitation above - but that is for the bbcode parser extension - once this pr is complete

I split the options text expression into two now, as @4ian suggested: HorizontalOptionsList and VerticalOptionsList

blurymind commented 4 years ago

Playing with this using it for signs, I now realize it would be useful to have a command to abruptly end a running dialogue- in the case of walking away from a sign. Will add it :)

blurymind commented 4 years ago

@4ian I am ready to start writing the wiki. The demo project is close to ready now too. How do I add a wiki page for my extension?

blurymind commented 4 years ago

updated project example dialogueExample.zip

it does avatarts too

blurymind commented 4 years ago

Updated demo - this time including some more cool features of the parser (like conditional dialogue). Managed to also catch a couple of minor, but annoying bugs.

updated the example: dialogueExample.zip Here is a screenshot example of how to set up an entire npc system with the extension: 2019-08-18_15-00 One with animated avatars and conditional branches

4ian commented 4 years ago

For making a page, just visit the URL and edit the page to create it: http://wiki.compilgames.net/doku.php/gdevelop5/all-features/dialogue-tree :) (read a few pointers about writing on the wiki here)

The example will be really nice!

blurymind commented 4 years ago

@4ian I registered an account at the wiki as blurymind, I don't yet have permission to edit : )

Edit: actually I think I can, just didnt do it right last time

Things to do before this is ready:

blurymind commented 4 years ago

@4ian I removed the flowfixme's, made the titles consistent and added the demo project to the examples folder. The onChangesSaved will need to be sorted out after I get back from travelling - 2 weeks from now. I need to carefully do that refactoring and test it extensively as it affects three editors so far. I agree that it stinks how it works right now and can be simplified. Just need more time

Will do the editor name in metadata when refactoring that function, as they are related

4ian commented 4 years ago

Great, thanks for the demo project! Let's merge this once you have time to look at onChangesSaved and add the editor metadata :)

4ian commented 4 years ago

@blurymind When you're back and have some time, let me know how it goes about onChangesSaved. Should not be that hard to clean and once it's done we can merge this :)

blurymind commented 4 years ago

@4ian i simplified the onChangesSaved now- so it only take one parameter - the resources. Also removed the redundant now flowfixmes. Was simpler than I thought. Semaphore test still failing for some reason - not sure why

Edit: my local flow is not working for some reason, need to update the flow file

blurymind commented 4 years ago

@4ian sorry about this, I have to use travis to fix the flow stuff, since my local flow checking is broken for some reason

blurymind commented 4 years ago

ah damn, I need to refactor piskel's way of passing data, since it uses the two extra parameters too :/

blurymind commented 4 years ago

ok I think I got the flow right now, please check if you can find any regressions in piskel's data handling and ability to store layers

blurymind commented 4 years ago

now I am getting flow checking to work locally, but for some reason it is now testing all extensions and giving me 9 errors from various extensions

4ian commented 4 years ago

What are these errors? Always include your errors in your message. Are you sure you're using flow by launching npm run flow/yarn flow?

4ian commented 4 years ago

I've just ran yarn flow on master and there should not be any issue. In case of doubt, stop flow (npm run flow -- stop or yarn flow stop), remove node_modules and reinstall and relaunch flow (npm run flow or yarn flow).

blurymind commented 4 years ago

this is turning out to be a bit of a pain in the neck to refactor. Will need to revisit it on a fresh mind tomorrow :)