fheroes2 icon indicating copy to clipboard operation
fheroes2 copied to clipboard

Fix a crash while accessing invalid object part for Whirlpool object

Open ihhub opened this issue 1 year ago • 6 comments

  • update object type after a hero boards a boat and removes the boat sprite from a tile (close #9141)
  • fix a crash while a hero tries to disembark from a whirlpool (close #9214)

ihhub avatar Oct 19 '24 07:10 ihhub

Hi @zenseii and @oleg-derevenetz , could you please check this pull request whether it fixes the mentioned issue?

ihhub avatar Oct 19 '24 07:10 ihhub

Hi @zenseii and @oleg-derevenetz , could you please check this pull request whether it fixes the mentioned issue?

Please hold on with the review. I found much bigger problem with the code.

ihhub avatar Oct 19 '24 08:10 ihhub

I've tested it on several maps and it works well.

I did notice in the save provided by LiquidYetti that the previously broken whirlpool tile will now contain an ICN object type as unknown. I didn't notice this happening in my fheroes2 map:

19.10.2024 13:07:28: [DBG_DEVEL]        Interface::AdventureMap::mouseCursorAreaPressRight:
******* Tile info *******
Tile index      : 7096, point: (76, 65)
MP2 object type : 167 (Whirlpool)
UID             : 0
ICN object type : 0 (UNKNOWN)
image index     : 255
layer type      : 0 - Object layer
is shadow       : no
region          : 36
ground          : Ocean (isRoad: 0)
ground img index: 13, image flags: 4
passable from   : center, top, top-right, right, bottom-right, bottom, bottom-left, left, top-left
metadata value 1: 0
metadata value 2: 0
metadata value 3: 0
--------- Level 1 --------
UID             : 62
ICN object type : 50 (OBJNWATR.ICN)
image index     : 202
layer type      : 1 - Background layer
is shadow       : no
--- Extra information ---

If this is expected then I'm ready to approve the PR.

Hi @zenseii , this is a leftover from the previous logic. A hero jumped into a boat before making this save file. If it is done with the proposed logic the main object remains whirlpool. However, the engine works even with this case as I added required changes for it.

ihhub avatar Oct 19 '24 11:10 ihhub

Hi, @zenseii and @ihhub, it happens because a Summon Boat spell was used by the Purple AI player. And its behavior was not changed in this PR. Technically it is OK - OBJNWATR.ICN object is now in the bottom layer addons, but yes - it looks a little weird that it is not moved back to the main addon. :) We can update this logic in a new PR.

Districh-ru avatar Oct 19 '24 11:10 Districh-ru

Hi, @ihhub An assertion occurred, when I summon a boat standing on a lighthouse and board in it. Assertion.zip

Summon a boat with Astra and step on it. image

Branikolog avatar Oct 19 '24 12:10 Branikolog

Hi, @ihhub An assertion occurred, when I summon a boat standing on a lighthouse and board in it.

@ihhub, the AddonsSort() method moves the MP2::OBJ_ICN_TYPE_FLAG32 to the _mainAddon which is probably not good. We may change the behavior of AddonsSort() not to move MP2::OBJ_NONE objects to the _mainAddon by inserting between the 884 and 885 lines the next code:

        if ( getObjectTypeByIcn( highestPriorityAddon._objectIcnType, highestPriorityAddon._imageIndex ) == MP2::OBJ_NONE ) {
            return;
        }

Or we should only don't move only highestPriorityAddon._objectIcnType == MP2::OBJ_ICN_TYPE_FLAG32? What do you think?

Districh-ru avatar Oct 19 '24 14:10 Districh-ru

Hi @Districh-ru , I removed object part sorting call for now as I don't think a solution would be that simple.

ihhub avatar Oct 19 '24 15:10 ihhub

Hi @Branikolog , I fixed the assertion. Could you please take a look again?

ihhub avatar Oct 19 '24 15:10 ihhub

Hi @oleg-derevenetz and @Districh-ru , I updated the description of this pull request to explain in details the problems and how they were fixed. I hope now it clarifies many thing. Please let me know if something is not clear.

ihhub avatar Oct 20 '24 07:10 ihhub