Feature/portrait system refactor #490

Closed
wants to merge 22 commits into
from

Projects

None yet

2 participants

@lealeelu
Contributor
lealeelu commented Jun 9, 2016

Move most of the logic of the Portrait command into a Portrait Controller.

@lealeelu
Contributor
lealeelu commented Jun 9, 2016

Not done yet!
I'm having trouble with the Continue() from Portrait.cs. I think it's messing up the Narrative/PortraitTest.

@chrisgregan
Contributor

Looks good, going in the right direction for sure. Continue() needs be called at the correct times back on the Portrait command. Easiest way to do this would be via a delegate, Action would do the trick - like this.

stage.getPortraitController().RunPortraitCommand(options, () => {
     Continue();
});

public void RunPortraitCommand(PortraitOptions options, Action onComplete)
{
...
    onComplete();
    return;
...
}
@lealeelu
Contributor
lealeelu commented Jun 9, 2016

Cool, I'll add that tonight.

@lealeelu
Contributor

The tests pass now but they look a little strange. Sometimes the portrait turn entirely white.

@lealeelu
Contributor

Everything seems to be back in working order. If it looks good to you, I'm going to start working on helper functions.

@lealeelu
Contributor

What's a good way to test the helper functions? Is there anything else I need to do before I move onto lua and documentation?

@chrisgregan
Contributor

Looks great! I'll do a proper line by line review over the weekend and commit, but on first pass through look it looks spot on.

You can add some new tests yourself for the new helper functions, add a new scene under the Tests/Narrative folder. The Create button on the Integration Test window will create a new test. I find it easiest these days to use Lua to perform the tests, but you can also use the IntegrationTest component or a c# script - whatever you prefer.

