Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WINTERMUTE: Wintermute subengine for FoxTail game #1319

Merged
merged 24 commits into from Jan 11, 2020

Conversation

@lolbot-iichan
Copy link
Contributor

lolbot-iichan commented Aug 27, 2018

FoxTail is a point-and-click adventure game by Gingertips Game Studio, featuring extended and hacked WinterMute engine fork with closed sources. Most features are added by introducing new API methods and properties, but some are just hardcoded to be different from normal WME stuff.

I'd like to get some feedback on how I implemented those features while I look if I can do anything with known issues and re-test everything. Most arguable things are:
0. should new methods and properties be avaliable for other WME games or should we add if(BaseEngine::instance().getGameId() == "foxtail") guard everywhere in addition to subengine's #ifdef? Is if(BaseEngine::instance().getGameId() == "foxtail") the right way to detect FoxTail inside engine?

  1. most SX-objects don't have "normal" constructors, only those that are using values from ScStack. Is it ok to put use ScStack for casting integers into makeSXDate, etc?
  2. version properties - should they be the same as in actual games or return something else for debug purposes? Should we show ScummVM's version instead? What's the right way to get detected game version into the engine?
  3. is it ok to supress an error if everything seems to work fine? I guess I should do some investigations on LoadItems runtime error that I suppress.
  4. could I somehow avoid adding _minimizeSpacing to BaseFontBitmap to avoid messing with BaseFontBitmap::persist? I don't want to change sizeof(BaseFontBitmap).
  5. I just made _savedTimestamp field of BasePersistenceManager public. Should I add getter function instead?

Known issues:

  1. Dinamic lighting is working, but there is a semi-transparent rectangle around actor. See https://bugs.scummvm.org/ticket/10686
  2. Clock in grandma's room are not rendered correctly. See https://bugs.scummvm.org/ticket/10684
  3. Progress bar of game save/load is rendered above transparent loading_saving.png, not below as it hacked in FoxTail. Will look into this.
@lolbot-iichan lolbot-iichan force-pushed the lolbot-iichan:wme_foxtail branch from 0fd0270 to 395de38 Aug 27, 2018
@tobiatesan

This comment has been minimized.

Copy link
Contributor

tobiatesan commented Aug 27, 2018

Neat, thanks! Will give this a detailed look in the next couple of days.

Are savegames still broken, right?

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Aug 28, 2018

