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

ZVISION: Fix initial out-of-bounds condition in clock. #2030

Merged
merged 1 commit into from Feb 2, 2020

Conversation

@DirtyHairy
Copy link
Contributor

DirtyHairy commented Feb 1, 2020

This fixes an out-of-bounds condition when the clock is queried for the first time. Without this patch, Zork Nemesis crashes on my iPad after the initial cut scene.

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 1, 2020

What kind of crash are you experiencing? Do you have a stack trace?

@criezy

This comment has been minimized.

Copy link
Member

criezy commented Feb 1, 2020

Maybe an alternative to this change would be to initialise _lastTime to _system->getMillis() in the Clock constructor? In particular since the Clock seems to be constructed as running (it initializes _paused to false), and start() does not set _lastTime when _paused is false, this would seem to make sense to me so that the first call to update() does not set a _deltaTime bigger than expected.

Note also that the iOS backend implementation of OSystem::getMillis() does not comply with the documentation (number of milliseconds since the application started) and may return a much bigger number. This may be the reason why the issue appears with this platform. I am looking at fixing that, but this does not change the fact there is something fishy with the Clock class implementation in the zvision engine.

@DirtyHairy

This comment has been minimized.

Copy link
Contributor Author

DirtyHairy commented Feb 1, 2020

What kind of crash are you experiencing? Do you have a stack trace?

I can reproduce the exact location tomorrow if you like, but essentially I get an assertion failure in Rect because Rect::isValidRect returns false. This happens because MenuNemesis::process is called with the initial (huge) value for deltaTime and uses it to calculate an invalid scroll position.

Maybe an alternative to this change would be to initialise _lastTime to _system->getMillis() in the Clock constructor?

Yeah, that would work, too. Sounds like a cleaner fix to me. If you like, I can make the change and update the PR.

Note also that the iOS backend implementation of OSystem::getMillis() does not comply with the documentation (number of milliseconds since the application started) and may return a much bigger number.

Ah 😏 I suspected as much, as the engine runs fine on other platforms.

@criezy

This comment has been minimized.

Copy link
Member

criezy commented Feb 1, 2020

I grabbed my copy of Zork Nemesis from GoG and I can confirm the crash on iOS. As I expected this is due to the _deltaTime being a lot bigger than expected due to the peculiar implementation of getMillis() for this backend. This triggers an assert in a Common::Rect created in MenuNemesis::process(uint32 deltatime) because the y1 of the rect (computed from the deltatime) his too big).

Screenshot 2020-02-01 at 22 31 34

I suspect the issue could be reproduced on other backend by keeping ScummVM a long time in the launcher before starting the game. And I still think at this point that a better fix would be to initialise _lastTime to _system->getMillis() in the Clock constructor.

@DirtyHairy DirtyHairy force-pushed the DirtyHairy:fix-zvision-out-of-bounds branch from 0fa00f8 to 2a35947 Feb 2, 2020
@DirtyHairy

This comment has been minimized.

Copy link
Contributor Author

DirtyHairy commented Feb 2, 2020

I've modified the PR to properly initialize _deltaTime on construction.

@criezy
criezy approved these changes Feb 2, 2020
Copy link
Member

criezy left a comment

Thank you. That looks good to me.

Also for information I fixed the implementation of getMillis() for the iOS7 backend yesterday (and I checked it was already correct for the iPhone backend used for older iOS devices).

@bluegr

This comment has been minimized.

Copy link
Member

bluegr commented Feb 2, 2020

Sorry for the late reply, I was busy these days.

Many thanks to @criezy for identifying and proposing a fix for the actual cause. Yes, the correct fix for this is to initialize _lastTime correctly.

This is good to be merged now, thanks @DirtyHairy for raising the issue, and providing a fix.

Merging

@bluegr bluegr merged commit fef5139 into scummvm:master Feb 2, 2020
2 checks passed
2 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.