Brookke / SEPR-Assessment2

1 stars 6 forks source link

Speech Box #88

Closed jasonmash closed 7 years ago

jasonmash commented 7 years ago

Fixes #46, an updated version of pull request #67

Brookke commented 7 years ago

Would it be worth adding a timeout value to this, i.e. a value we give to the speechbox that is how many seconds it should be displayed for before being removed/hidden? -1 would be show always. We can then have some sort of stack of speech boxes that the screen iterates through showing each one for their timeout period before removing them from the stack and moving onto the next?

jasonmash commented 7 years ago

@Brookke I think we should add the timeout eventually, but when we have a feature that needs it.

Not sure it's essential at the moment (correct me if I'm wrong), so I'd suggest putting a new task in the "stuff for later" column on the project.

Brookke commented 7 years ago

@jasonmash Yeah the conversations in general dependant on how we implement it will need it, also how do we currently hide the box?

jasonmash commented 7 years ago

That's a good point - perhaps on the last speechbox of a conversation, the only possible responses are saying "Goodbye" in a variety of different personalities, then when clicked the box disappears. What are you thinking?

Brookke commented 7 years ago

I just mean how would you get to the last speechbox? surely each one needs to be displayed for a certain amount of time then replaced by the next? e.g.

Brookke commented 7 years ago

So one option is you can use the space bar or down arrow to go to the next, but then you still need some sort of stack to manage that and having it flow automatically would also be nice.

jasonmash commented 7 years ago

Ahh I see where you're coming from. I was imagining this sort of flow:

Brookke commented 7 years ago

Ah I see, yeah that works too although how does the game know to move on from the first bob box? I do think some sort of list/stack would be good to manage this, the conversation class could hold this list and add to it dependant on responses etc. (I know we are just talking about the ui element at the moment, however it is useful to work this stuff out early on)

jasonmash commented 7 years ago

The game knows to move on when the player clicks the response buttons underneath Bob's speech. That might require some restructuring of the SpeechBox UI elements, but it's not too hard to do.

Yeah the conversation stack/list should definitely be held in a different place - as you say, the SpeechBox is for the UI only. We should probs discuss it in #89

jasonmash commented 7 years ago

What's the best way of implementing the dynamic buttons? We could have an array of SpeechBoxButton objects, with properties like (Name, Position, EventHandler), but I'm not sure how we're going to deal with the EventHandler part as you can't pass functions around as variables like you can in JavaScript

jasonmash commented 7 years ago

Just had a thought, as @beno11224 said, I'm not sure we want the SpeechBox to take an AbstractPerson parameter. The reason for this is the SpeechBox is just a UI control, like a label, so it shouldn't have any logic inside of it, which passing in an AbstractPerson would involve. It should just take the raw data it needs to render correctly, which should be strings

My current thoughts for the SpeechBox constructor are: new SpeechBox( String personName, String speechText, List<Buttons> responseButtons ).

And the SpeechBox should only have one layout, rather than the two currently, consisting of a UI element arranged like so:

The List<Buttons> should contain an ordered array of objects with these parameters:

How does that sound?

Brookke commented 7 years ago

@jasonmash passing the person doesn't necessarily equal logic. Having just layout will mean compromises when we want to display lots of text (eg at the intro screen if we were to give instructions to the player) Or if we want to use it just to display text, how does that work? Will there be a gap where the buttons would have been placed?

jasonmash commented 7 years ago

Hmm, not sure passing in the person isn't logic, as the SpeechBox will need to perform the NPC.getSpeechText() and NPC.getPersonName() logic to get the strings needed to render. If it is a UI element, those two values should be calculated before being given to render. It will also allow us to display text as you mention earlier.

Not sure the layout will involve compromises, it can adapt based on the input the SpeechBox is given, e.g if the Button list is empty, it will not show the button row, so there's no gap. There are ways to align UI elements dynamically.

@beno11224 we've still got changes to make, as we need a different layout for speech because the current version doesn't have buttons (see my earlier comment). We also need to implement rendering the button list + event handlers.

Brookke commented 7 years ago

For the name It'd just be returning a pre calculated string there is no logic going on in the class, that would always be the same and never change, its not having to run any thing to generate or using if statements or calculations

jasonmash commented 7 years ago

Okay sure, I get that the PersonName is just a constant. Using that as an example, I'm not convinced that NPC.name belongs inside the SpeechBox, I think it should be passed into the SpeechBox when it's defined.

So we'd do new SpeechBox(npc.name, ...) rather than new SpeechBox(npc, ...) because that way we can provide an alternative source for the PersonName variable. This is useful because the SpeechBox is a dumb UI component, and shouldn't make assumptions about the data source it needs to render (i.e that it will always be an AbstractPerson passed in).

Brookke commented 7 years ago

Okay thats fine :)

jasonmash commented 7 years ago

The SpeechBox will need to present the Question, Accuse, Ignore buttons as well. The code is 90% the same, but the top text row would need rendering differently (a simplification of the PersonName & Speech case).

Should we have two constructors (i.e one with and without the PersonName parameter)? Just trying to work out the best way of doing it, I'm open to other suggestions.

Brookke commented 7 years ago

@jasonmash Yes I would probably have two separate constructors :)