{"action":"created","issue":{"url":"https://api.github.com/repos/scummvm/scummvm/issues/1416","repository_url":"https://api.github.com/repos/scummvm/scummvm","labels_url":"https://api.github.com/repos/scummvm/scummvm/issues/1416/labels{/name}","comments_url":"https://api.github.com/repos/scummvm/scummvm/issues/1416/comments","events_url":"https://api.github.com/repos/scummvm/scummvm/issues/1416/events","html_url":"https://github.com/scummvm/scummvm/pull/1416","id":383956268,"node_id":"MDExOlB1bGxSZXF1ZXN0MjMzMjk5MDkx","number":1416,"title":"SCI32: Fix QFG4 conditional void calls","user":{"login":"Vhati","id":2262621,"node_id":"MDQ6VXNlcjIyNjI2MjE=","avatar_url":"https://avatars1.githubusercontent.com/u/2262621?v=4","gravatar_id":"","url":"https://api.github.com/users/Vhati","html_url":"https://github.com/Vhati","followers_url":"https://api.github.com/users/Vhati/followers","following_url":"https://api.github.com/users/Vhati/following{/other_user}","gists_url":"https://api.github.com/users/Vhati/gists{/gist_id}","starred_url":"https://api.github.com/users/Vhati/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Vhati/subscriptions","organizations_url":"https://api.github.com/users/Vhati/orgs","repos_url":"https://api.github.com/users/Vhati/repos","events_url":"https://api.github.com/users/Vhati/events{/privacy}","received_events_url":"https://api.github.com/users/Vhati/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":3,"created_at":"2018-11-24T03:48:34Z","updated_at":"2018-11-25T18:36:22Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/scummvm/scummvm/pulls/1416","html_url":"https://github.com/scummvm/scummvm/pull/1416","diff_url":"https://github.com/scummvm/scummvm/pull/1416.diff","patch_url":"https://github.com/scummvm/scummvm/pull/1416.patch"},"body":"Fixes arithmetic hacks in combat, bugs [#10138](https://bugs.scummvm.org/ticket/10138), [#10419](https://bugs.scummvm.org/ticket/10419), [#10710](https://bugs.scummvm.org/ticket/10710), [#10814](https://bugs.scummvm.org/ticket/10814)\r\n\r\nSupersedes commits 4dc9f0e and 01f3e6c\r\n\r\n\t// In QFG4, the kernel func SetNowSeen() returns void - meaning it doesn't\r\n\t// modify the accumulator to store a result. Yet math was performed on it!\r\n\t//\r\n\t// The function updates boundary box properties of a given View object, prior\r\n\t// to collision tests. IF (collision detection is warranted, and IF those tests\r\n\t// are true), THEN respond to the collision.\r\n\t//\r\n\t// SetNowSeen() was inserted in the middle of the IF block's list of conditions.\r\n\t// That way it can be short-circuited, and it runs before the tests.\r\n\t//\r\n\t// Problem: void functions make no promise about their truth value. After the\r\n\t// call, acc will be *whatever* happened to already be in there. This is bad.\r\n\t//\r\n\t// \"(| (SetNowSeen horror) $0001)\"\r\n\t//\r\n\t// Original Solution: make whatever it is non-zero. Do a bitwise OR against 1.\r\n\t// The OR wrapper fakes a true return value, so the IF block won't break.\r\n\t//\r\n\t// Someone exploited an unsafe compiler with a clever hack. It even worked when\r\n\t// an object snuck into the accumulator. ScummVM enforces safe arithmetic. Now\r\n\t// the code cleverly crashes - unless a number accidentally leaks into acc.\r\n\t//\r\n\t// \"Invalid arithmetic operation (bitwise OR - params: 002e:1694 and 0000:0001)\"\r\n\t//\r\n\t// We leave the OR wrapper. When the call returns, we manually feed the OR a\r\n\t// literal 1. Same effect. The IF block goes on evaluating conditions.\r\n\r\nDiscovery process is documented on the ticket for [#10814](https://bugs.scummvm.org/ticket/10814).\r\nThe hack was used exclusively in combat code.\r\nSavegames for each monster are attached to the ticket."},"comment":{"url":"https://api.github.com/repos/scummvm/scummvm/issues/comments/441462039","html_url":"https://github.com/scummvm/scummvm/pull/1416#issuecomment-441462039","issue_url":"https://api.github.com/repos/scummvm/scummvm/issues/1416","id":441462039,"node_id":"MDEyOklzc3VlQ29tbWVudDQ0MTQ2MjAzOQ==","user":{"login":"Vhati","id":2262621,"node_id":"MDQ6VXNlcjIyNjI2MjE=","avatar_url":"https://avatars1.githubusercontent.com/u/2262621?v=4","gravatar_id":"","url":"https://api.github.com/users/Vhati","html_url":"https://github.com/Vhati","followers_url":"https://api.github.com/users/Vhati/followers","following_url":"https://api.github.com/users/Vhati/following{/other_user}","gists_url":"https://api.github.com/users/Vhati/gists{/gist_id}","starred_url":"https://api.github.com/users/Vhati/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/Vhati/subscriptions","organizations_url":"https://api.github.com/users/Vhati/orgs","repos_url":"https://api.github.com/users/Vhati/repos","events_url":"https://api.github.com/users/Vhati/events{/privacy}","received_events_url":"https://api.github.com/users/Vhati/received_events","type":"User","site_admin":false},"created_at":"2018-11-25T18:36:22Z","updated_at":"2018-11-25T18:36:22Z","author_association":"CONTRIBUTOR","body":"@bluegr:\r\n> There is a variant of kSetNowSeen, which returns a boolean, which was used in newer SCI versions. Seems like the intended behavior in these scripts is to handle a boolean.\r\n\r\nMy speculation is that was still void at the time, and needing to hack it factored into their decision to make it boolean later.\r\n\r\n> the cleanest solution would be to use this variant of kSetNowSeen in QFG4\r\n\r\nProbably.\r\n\r\nWhere not used as a conditional, the return value would be irrelevant. Grepping ALL occurrences of SetNowSeen... they never use it in a fashion that would store a result from it.\r\n\r\nWhere used as a conditional, the OR wrappers would never let it abandon the block. Relevant only insofar as it'd be a number.\r\n\r\nSo the end effect of booleanizing the func would be the same as this patch. False returns won't matter, but they'll still be numbers."}}
{"actor":{"display_login":"Vhati"}}