Opened 5 years ago

Closed 5 years ago

#6663 closed defect (fixed)

NEVERHOOD: Cannon button sometimes doesn't work (uninitialized variables)

Reported by: eriktorbjorn Owned by: johndoe123
Priority: normal Component: Engine: Neverhood
Keywords: Cc:
Game: The Neverhood

Description

The cannon controls sometimes doesn't work: Nothing happens when I click on the big red button.

This appears to be because Scene3009 adds three collision sprites: SsScene3009FireCannonButton, AsScene3009VerticalIndicator and AsScene3009HorizontalIndicator. But the collision bounds for the second and third are never initialized. Sometimes, by pure chance, they happen to cover the entire screen, and apparently they have higher priority than the fire button.

I don't know why they are collision sprites. Actually, I can't even see them visually in the scene, at least not when the cannon is used early in the game. Perhaps they're only visible later? Either way, it's not obvious to me what the correct fix for this is.

For reference, here is the Valgrind log:

==10730== Conditional jump or move depends on uninitialised value(s)
==10730==    at 0x8B04FD2: Neverhood::Sprite::isPointInside(short, short) (sprite.cpp:70)
==10730==    by 0x8B01263: Neverhood::Scene::queryPositionSprite(short, short) (scene.cpp:342)
==10730==    by 0x8B00ED9: Neverhood::Scene::update() (scene.cpp:267)
==10730==    by 0x8AF3187: Neverhood::Scene3009::update() (module3000.cpp:486)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A89A74: Neverhood::Module::updateChild() (module.cpp:104)
==10730==    by 0x8AF21FF: Neverhood::Module3000::updateScene() (module3000.cpp:177)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A89A74: Neverhood::Module::updateChild() (module.cpp:104)
==10730==    by 0x8A81C5D: Neverhood::GameModule::updateModule() (gamemodule.cpp:620)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A6D3F5: Neverhood::NeverhoodEngine::mainLoop() (neverhood.cpp:196)
==10730== 
==10730== Conditional jump or move depends on uninitialised value(s)
==10730==    at 0x8B04FE2: Neverhood::Sprite::isPointInside(short, short) (sprite.cpp:70)
==10730==    by 0x8B01263: Neverhood::Scene::queryPositionSprite(short, short) (scene.cpp:342)
==10730==    by 0x8B00ED9: Neverhood::Scene::update() (scene.cpp:267)
==10730==    by 0x8AF3187: Neverhood::Scene3009::update() (module3000.cpp:486)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A89A74: Neverhood::Module::updateChild() (module.cpp:104)
==10730==    by 0x8AF21FF: Neverhood::Module3000::updateScene() (module3000.cpp:177)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A89A74: Neverhood::Module::updateChild() (module.cpp:104)
==10730==    by 0x8A81C5D: Neverhood::GameModule::updateModule() (gamemodule.cpp:620)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A6D3F5: Neverhood::NeverhoodEngine::mainLoop() (neverhood.cpp:196)
==10730== 
==10730== Conditional jump or move depends on uninitialised value(s)
==10730==    at 0x8B04FF2: Neverhood::Sprite::isPointInside(short, short) (sprite.cpp:70)
==10730==    by 0x8B01263: Neverhood::Scene::queryPositionSprite(short, short) (scene.cpp:342)
==10730==    by 0x8B00ED9: Neverhood::Scene::update() (scene.cpp:267)
==10730==    by 0x8AF3187: Neverhood::Scene3009::update() (module3000.cpp:486)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A89A74: Neverhood::Module::updateChild() (module.cpp:104)
==10730==    by 0x8AF21FF: Neverhood::Module3000::updateScene() (module3000.cpp:177)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A89A74: Neverhood::Module::updateChild() (module.cpp:104)
==10730==    by 0x8A81C5D: Neverhood::GameModule::updateModule() (gamemodule.cpp:620)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A6D3F5: Neverhood::NeverhoodEngine::mainLoop() (neverhood.cpp:196)
==10730== 
==10730== Conditional jump or move depends on uninitialised value(s)
==10730==    at 0x8B05002: Neverhood::Sprite::isPointInside(short, short) (sprite.cpp:70)
==10730==    by 0x8B01263: Neverhood::Scene::queryPositionSprite(short, short) (scene.cpp:342)
==10730==    by 0x8B00ED9: Neverhood::Scene::update() (scene.cpp:267)
==10730==    by 0x8AF3187: Neverhood::Scene3009::update() (module3000.cpp:486)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A89A74: Neverhood::Module::updateChild() (module.cpp:104)
==10730==    by 0x8AF21FF: Neverhood::Module3000::updateScene() (module3000.cpp:177)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A89A74: Neverhood::Module::updateChild() (module.cpp:104)
==10730==    by 0x8A81C5D: Neverhood::GameModule::updateModule() (gamemodule.cpp:620)
==10730==    by 0x8A7F952: Neverhood::Entity::handleUpdate() (entity.cpp:64)
==10730==    by 0x8A6D3F5: Neverhood::NeverhoodEngine::mainLoop() (neverhood.cpp:196)
==10730== 

Ticket imported from: bugs/6663.

Attachments (1)

neverhood-win.008 (12.5 KB ) - added by eriktorbjorn 5 years ago.

Download all attachments as: .zip

Change History (5)

by eriktorbjorn, 5 years ago

Attachment: neverhood-win.008 added

comment:1 by digitall, 5 years ago

Replicated here with Linux x86_64.

I think the root cause of the issue is that the Sprite class defines a member variable for the Collision Bounds, but sets no default valueas NRect has no default constructor to set it to zero... as opposed to NDrawRect which does:
https://github.com/scummvm/scummvm/blob/master/engines/neverhood/sprite.h#L94

See here and here for NRect vs. NDrawRect:
https://github.com/scummvm/scummvm/blob/master/engines/neverhood/graphics.h#L43
https://github.com/scummvm/scummvm/blob/master/engines/neverhood/graphics.h#L68

Beyond this, the values that should be set for the collision bounds for SsScene3009FireCannonButton, AsScene3009VerticalIndicator and AsScene3009HorizontalIndicator is a different issue... but this will at least ensure that they and other StaticSprite / Sprite usage does not end up with uninitialized and unstable values.

comment:2 by digitall, 5 years ago

Owner: set to johndoe123

comment:3 by johndoe123, 5 years ago

Some fields in Sprite weren't correctly inizialized, they're all set to 0 now.

comment:4 by johndoe123, 5 years ago

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.