I usually use the assets from the Sherlock demo to test portrait system, but you can add your own if you like (assuming they're properly open source licensed).

@lealeelu
Contributor
lealeelu commented Jun 10, 2016 edited

Yay! Thanks!
--copied old tasks down lower

I'm so glad to see it mostly working ^_^

@chrisgregan
Contributor

Sounds good. If you want to update the docs send in a pull request on the fungus-site repo and then I'll pull an update to the website. Thanks for all the hard work!

@lealeelu
Contributor

Hope I didn't mess up anything with that push. I did a pull rebase with the master branch to catch up.

@lealeelu
Contributor

I'm having a heck of a time figuring out why the hide command is so spastic. I put in an example at FungusExamples/FungusLua/Narrative if you want to see what I mean.

I think I'll punt on this for a day or so if you want to take a crack at it.

@lealeelu
Contributor

nvm figured it out. Struct variables automatically get assigned values that I didn't expect:
null for reference types, 0 for numeric types, false for bools, etc.

@chrisgregan
Contributor

Cool, prob be Monday when I get to look at merging this in

Sent from my iPhone

On 11 Jun 2016, at 20:53, Leah Lee notifications@github.com wrote:

nvm figured it out. Struct variables automatically get assigned values that I didn't expect:
null for reference types, 0 for numeric types, false for bools, etc.

โ€•
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

lealeelu added some commits Jun 11, 2016
@lealeelu lealeelu Fix FinishCommand d4cf67d
@lealeelu lealeelu Add simpler show function 6e1cf63
@lealeelu lealeelu Add move to portrait options 197c1c9
@lealeelu lealeelu Add string search for stage position and character portraits
That way you don't have to add everything to the Lua Bindings. We
already know which stage and which characters are being used, no use in
making extra work for the user.
5c6ff57
@lealeelu
Contributor
lealeelu commented Jun 12, 2016 edited

Ok, for real this time I'm going to put this down and work on something else for the next couple days. Give you some time to make heads or tails of it.

Here's the current TODO:

  • Figure out what to do with the FinishPortraitCommand bit - it's buggy
  • The portrait controller's functions aren't showing up in the LuaBinding's Member Info dropdown.
  • I want to be able to do one of these in lua stage.show{character=sherlock, portrait="angry", position="left"} but I'm not sure how to do it. I'm overloading the show function more than I feel comfortable with.
  • Finish Lua Stage Control example.
  • Create new Scene in Tests/Narrative folder with Lua tests of the helper functions
  • Write some documentation on how to use it and point to the example project - but this needs to be added to the documentation repository
@chrisgregan
Contributor

Just pushed an option to show inherited members, so you can now inspect the portrait controller members

@lealeelu
Contributor

Cool! That will be handy.

@chrisgregan
Contributor

Hmmm.. I seem to have pushed that to a new branch, will sort it out in the morning

@chrisgregan
Contributor

I pushed that change to your lealeelu:feature/portrait-system-refactor branch. Is there a way to make that change part of this pull request? Happy to just commit it to master either on my end. I'm gonna be honest here - git kinda melts my brain :{

@chrisgregan
Contributor

btw I noticed an error in the console in the PortratControllerExample scene.
"Unidentified stage position: bored"

I think it's related to your point about overloading the show function. I'd lean towards having different versions of show (e.g. showat, showportrait) or conceptually seperate fucntions altogether rather than overloading it to much.

@chrisgregan
Contributor

Another option would be to use named arguments
These are good when the call signature gets complex or has lots of optional parameters.

@lealeelu
Contributor
lealeelu commented Jun 14, 2016 edited

I remember the last pull request I did I thought I saw you were able to commit to my pull request. strange. Maybe I saw that wrong.

Anyways! I could start a new pull request to the branch you have called lealeelu:feature..blah blah
since at this point it feels mostly like cleanup stuff is left to be done.

I would like to use the named arguments. It just feels more intuitive. I'm thinking I would add something like this to fungus.txt:

function show (arg)
    return stage.show(arg)
end

and use it like this:

show{character="sherlock", portrait="angry", fromposition="right", toposition="left"}

Can you just pass args like that? We might need to have a stage lookup in case a stage isn't added to the luabindings...

The showportrait one sounds good too. I'll think about that some more.

@chrisgregan
Contributor

Ok, that's prob easiest - just include my changes in that new pull request.

You can pass args like that if the receiving function takes a MoonSharp Table as an argument. I'm fine with Fungus having dependencies on stuff in FungusLua (like Table) but I really want to avoid dependencies in the other direction so that FungusLua can always be used independently of the rest of Fungus.

@lealeelu lealeelu Add ShowPortrait and removed RectTransform uses
- added a showportrait but also added a show with the portrait option.
- fixed up the portrait control example so we can see the new, working
functions in use.
- Simplified the hide since we only need to know where it's going off to
hide
08fcb3f
@lealeelu
Contributor

I was able to get the Moonsharp table to pass values into c# but the table is really hard to work with. Maybe converting it to a list to iterate through would be nice but I can't access this the TableConversions to do it: https://github.com/xanathar/moonsharp/blob/master/src/MoonSharp.Interpreter/Interop/Converters/TableConversions.cs
Is there another way to do it?

@chrisgregan
Contributor

Is the Table class that hard to work with? You query the DynValues you need from the Table by parameter name and then use the correct accessor to get at the C# value - should just be a few lines of code per property. Not at my laptop to check, but there should be a few examples of this in the FungusLua source.

Sent from my iPhone

On 15 Jun 2016, at 20:00, Leah Lee notifications@github.com wrote:

I was able to get the Moonsharp table to pass values into c# but the table is really hard to work with. Maybe converting it to a list to iterate through would be nice but I can't access this the TableConversions to do it: https://github.com/xanathar/moonsharp/blob/master/src/MoonSharp.Interpreter/Interop/Converters/TableConversions.cs
Is there another way to do it?

โ€•
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@lealeelu
Contributor

I just thought it would be nice to iterate through each supplied value and use a switch statement instead of checking if each possible value exists in the Moonsharp Table. Just being picky. I was actually able to get it to work. It just looks messy. I'll push it up so you can see what I mean.

@chrisgregan
Contributor

Maybe you could wrap up the gunk in a utility class that takes a show args table and exposes the data in a nice clean way for the rest of the system to work with?

@lealeelu
Contributor

null checks for everyone! I was using fancier stuff like ?? operators but this straightforward method worked best.

I'll put together some tests.

@chrisgregan
Contributor

Had a look through yesterday's commit, looks good. What's left on the todo list?

I'm away for a few days in Spain at the minute, but would be good to get this all merged in next week sometime. I fixed some bugs on the plane over so thinking of pushing another beta release next week - would be great to include your stuff.

Sent from my iPhone

On 17 Jun 2016, at 14:42, Leah Lee notifications@github.com wrote:

null checks for everyone! I was using fancier stuff like ?? operators but this straightforward method worked best.

I'll put together some tests.

โ€•
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@lealeelu
Contributor
lealeelu commented Jun 17, 2016 edited

Awesome! I think the only things left to do are...

  • Write tests - should have done it before rather than after but I'm still kinda getting used to it.
  • Write Documentation and add to the fungus-site repo

I think I can get those done this weekend. Anything else you can think of?

@chrisgregan
Contributor

I noticed the portrait controller example was using the show command with the parameters rather than the table version, maybe show how to do it with named args as well? Apologies if that's already done - can't check here!

Would be nice if you could add member comments (just summary is fine) on the new class. I want to start doing a better job of documenting the API myself.

Sent from my iPhone

On 17 Jun 2016, at 17:03, Leah Lee notifications@github.com wrote:

Awesome! I think the only things left to do are...

Write tests - should have done it before rather than after but I'm still kinda getting used to it.
Write Documentation and add to the fungus-site repo
I think I can get those done this weekend. Anything else you can think of?

โ€•
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@lealeelu
Contributor

I think the Portrait Example is done. It has both a show command with named parameters and an example using the new c# commands.

@lealeelu
Contributor
lealeelu commented Jun 18, 2016 edited

That PortraitControllerTest isn't passing. I'm getting a strange error message that I haven't been able to figure out. Should I actually put this test in the Lua section instead of Narrative?

I'm going to switch to doing documentation and give you time to look at test stuff.

@chrisgregan
Contributor

Lua section might be better yeah. Won't get a chance to look at that error until next week, want to post that message here and I might know what it is?

Sent from my iPhone

On 18 Jun 2016, at 02:26, Leah Lee notifications@github.com wrote:

That PortraitControllerTest isn't passing. I'm getting a strange error message that I haven't been able to figure out. Should I actually put this test in the Lua section instead of Narrative?

โ€•
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@lealeelu
Contributor

The error was because the copy and paste I did between scenes wiped out some of the values along the way. Just had to add some stuff back to the LuaBindings.

@chrisgregan
Contributor

Ah ok - cool

Sent from my iPhone

On 18 Jun 2016, at 22:16, Leah Lee notifications@github.com wrote:

The error was because the copy and paste I did between scenes wiped out some of the values along the way. Just had to add some stuff back to the LuaBindings.

โ€•
You are receiving this because you commented.
Reply to this email directly, view it on GitHub, or mute the thread.

@chrisgregan
Contributor

Will start into merging this tonight - good to go on your end?

@lealeelu
Contributor

Yup! Good to go! Yaaaaaay!

@lealeelu
Contributor

I also have some documentation in fungus-site

@chrisgregan chrisgregan added a commit that referenced this pull request Jun 22, 2016
@chrisgregan chrisgregan Merging PR #490 a0b11e0
@chrisgregan
Contributor

Those changes are merged in now #500. Will do the docs next.

@lealeelu lealeelu deleted the lealeelu:feature/portrait-system-refactor branch Jul 10, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment