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

SCI: Script patch for LB2 crate room bug #10701 #1329

Merged
merged 1 commit into from Sep 16, 2018

Conversation

Projects
None yet
4 participants
@sluicebox
Contributor

sluicebox commented Sep 11, 2018

Fixes a lockup in the original game

@dafioram

This comment has been minimized.

Show comment
Hide comment
@dafioram

dafioram Sep 11, 2018

Member

Nice work.

I've tested that this resolves the bug.

Can you update your comments so it includes the acts that this occurs in (Act 5?). Can you brake up your two paragraphs into more so that it is easier to read. Like after the first sentence, Can you add which rooms this concerns in the comments?

How do you know that this also happens in the original? Viewing the scripts/disassembly or actually playing the game (dosbox)?

Member

dafioram commented Sep 11, 2018

Nice work.

I've tested that this resolves the bug.

Can you update your comments so it includes the acts that this occurs in (Act 5?). Can you brake up your two paragraphs into more so that it is easier to read. Like after the first sentence, Can you add which rooms this concerns in the comments?

How do you know that this also happens in the original? Viewing the scripts/disassembly or actually playing the game (dosbox)?

@sluicebox

This comment has been minimized.

Show comment
Hide comment
@sluicebox

sluicebox Sep 11, 2018

Contributor

I've updated the comments. Yes, I confirmed the bug in the original in dosbox.

Contributor

sluicebox commented Sep 11, 2018

I've updated the comments. Yes, I confirmed the bug in the original in dosbox.

@dafioram

This comment has been minimized.

Show comment
Hide comment
@dafioram

dafioram Sep 12, 2018

Member

Can you combine your two commits into one (Using rebase interactive or amend)? You will have to force push, but that is fine.

Member

dafioram commented Sep 12, 2018

Can you combine your two commits into one (Using rebase interactive or amend)? You will have to force push, but that is fine.

SCI: Script patch for LB2 crate room bug #10701
Fixes a lockup in the original game
@digitall

This comment has been minimized.

Show comment
Hide comment
@digitall

digitall Sep 14, 2018

Member

@dafioram : This looks good now. OK to merge?

Member

digitall commented Sep 14, 2018

@dafioram : This looks good now. OK to merge?

@dafioram

This comment has been minimized.

Show comment
Hide comment
@dafioram

dafioram Sep 14, 2018

Member

I think it is fine. it might be good to let some time pass in case @bluegr wants a chance to look at this.

Member

dafioram commented Sep 14, 2018

I think it is fine. it might be good to let some time pass in case @bluegr wants a chance to look at this.

@digitall

This comment has been minimized.

Show comment
Hide comment
@digitall

digitall Sep 14, 2018

Member

@dafioram : OK Thanks. Better to do an explicit request for review from him though, rather than just hope he sees this. Will do that. If he is happy / indifferent, we can then merge.

Member

digitall commented Sep 14, 2018

@dafioram : OK Thanks. Better to do an explicit request for review from him though, rather than just hope he sees this. Will do that. If he is happy / indifferent, we can then merge.

@digitall digitall requested a review from bluegr Sep 14, 2018

@bluegr

This comment has been minimized.

Show comment
Hide comment
@bluegr

bluegr Sep 16, 2018

Member

Nice work on the patch!

I really liked the way you used the eq operator here - basically, you add its result (0 or 1).

I reviewed your patch, and tested your changes. It all works fine. Well done, merging

Member

bluegr commented Sep 16, 2018

Nice work on the patch!

I really liked the way you used the eq operator here - basically, you add its result (0 or 1).

I reviewed your patch, and tested your changes. It all works fine. Well done, merging

@bluegr bluegr merged commit 2373fd8 into scummvm:master Sep 16, 2018

1 check passed

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