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

ASYLUM: DO NOT MERGE: Fix crash when cursor is on top of menu screen #6383

Closed

Conversation

antoniou79
Copy link
Contributor

@antoniou79 antoniou79 commented Jan 4, 2025

This crash happens in a release build (--enable-release) with --disable-debug on msys2/mingw64

The crash seems to be caused by an overflow for int32 when calculating the index value in the getAngle() method, and diffY value is equal or greater than 0x800000 (ie. 2^23)

The Actor::getAngle() method in the context of the in-game menu (called from Menu::update()) is used for calculating the correct "frame" for the eyes in the center of the screen which are supposed to follow the cursor movement. The getAngle() method is also used elsewhere in the engine (called from Actor::faceTarget()), so this fix should be tested so that it has no side-effects in that part of the code.

This is a potential fix for this bug ticket: https://bugs.scummvm.org/ticket/15658

Edit: Please do not merge. Currently exploring an alternative fix that will also bring behavior closer to the original

This crash happens in a release build (--enable-release) with --disable-debug on msys2/mingw64

The crash seems to be caused by an overflow for int32 when calculating the index value in the getAngle() method, and diffY value is equal or greater than 0x800000 (ie. 2^23)
@eriktorbjorn
Copy link
Member

Good detective work! But maybe we could also fix it by making the calculation unsigned to begin with? Something like this, though I have only given it a quick test:

diff --git a/engines/asylum/resources/actor.cpp b/engines/asylum/resources/actor.cpp
index 77cbba1c675..49a3b13adc7 100644
--- a/engines/asylum/resources/actor.cpp
+++ b/engines/asylum/resources/actor.cpp
@@ -3640,18 +3640,21 @@ void Actor::setVolume() {
 //////////////////////////////////////////////////////////////////////////
 
 ActorDirection Actor::getAngle(const Common::Point &vec1, const Common::Point &vec2) {
-	int32 diffX = (vec2.x << 16) - (vec1.x << 16);
-	int32 diffY = (vec1.y << 16) - (vec2.y << 16);
-	int32 adjust = 0;
+	uint32 diffX, diffY;
+	byte adjust = 0;
 
-	if (diffX < 0) {
-		adjust = 2;
-		diffX = -diffX;
+	if (vec1.x > vec2.x) {
+		adjust |= 2;
+		diffX = (vec1.x - vec2.x) << 16;
+	} else {
+		diffX = (vec2.x - vec1.x) << 16;
 	}
 
-	if (diffY < 0) {
+	if (vec2.y > vec1.y) {
 		adjust |= 1;
-		diffY = -diffY;
+		diffY = (vec2.y - vec1.y) << 16;
+	} else {
+		diffY = (vec1.y - vec2.y) << 16;
 	}
 
 	int32 dirAngle = -1;

@antoniou79 antoniou79 changed the title ASYLUM: Fix crash when cursor is on top of menu screen ASYLUM: DO NOT MERGE: Fix crash when cursor is on top of menu screen Jan 4, 2025
antoniou79 added a commit to antoniou79/scummvm that referenced this pull request Jan 4, 2025
And also clean up code and fix eyes movement to match original

This fix was proposed by eriktorbjorn in a previous PR scummvm#6383 (now closed in favor of this one).

This prevents an overflow for int32 in Actor::getAngle() for the calculation of the "index" variable value, which would happen (before) when diffY would be >= 0x800000 (ie. 2^23) and was multiplied by 256 (ie. 2^8). This, in the context of the game's main menu would happen when moving the cursor to the top part of the screen.

The code in getAngle() is also cleaned up to be more readable.

An additional fix for the values of angleTable03[] is included, since previously it was populated by wrong hex values. What seems to have happened here was that the decimal values were wrongly entered as hex values by putting the "0x" prefix on the literal.

This is a potential fix for bug ticket: https://bugs.scummvm.org/ticket/15658
@antoniou79
Copy link
Contributor Author

closing in favor of #6385

@antoniou79 antoniou79 closed this Jan 4, 2025
antoniou79 added a commit that referenced this pull request Jan 8, 2025
And also clean up code and fix eyes movement to match original

This fix was proposed by eriktorbjorn in a previous PR #6383 (now closed in favor of this one).

This prevents an overflow for int32 in Actor::getAngle() for the calculation of the "index" variable value, which would happen (before) when diffY would be >= 0x800000 (ie. 2^23) and was multiplied by 256 (ie. 2^8). This, in the context of the game's main menu would happen when moving the cursor to the top part of the screen.

The code in getAngle() is also cleaned up to be more readable.

An additional fix for the values of angleTable03[] is included, since previously it was populated by wrong hex values. What seems to have happened here was that the decimal values were wrongly entered as hex values by putting the "0x" prefix on the literal.

This is a potential fix for bug ticket: https://bugs.scummvm.org/ticket/15658
antoniou79 added a commit that referenced this pull request Jan 9, 2025
And also clean up code and fix eyes movement to match original

This fix was proposed by eriktorbjorn in a previous PR #6383 (now closed in favor of this one).

This prevents an overflow for int32 in Actor::getAngle() for the calculation of the "index" variable value, which would happen (before) when diffY would be >= 0x800000 (ie. 2^23) and was multiplied by 256 (ie. 2^8). This, in the context of the game's main menu would happen when moving the cursor to the top part of the screen.

The code in getAngle() is also cleaned up to be more readable.

An additional fix for the values of angleTable03[] is included, since previously it was populated by wrong hex values. What seems to have happened here was that the decimal values were wrongly entered as hex values by putting the "0x" prefix on the literal.

This is a potential fix for bug ticket: https://bugs.scummvm.org/ticket/15658
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants