Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Some Myst improvements and deifinitions for Phantasmagoria french #170

Closed
wants to merge 7 commits into from

7 participants

@monnerat

Thanks for merging these patches into the main repo if you believe their quality is good enough :-)
Regards
Patrick

monnerat added some commits
@monnerat monnerat MOHAWK: Various fixes around the Myst selenitic age sound receiver:
        Repeat reate
        Initial position
        Sound termination
ea9fb8b
@monnerat monnerat MOHAWK: Myst extensions option flag.
MOHAWK: keep telescope position while staying in the Myst stoneship age.
f1a9556
@monnerat monnerat MOHAWK: rename MystResource "type" member to "_type". a5c6ff7
@monnerat monnerat MOHAWK: implement common code for Myst resource indexing. c341c7f
@monnerat monnerat MOHAWK: new graphics procedure copyClippedImageToScreen().
MOHAWK: complete rework of Myst observatory time controls:
        Handle each of the four controls in a separate object class.
        Determine date format from month display position.
        Fix sound in control preset.
        Adjust sound duration of star panel animation.
        Differentiate controls increase/decrease op-codes.
        Light-off button when not blinking.
        Allow control both by position and by function (D,M,Y,T).
9371a71
@monnerat monnerat MOHAWK: make Myst mechanical age elevator sound from outside looping. 98dedbd
@monnerat monnerat SCI: add Phantasmagoria 1 DOS french to game detection tables. a877938
@clone2727
Collaborator

The SCI commit should not be part of this pull request.

@clone2727
Collaborator

What is this extensions flag? What does it do?

Collaborator

monnerat, firstly thanks for looking at these issues and providing code patches, rather than just complaining :-)

Based on my reading of this code, this is an attempt at an "improvement" on the original interpreter.

In the original interpreter, when you move away from looking through the Telescope on Stoneship, it moves back to the 0 deg position, rather than staying at the last position... This patch changes this so when this "extension" flag is enabled, it stays at the last position. (I've always viewed this more likely that the Stranger is neat and moves the telescope back after he stops looking though it, rather than magic pixies put it back behind you back... The real technical reason from Cyan's POV IMHO was to avoid having vastly complex world state to render i.e. see the discussions in the Making of Riven about the Rotating Room on the start Island).

I should say that we already have a flag in the Mohawk Myst codebase for "Improved vs. Same as Original" behaviour, adding an "extensions" flag is redundant. The older flag is the engine flag "_tweaksEnabled", which is used to add the missing Myst flyby movie and is currently hardcoded to true/enabled.

However, if this is to be changed at runtime, then this would be better as a per-engine boolean in the scummvm config file as per SCI dithering etc., rather than as a savegame field... as clone2727 points out since it breaks savegame compatibility.

Collaborator

@digitall: Yes, but I am not a fan of that flag to begin with and would prefer it gone in the long run.

Collaborator

Shrug.. I suspected that we might have a fair few cases where we have arguments over whether we should "improve" the engine or provide the same behaviour as the original, so this flag allows us to avoid anoying people by forcing a choice on them. (Though we can still enforce a preferred option by the default).

Previously in other engines, these have mainly been bug fixes and fairly uncontroversial i.e. very few fans/players want the crash/gfx foulup of the original, so this kind of boolean is not warranted, but it gets trickier when you are making more "aesthetic" decisions, which some fans want the new and improved and others want to play it as the original (Yes, I know they could use DOSBox for that, but you know my point) so you need to provide a config flag for this.

A good example where this has been done previously is the SCI dithering/undithering config option.

Basically I would prefer this as a config option, but this is up for discussion.

@clone2727

Doesn't this break every save file?

Collaborator

It definitly should cause the warning at line 314 "warning("Unexpected File Position 0x%03X At End of Save/Load", s.bytesSynced());" to be trigged all the time (for new saves at least) I think.

@monnerat

The extension flag controls whether the behavior is according to the original Myst or "improved" Myst. This has been discussed and requested by e-mail by bgK.
For now, the only extension it controls is the latching of the stoneship telescope position: original Myst always restarts at zero.
And yes, it may break save files, although the tests I performed were OK with old ones.

@clone2727
Collaborator

The commit message on this is ill-formatted. In addition, it's misleading since it doesn't say what this actually does on the first line.

Collaborator

copyClippedImageToScreen seems to be a method for safety. But it does not seem to handle the case where the source rect is an invalid rect as far as I can tell.

copyClippedImageToScreen() is not a safety method: it allows to display the part of an image "windowed" by a given rectangle.
Its primary use is for the observatory controls: the 4 up resp. down button pictures are kept together in a single image that is a partial capture of the (modified) screen image; the button offsets in this image are thus all relative to the same screen offset. In order to change the state of a single button, the current (i.e.: master) code deals with a lot of absolute offsets, for each case.
It is easier to display the full image at its fixed capture offset and clipped by the button's rectangle,
Test for invalid rectangle might indeed by added.

@clone2727
Collaborator

@monnerat: Well, first off, I don't think that we should "improve" the game in any way; we're here to reimplement the game as close to the original interpreter as possible.

Secondly, if that's what the original does, we should do it to.

Third, it breaks saves and I am not OK with that.

@bgK

Is it safe and portable to initialize a class by setting its memory to zero?

Collaborator

Nope, only POD variables can be safely cleared that way AFAIK

Collaborator

