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

[CPP] Fix non-equipment items can cause map crash #5711

Conversation

ampitere
Copy link
Contributor

@ampitere ampitere commented May 13, 2024

I affirm:

  • I understand that if I do not agree to the following points by completing the checkboxes my PR will be ignored.
  • I understand I should leave resolving conversations to the LandSandBoat team so that reviewers won't miss what was said.
  • I have read and understood the Contributing Guide and the Code of Conduct.
  • I have tested my code and the things my code has changed since the last commit in the PR and will test after any later commits.

What does this pull request do?

Currently since CLuaBaseEntity::canEquipItem only takes an item ID as a param then casts it, it will sometimes be a non-equipment item. With the addition of isEquippableByRace it attempts to check a modifier that doesn't exist on non-equipment items. To fix this we'll add a return false if the item is not equipment. Resolves #5703

Steps to test these changes

  1. Go to a conquest NPC
  2. Keep opening the "Are you sure you want to purchase the X?" menu on a non-equipment item (Scroll of Instant Warp for example)

@WinterSolstice8
Copy link
Member

Just to be sure -- you also checked ranged weapons, ammo, shields, things that aren't weapons but are equippable in ammo, etc?

I'm sure it works based on the set type/get type functions

void CItem::setType(uint8 type)
{
    m_type |= type;
}

bool CItem::isType(ITEM_TYPE type) const
{
    return (m_type & type);
}

all of those things should be under "CItemEquipment" but this GetType junk is a bit unknown to me

@ampitere
Copy link
Contributor Author

Just to be sure -- you also checked ranged weapons, ammo, shields, things that aren't weapons but are equippable in ammo, etc?

I'm sure it works based on the set type/get type functions

void CItem::setType(uint8 type)
{
    m_type |= type;
}

bool CItem::isType(ITEM_TYPE type) const
{
    return (m_type & type);
}

all of those things should be under "CItemEquipment" but this GetType junk is a bit unknown to me

The cast to CItemEquipment changes the type to EQUIPMENT so if it's not that type then it failed the cast and still has it's previous type as far as I can tell. Shields, ammo, fishing rods, bait, lures, guns, bows, etc. are all in item_equipment.sql so those all should properly cast to equipment.

@claywar claywar merged commit 8d19b12 into LandSandBoat:base May 14, 2024
12 checks passed
@ampitere ampitere deleted the fix_non_equipment_items_can_cause_map_crash branch May 14, 2024 19:05
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.

🐛 map server crash while calling canEquipItem()
4 participants