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

Fixed variable type in npc.cpp #3996

Merged
merged 6 commits into from
Mar 11, 2022
Merged

Fixed variable type in npc.cpp #3996

merged 6 commits into from
Mar 11, 2022

Conversation

EPuncker
Copy link
Contributor

@EPuncker EPuncker commented Mar 9, 2022

Pull Request Prelude

Changes Proposed

Changed from uint to int because we pass -1 at buy/sell

Issues addressed:
Closes #3980

ranisalt
ranisalt previously approved these changes Mar 9, 2022
Copy link
Contributor

@MillhioreBT MillhioreBT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For consistency with the expected value uint32_t we can't use int32_t since we would lose half of the available numbers, so we should use int64_t in this case

@nekiro
Copy link
Member

nekiro commented Mar 10, 2022

For consistency with the expected value uint32_t we can't use int32_t since we would lose half of the available numbers, so we should use int64_t in this case

Or stop passing -1

@ramon-bernardo
Copy link
Contributor

🤔

enum.h
struct ShopInfo {
	...
	uint32_t buyPrice = 0;
	uint32_t sellPrice = 0;
	...
};

src/protocolgame.cpp Outdated Show resolved Hide resolved
@EPuncker EPuncker requested a review from ranisalt March 11, 2022 16:09
Copy link
Member

@ranisalt ranisalt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@EPuncker EPuncker merged commit fbe85a1 into otland:master Mar 11, 2022
@EPuncker EPuncker deleted the npc-fix branch March 11, 2022 17:50
@ramon-bernardo
Copy link
Contributor

ramon-bernardo commented Mar 11, 2022

Shouldn't we use price > 0?

bool Player::updateSaleShopList() {
    ...
    auto it = std::find_if(shopItemList.begin(), shopItemList.end(), [itemId](const ShopInfo& shopInfo) {
        return shopInfo.itemId == itemId && shopInfo.sellPrice != 0;
    });
    ...
}
bool Player::hasShopItemForSale() {
    ...
    return shopInfo.itemId == itemId && shopInfo.buyPrice != 0 && (!itemType.isFluidContainer() || shopInfo.subType == subType);
    ...
}

and price < 1 or price <= 0

void ProtocolGame::sendSaleItemList() {
    ...
    if (shopInfo.sellPrice == 0) { ... }
    ...
    if (shopInfo.sellPrice == 0) { ... }
    ...
}

msg.add<uint32_t>(item.buyPrice == std::numeric_limits<uint32_t>::max() ? 0 : item.buyPrice);
msg.add<uint32_t>(item.sellPrice == std::numeric_limits<uint32_t>::max() ? 0 : item.sellPrice);
msg.add<uint32_t>(std::max<uint32_t>(item.buyPrice, 0));
msg.add<uint32_t>(std::max<uint32_t>(item.sellPrice, 0));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work as you think it does. This first converts both values to the template type (uint32_t). Since the buyPrice/sellPrice can be -1, this will just overflow and return that value instead of 0.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

@MillhioreBT MillhioreBT Jul 19, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not work as you think it does. This first converts both values to the template type (uint32_t). Since the buyPrice/sellPrice can be -1, this will just overflow and return that value instead of 0.
GIF 19-07-2022 02-47-55 p  m

Confirmed!
I guess a more explicit check should be done since buyPrice and sellPrice are of type int64_t

A possible solution:

uint32_t value = std::max<int64_t>(std::min<int64_t>(number, std::numeric_limits<uint32_t>::max()), 0);

Codinablack pushed a commit to Codinablack/forgottenserver that referenced this pull request Apr 5, 2022
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.

OnCreatureSay out of range value error
6 participants