It is not safe in general. In this case it seems the ObservatoryControl contains a non-POD type, i.e. Common::Array, which by chance initializes all its members to zero in its default constructor. That is the reason why it probably works by chance in this setting. But this should really be changed, since any changes to Array or adding other non-POD members to this class might result in things going bad.

Collaborator

As another remark: this should really read "sizeof(*this)" with our conventions. But since this memset should be gone altogether it's just a remark for reference.

@lordhoto

No memset here either.

@lordhoto

Please don't use qsort on container classes, this defeats the abstraction level introduced with containers. Use Common::sort instead, which actually gives you the same guarantees as qsort.

Thanks for the hint. I didn't even know scummvm has its internal sort support.

@lordhoto

Is there any special reason why this isn't using src.width() and src.height()?

No special reason: just that I'm more familiar with C that with C++

@lordhoto

This "}" looks like it's on a completely wrong position. Maybe this code isn't using tabs properly?

No. No tab problem. Just a mistyping.

@bluegr
Collaborator

Just a side note: personally, I'm all for improving the original games, if possible, and we have several occasions where we do that (e.g. we fix bugs in the original games, we add enhanced save/load dialogs etc). So, if the savegame format isn't changed, I don't see why ScummVM shouldn't be enhancing parts of the original games to provide a better end user experience. Such enhancements could be per-engine options, that the user can toggle (once the per-engine options code is in master, that is).

@clone2727
Collaborator

@bluegr: The original game developers didn't intend the game to be played this way, so we shouldn't put our own two cents into it. If we could use the original save/load dialogs in Myst/Riven, I would love to have it in there. I am against any such "enhancements" (which are very much in the eye of the beholder, btw).

@monnerat

Thanks to all of you for these comments.

As some of these commits block the whole pull, I'll resubmit them as separate pull requests.
I'll fix the blocking ones as suggested in your comments.

Concerning the "extension" patch:
It seems there is a lot of different opinions about those kind of changes. Of course, people wanting to play game as near the original as possible should be able to do it. On the other side, odd original behaviour might be altered here because we have control on the source.
I agree that breaking the old save files is not pleasant. But the Myst game support is not complete, as warned by the pre-start screen.
However, having a per-engine flag rather than a per-game flag has the inconvenient of affecting all the games supported by the engine, and being less easy to change for the user.
The _tweaksEnabled approach is probably not satisfactory yet, as it is not configurable. But it would probably be the best solution (for flag globalisation) once it would be turned into a truly configurable flag.

I'll be pleased to implement such a configuration option once all of you agree on a policy.

@bgK
Collaborator

@monnerat The save format for Myst is the same as the original engine. That means ScummVM can load saves written by the original engine. It's something we really don't want to break. Changing the saves format is a big no go.

About the "improvements", I have no strong opinion, as long as they are optional and disabled by default. That setting should however be saved per game in ScummVM's config file. You can take a look at what the other engines do to achieve that result. There is also ongoing work to add a common GUI for those settings, see #169.

@clone2727
Collaborator

@monnerat: bgK pretty much summed up the save situation already. In addition, that pre-start screen is not some flag saying that we can just break stuff in the code without consequences.

I am against any such improvements for any game in ScummVM, which means I would be against any such improvements to Myst/Riven. I would seriously like to see the _tweaksEnabled flag removed. I would only be for fixing bugs in the original, which is different than adding our own "improvements" on top of the game. Therefore, I would block any patch doing such an improvement to Myst/Riven.

@wjp
Owner

@bgK: do you want to keep the ability for ScummVM to load savegames from the original, the ability for the original to load savegames from ScummVM, or both? (Since they're very separate things, and the latter is already complicated by our use of compression.)

@clone2727
Collaborator

@wjp: Well, it's the same format minus the ScummVM saves being gzip'd. Since that compression cannot be stopped as long as zlib is compiled in, I don't really see why that question matters. As with the other games that share the saved game format with the original, we would have the note that the user may have to gunzip the files first.

@bgK
Collaborator

@wjp Currently both ways work, as long as you manually decompress the savefiles written by ScummVM before feeding them to the original engine. I see no reason to change that behavior since we don't plan to add new features to the game, and in that particular case, it seems more appropriate to use ScummVM's settings. Moreover the savefile format has no "magic code" in the header, nor supports versioning which makes it difficult to extend.

@clone2727
Collaborator

Closing the pull request.

@clone2727 clone2727 closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jan 16, 2012
  1. @monnerat

    MOHAWK: Various fixes around the Myst selenitic age sound receiver:

    monnerat authored
            Repeat reate
            Initial position
            Sound termination
  2. @monnerat

    MOHAWK: Myst extensions option flag.

    monnerat authored
    MOHAWK: keep telescope position while staying in the Myst stoneship age.
  3. @monnerat
  4. @monnerat
  5. @monnerat

    MOHAWK: new graphics procedure copyClippedImageToScreen().

    monnerat authored
    MOHAWK: complete rework of Myst observatory time controls:
            Handle each of the four controls in a separate object class.
            Determine date format from month display position.
            Fix sound in control preset.
            Adjust sound duration of star panel animation.
            Differentiate controls increase/decrease op-codes.
            Light-off button when not blinking.
            Allow control both by position and by function (D,M,Y,T).
  6. @monnerat
  7. @monnerat
Something went wrong with that request. Please try again.