At the number of places this violates our code formatting conventions (http://wiki.scummvm.org/index.php/Code_Formatting_Conventions). Could you please address those? I could mark those spots if you struggling with locating them.

As to how to properly detect the game, I recommend checking against _gameDescription.adDesc. gameId. Normally, there would be a recommendation to add another GID (gameid), but as I see, Wintermute engine does not use that functionality. This will assure that you're not relying on the game data but have a consistent game id to check against.

@somaen

This comment has been minimized.

Copy link
Member

somaen commented Aug 28, 2018

  1. I lack a bit of context here, could you expand upon this with an example?

  2. What are the version-properties used for? If they are purely aesthetical, then we might as well just expose the ScummVM-version number.

  3. Probably no, unless you're certain that it's noise, in which case I'd still retain it as a debug-call so that we can get at it for debugging purposes.

  4. Don't worry about changing the size of BaseFontBitmap, just keep the new change behind a version-number check, and ensure that a decent (i.e. no-op) default value is set for older savegames. There will never exist older FoxTail games that lack this, and non-FoxTail-games should have no dependency on the value there anyhow as they should have behaviour as if the variable wasn't there.

  5. Presumably you only need to read it, so please make a getter.

@@ -56,6 +57,9 @@ BaseFontBitmap::BaseFontBitmap(BaseGame *inGame) : BaseFont(inGame) {
_fontextFix = false;
_freezable = false;
_wholeCell = false;
#ifdef ENABLE_FOXTAIL

This comment has been minimized.

Copy link
@somaen

somaen Aug 28, 2018

Member

In the interest of save game logic, I guess it might be preferable to unconditionally have this variable, but make it false for all non-foxtail games.

This comment has been minimized.

Copy link
@lolbot-iichan

lolbot-iichan Aug 28, 2018

Author Contributor

See comment below. Maybe _minimizeSpacing should be removed completely.

@@ -509,6 +518,11 @@ bool BaseFontBitmap::persist(BasePersistenceManager *persistMgr) {
persistMgr->getBytes(_widths, sizeof(_widths));
}

#ifdef ENABLE_FOXTAIL
if (!persistMgr->getIsSaving()) {
_minimizeSpacing = BaseEngine::instance().getGameId() == "foxtail";

This comment has been minimized.

Copy link
@somaen

somaen Aug 28, 2018

Member

Am I understanding this right? minimizeSpacing is always true in foxtail?

This comment has been minimized.

Copy link
@lolbot-iichan

lolbot-iichan Aug 28, 2018

Author Contributor

Yes. FoxTail has a globally enabled hack for making spaces between character and spaces between lines smaller than it is according to FONT settings. I added _minimizeSpacing to avoid checking if game is FoxTail on each BaseFontBitmap::getCharWidth().

Now when I think about it, maybe we should just make some globally avaliable bool at BaseEngine::instance() and remove _minimizeSpacing from BaseFontBitmap completely?

This comment has been minimized.

Copy link
@tobiatesan

tobiatesan Sep 1, 2018

Contributor

maybe we should just make some globally avaliable bool at BaseEngine::instance() and remove _minimizeSpacing from BaseFontBitmap completely?

I'd say it's a good idea.

Could this be the time to introduce some general way of marking features (including "bugfeatures") required/expected by a WME game?

@lolbot-iichan lolbot-iichan force-pushed the lolbot-iichan:wme_foxtail branch from df4d39c to b32cff2 Aug 28, 2018
@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

lolbot-iichan commented Aug 29, 2018

@tobiatesan
Savegames seems to be fine.
AdActor has no new persisted fields since I reimplemented SetSpeedWalkAnim using "frame->_delay = speedWalk".
The only new persisted field left is _minimizeSpacing at BaseFontBitmap, which seems unneeded too.

@sev-
I hope I fixed most code formatting violations with b956156. Please, let me know if I missed any more.
I currently use _gameDescription->adDesc.gameId for checking if current game is FoxTail.
I also use _gameDescription->adDesc.extra at aa97ca4 to check which exact FoxTail build it is.

@somaen
0. Sorry, I started list with "0.", so maybe you have missed a question. =)

  1. https://github.com/scummvm/scummvm/pull/1319/files#diff-935ae44957169f7436a484684ab18be3R1385 here it is. I push char* to make SXString from Common::String::c_str(), then I push 7 integers to make SXDate from TimeDate, then I push zero to make SXObject. This code looks extremely ugly to me, however I seriously doubt SXObject(), SXDate(TimeDate), SXString(whatever) constructors would ever be usefull outside of this code.
  2. They are purely aesthetical, see lower-left corner of https://imgur.com/a/UTVFWt7. Original properties are "1.2.230" and "1291" for 1.2.230.1291, "1.2.362" and "2047" for 1.2.362.2047. Those builds have slightly different walkthroughs, so seeing some version info may be useful.
  3. I'm not certain that it's noise, so I'll look deeper into what's wrong with LoadItems method and why it's absence does not affect gameplay.
  4. I made a second thought over BaseFontBitmap and now I think that _minimizeSpacing could be removed completely. See comments at review.
  5. Getter for timestamp is implemented.
#ifdef ENABLE_FOXTAIL
// FoxTail 1.2.230.1291 (English)
WME_WINENTRY("foxtail", "1.2.230.1291",
WME_ENTRY1s("data.dcp", "651ae5b062073021edaca7e1de131eec", 59357572), Common::EN_ANY, ADGF_UNSTABLE | ADGF_DEMO, LATEST_VERSION),

This comment has been minimized.

Copy link
@sev-

sev- Aug 29, 2018

Member

why is it marked as ADGF_DEMO?

This comment has been minimized.

Copy link
@lolbot-iichan

lolbot-iichan Aug 29, 2018

Author Contributor

Because it is a demo. Steam & GOG releases are currently "Game in development" / "Early Access Game" with only first chapter avaliable.

This comment has been minimized.

Copy link
@lolbot-iichan

lolbot-iichan Jul 14, 2019

Author Contributor

Currently there are 2 chapters avaliable.
Technically, this still is a "Game in development", but in fact this game is bigger then some finished Wintermute games.
So I'm not sure if it should be ADGF_DEMO...

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Aug 29, 2018

Thanks, the code formatting is fine now.

@lolbot-iichan lolbot-iichan changed the title WIP: Wuntermute subengine for FoxTail game - DO NOT MERGE YET WIP: Wintermute subengine for FoxTail game - DO NOT MERGE YET Aug 29, 2018
@tobiatesan

This comment has been minimized.

Copy link
Contributor

tobiatesan commented Sep 1, 2018

@somaen @sev-: tangentially, @lolbot-iichan has built a bunch of test games for the new features.
They currently reside in https://github.com/tobiatesan/wme_testsuite, but do you think that those could be moved somewhere under github.com/scummvm/ (after cleaning it up a bit)?

@sev-

This comment has been minimized.

Copy link
Member

sev- commented Sep 2, 2018

@tobiatesan that is a great idea, we could treat them as demos

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Dec 2, 2018

Any news on this?

@tobiatesan

This comment has been minimized.

Copy link
Contributor

tobiatesan commented Dec 3, 2018

@bluegr Sorry for the extended away, this has been a terrible couple of months.
I haven't heard from @lolbot-iichan... yet :)

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Mar 2, 2019

And... 3 months have passed. Any news on this? After the reviews, are the changes in a state that can be merged?

@lolbot-iichan lolbot-iichan force-pushed the lolbot-iichan:wme_foxtail branch from b6197ae to 825c743 Jun 3, 2019
@lolbot-iichan lolbot-iichan changed the title WIP: Wintermute subengine for FoxTail game - DO NOT MERGE YET WIP: Wintermute subengine for FoxTail game Jun 3, 2019
@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

lolbot-iichan commented Jun 3, 2019

Sorry for being missing for such a long time... Anyway, 2nd chapter of FoxTail was released and here I am again, with a new bunch of changes...

2nd chapter's engine is a bit different not only from original WME Lite, but it's different from 1st chapter's FoxTail engine as well. So, I had to add engine versioning (see a39d89a). Commit c629f1f features example of version-depending addition to WME.

There are still some glitches (I don't know how to fix saveload progress bar with dirtyrects), there are still some drawing related bugs posted to bugs.scummvm.org that affects FoxTail), but this pull request allows to play FoxTail, and I'd like to continue work on subengine after reviewing and merging this.

@tobiatesan

This comment has been minimized.

Copy link
Contributor

tobiatesan commented Jun 8, 2019

Thank you for your work.

Have you considered disabling dirtyrects altogether while saving? It's hacky and I'm not saying it's a great idea, but...

lolbot-iichan added 16 commits Jul 14, 2019
Game is not playable with standard Wintermute, so we should stop player
from even trying to run it if ENABLE_FOXTAIL is off.
There are two new methods: immidiate stop and changing walking speed
For stopping, I'm reusing turnTo() code to finalize moving.
I talked to FoxTail devs about what SetSpeedWalkAnim actually does and answer was to set animation delays with given number of milliseconds.
FoxTail have flexible cursor settings system that require things like toggling visability of inventory items.
FoxTail requires access to SubFrame's pixels to set actor.AlphaColor
with lighting map pixel value at x,y of actor's position.
Game.Split(str,sep) is added to split string into array of substrings with separator
array.Delete(index) is added to delete an array item by index
FoxTail code is hacked to use key codes from different from usual WME.
Got correct mapping using script decompiling, verified with sample game.
FoxTail use 2x, 3x, 4x, etc so-called "screen modes", that are switched between in Options menu.
Current implementation makes FoxTail no choice but to think that 640x360 is the only option avaliable, since we don't want game engine to mess with ScummVM's render filter settings.
FoxTail requires Game.RegistryFlush() method to force saving of persistent settings
FoxTail requires Game.GetSaveSlotDescriptionTimestamp for getting
information about saved games.
Engine 1.2.362 and below returns object with Description and Timestamp
fields.
Engine 1.2.527 and above returns array[2] with Description and Timestamp
as items.
FoxTail fork of WME seems to have hard-coded modifications for getting inter-line and inter-character spaces smaller.
FoxTail fork of WME seems to have hard-coded modifications for dialogue UI.
Buttons are ignoring hover-font and pressed-font settings.
Buttons also have hard-coded-path scripts attached to them.
Some FoxTail versions have bitmap fonts with white letters and COLOR properties.
Alpha value of subframe can be used to recolor white letters to desired color.
FoxTail require SCENE's ENTITY to support additional HINT_X and HINT_Y properties.
Those properties are accessed by scripts to display hints: node_object.HintX.
FoxTail reloads in-hand items and cursor during AfterLoad by calling
wrapped Game.LoadItems() method.
This does not work in WME, as WME explicitly disables methods call at
"AfterLoad" event.
Somehow, everything seems to work fine without it. Let's supress this
error by now...

TODO: inspect actor.TakeItem and SetItemCursor for possible effects
TODO: make a fake game that calls some methods on AfterLoad, test with FoxTail engine
Wintermute games usually display save/load progress bar as a rectangle drawn above an image.
FoxTail's progress bar is drawn below semitransparent image instead.

TODO: I have no idea how to make this work with "dirty_rect=true" mode.
@lolbot-iichan lolbot-iichan force-pushed the lolbot-iichan:wme_foxtail branch from acfee72 to 2be4dde Jan 4, 2020
@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

lolbot-iichan commented Jan 4, 2020

Changes since Summer 2019:

  1. Everything rebased, conflict resolved.
  2. isFoxTail(FOXTAIL_1_2_527, FOXTAIL_LATEST_VERSION) check added while persisting HINT_X & HINT_Y properties (added in FoxTail engine fork for 1.2.527.x games). Surprisingly, it seems to be the only line needed to make savegames of my branch and master branch compatible.
  3. Fixed minor issues and typos listed in comments.
I discovered, that difference in key mappings between FoxTail and usual
games is not due to FoxTail's engine differences from WME Lite, but were
made in WME Lite itself! WME 1.x is using "Virtual-Key Codes" mapping
and WME Lite and FoxTail are using "SDL_Keycode" mapping.

So, here is total refactoring of keycode handling.

Why nobody noticed this earlier? Because there are only 4 currently
known series that are based on WME Lite (J.U.L.I.A. series, Reversion
series, FoxTail and Securanote) and because most WME games, both
1.x-based and Lite-based are using Keyboard class only for handling
Enter and Escape keys, which are the same in both mappings (Backspace,
Space and Tab are also the same).
@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Jan 11, 2020

Excellent! Thanks for fixing all the issues, and many thanks for your work on this.

This can now be merged. Great work

@bluegr bluegr merged commit 720aea5 into scummvm:master Jan 11, 2020
1 of 2 checks passed
1 of 2 checks passed
Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@lolbot-iichan lolbot-iichan deleted the lolbot-iichan:wme_foxtail branch Jan 13, 2020
@lolbot-iichan

This comment has been minimized.

Copy link
Contributor Author

lolbot-iichan commented Jan 13, 2020

Thanks for reviewing, suggestions and helping this happen! I'm really happy to have Foxtail subengine at last integrated to master and I had extremely good time today playing FoxTail on Nindendo Switch with nightly build of ScummVM =)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.