re-working menus as a state-graph #76

Merged
merged 2 commits into from Dec 14, 2012

Conversation

Projects
None yet
2 participants
Contributor

IkarusDowned commented Dec 13, 2012

menu is now controlled by MenuStates, which act
as a state graph to describe the current options, menu
and active windows. it also helps better
describe the transition states available

pertains to
Bertram25#63

re-working menus as a state-graph
menu is now controlled by MenuStates, which act
as a state graph to describe the current options, menu
and active windows. it also helps better
describe the transition states available

started adding MainMenu and Formation states

added TODO

removed extranious Update() s

added FormationMode and filled out modes

a couple temporary changes

add all state skeletons

added my name (yay!) and slight big fix

updates to state machine moving properly now

solved transition crashes

non-active window drawing added.

bottom window drawing code moved to states

active windows, flow restored to original

code clean up and comments

While on it, can you this into a static? To avoid initialize it on each draw calls.

TODO and note to self:
Turn the time shown into a textbox member and update it in the update functions, not on each draw calls.

Same for drunes

A new line missing here. :)

A space missing here.
Btw, is this trivial enough to be put back into the header?

Are those two trivial enough to be put back into the header?
If not, could you reindent them anyway ?

Another newline missing here.

Missing new line and I wondered whether InventoryState::InventoryState(MenuMode* mode) is trivial enough to be put back into the header.

Could you add a new line and reindent?

Trivial enough?

Trivial enough?
Could you reindent?

Trivial enough?

same here + reindent.

useless spaces here

Please use const & for both parameters here while you're on it.

Just out of curiosity: Why does this member need to be static?

ah, thanks for nesting those. :)

Note to self: Remove the use of a std::map when a simple array or simple three members would suffice.

Owner

Bertram25 commented Dec 13, 2012

Wow, I've only done a grammar, coding style review so far and will test the new code live asap.

I must say that I like it very as it looks clean and very readable. :) 👍

Owner

Bertram25 commented Dec 13, 2012

did a functional review. Everything is working fine. I guess you can do the few cleanups I requested and we're good to go. I didn't look at it much carefully though but I still wonder why you're using a static member to get the state.

Great job, Ikarus. :D

Contributor

IkarusDowned commented Dec 13, 2012

np, will look at the comments tomorow (its near 1 am here) and fix / comment. thanks for the review!

Contributor

IkarusDowned commented Dec 14, 2012

@Bertram25
did some of the code cleanup as asked. a couple of points as per your comments:

  • the _ActiveWindowUpdate and _IsActive need to use the _menu_mode pointer, and due to the fact that the states are defined before MenuMode, the MenuMode class at that point is incomplete. In its current form, I can only put the forward decleration in the top of the header and use the pointers in the cpp files. There isn't a great way to solve this while still depending on MenuMode. There is one solution, but I'm not sure its worth it:
    1. I currently use MenuMode to hold the actual instantiations of OptionBoxes, windows and states. Essentially, it is the "manager" so that I can do all initialization / destruction in one area. If i move the individual windows out of MenuMode and into the individual states, then i cane move those two functions into the header file. On the other hand, there are other parts where it makes more sense to just go thru the _menu_mode pointer (IE, playing sounds) anyway, so the overall coupling of the classes wouldn't really have changed.

I'm happy to make the change tho, if you feel it is needed.

  • the static current_menu_state pointer is needed so that MenuMode can always call GetMenuState() with the proper current menu. The alternative is to move the current_menu_state pointer to MenuMode itself, and just update that. If you feel that makes more sense then I can make that change as well. Or, did you have a different idea in mind altogether?
Owner

Bertram25 commented Dec 14, 2012

Hi @IkarusDowned,

First of all, I'm aware I talking about technical way of doing cpp which no more than rubbish for certain people so thanks for answering.

the _ActiveWindowUpdate and _IsActive need to use the _menu_mode pointer, and due to the fact that the states are defined before MenuMode, the MenuMode class at that point is incomplete. In its current form, I can only put the forward decleration in the top of the header and use the pointers in the cpp files. There isn't a great way to solve this while still depending on MenuMode. There is one solution, but I'm not sure its worth it: 1) I currently use MenuMode to hold the actual instantiations of OptionBoxes, windows and states. Essentially, it is the "manager" so that I can do all initialization / destruction in one area. If i move the individual windows out of MenuMode and into the individual states, then i cane move those two functions into the header file. On the other hand, there are other parts where it makes more sense to just go thru the _menu_mode pointer (IE, playing sounds) anyway, so the overall coupling of the classes wouldn't really have changed.

Ok I see the point. No, you can leave it as is as the effort doesn't justify the effect, and there is worse in the current code we can take care before entering such technical refining level, if we ever enter it. Thanks for taking the time to explain.

the static current_menu_state pointer is needed so that MenuMode can always call GetMenuState() with the proper current menu. The alternative is to move the current_menu_state pointer to MenuMode itself, and just update that. If you feel that makes more sense then I can make that change as well. Or, did you have a different idea in mind altogether?

Yeah, that what's I had in mind. As it would feel more natural to me, do you mind pushing back menu_state pointer back to its "manager"? Feel free to tell me if it does bring some problems. And thanks a lot for the work done. :)

Ah, I forgot abut that one.

Owner

Bertram25 commented Dec 14, 2012

Anyway, I'll merge the feature as it is of now since it's working great and already well refined. You can do another pull request for the little change requested. Thanks again Ikarus :)

Bertram25 added a commit that referenced this pull request Dec 14, 2012

Merge pull request #76 from IkarusDowned/menu_changes
re-working menus as a state-graph

@Bertram25 Bertram25 merged commit 1aec41c into ValyriaTear:master Dec 14, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment