Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upMOHAWK: MYST: Turn held page state into an enum #1165
Conversation
|
Our code formatting conventions require that there be no whitespace after a cast. Then again, why not change |
|
I can make that change in /engines/mohawk/myst_state.h#L84, but how would I make that work with syncAsUint16LE() in engines/mohawk/myst_state.cpp#L313? |
|
IIRC the serializer's |
| @@ -1148,40 +1148,40 @@ void Myst::o_bookGivePage(uint16 var, const ArgumentsArray &args) { | |||
| uint16 mask = 0; | |||
|
|
|||
| switch (_globals.heldPage) { | |||
This comment has been minimized.
This comment has been minimized.
bonki
Apr 23, 2018
Member
This now needs an empty default case or will otherwise generate a compiler warning because of unhandled enum values.
This comment has been minimized.
This comment has been minimized.
dafioram
Apr 23, 2018
Author
Member
That is an easy fix.
Interestingly case 13/kWhitePage is handled separately above and not in the case statement.
This comment has been minimized.
This comment has been minimized.
|
Done. |
| @@ -121,13 +121,13 @@ bool MystGameState::load(int slot) { | |||
|
|
|||
| // Set our default cursor | |||
| _vm->_cursor->showCursor(); | |||
| if (_globals.heldPage == 0 || _globals.heldPage > 13) | |||
| if (_globals.heldPage == kNoPage || _globals.heldPage > kWhitePage) // TODO: This second condition can never happen | |||
This comment has been minimized.
This comment has been minimized.
| if (_gameState->_globals.currentAge == 2) | ||
| redrawArea(25); | ||
| } else if (page == 10) { | ||
| } else if (page == kRedStoneshipPage) { //TODO: Should this also happen for the blue page above? |
This comment has been minimized.
This comment has been minimized.
bgK
Apr 24, 2018
Member
This code is checked against the disassembly, so no, it does not need to happen in other cases.
| uint16 bookVar = 101; | ||
| uint16 mask = 0; | ||
|
|
||
| switch (_globals.heldPage) { | ||
| case 7: | ||
| case kNoPage: |
This comment has been minimized.
This comment has been minimized.
bgK
Apr 24, 2018
Member
Here you can collapse the kNoPage and kWhitePage cases. Also the break can be removed since return already exits the function.
|
Done. Ignoring the fireplace pages, any idea why the kRedStoneshipPage is redrawn differently then the other red pages when it is dropped and the current age == 1? While the blue page in the stoneship age redrawArea is handled like the other blue pages being dropped? |
| uint16 bookVar = 101; | ||
| uint16 mask = 0; | ||
|
|
||
| switch (_globals.heldPage) { | ||
| case 7: | ||
| case kNoPage: | ||
| // fallthrough |
This comment has been minimized.
This comment has been minimized.
bgK
Apr 24, 2018
Member
No need to put a fallthrough marker if there is no code in the case. It's used to tell the compiler we didn't forget a break. Here it's obvious the fallthrough is intentional.
| if (_gameState->_globals.currentAge == 2) | ||
| redrawArea(25); | ||
| } else if (page == 10) { | ||
| } else if (page == kRedStoneshipPage) { // This code was checked against the disassembly and |
This comment has been minimized.
This comment has been minimized.
bgK
Apr 24, 2018
Member
The whole thing is checked. It does not make sense to add that comment here.
The red stoneship page case is different because there are three cases. The drawer is closed. The drawer is open but there is no page, the drawer is open and there is a red page. See . |
| _vm->setMainCursor(kRedPageCursor); | ||
| else // if (globals.heldPage == 13) | ||
| else // if (globals.heldPage == kWhitePage), i.e., the white page is held |
This comment has been minimized.
This comment has been minimized.
Previously, the held page stage was an unsigned int 16 with values 0-13. The enum will make its state more clear. I have also noted interesting behavior as TODO's.
Two of the cases were being handled before the switch statement and I moved them into the switch statement. This prevents a compile warning of not all enums being used.
Two TODOs were added two commits ago and this commit addresses both of them. The first deals with a conditional checking if a heldPage is greater than the kWhitePage which can never happen since the kWhitePage is numerically the last page. The second was a curious cases were a set of conditions for droping a page did't have the same checks for checking dropping a blue page as it did a red page. It matches the disassembly.
|
Done. |
|
Very good, thanks |
dafioram commentedApr 23, 2018
Previously, the held page stage was an unsigned int 16
with values 0-13. The enum will make its state more clear.
Because enums are implicitly ints in c++03 and _global->heldPage
is an uint16 I have to typecast to uint16 when ever I compare
the enum. This is not ideal, but I still think the enum with
the cast is more clear.
I have also noted interesting behavior as TODO's.