forgottenserver icon indicating copy to clipboard operation
forgottenserver copied to clipboard

fixed loss-of-precision warnings

Open DSpeichert opened this issue 3 years ago • 3 comments

Pull Request Prelude

  • [x] I have followed [proper The Forgotten Server code styling][code].
  • [x] I have read and understood the [contribution guidelines][cont] before making this PR.
  • [x] I am aware that this PR may be closed if the above-mentioned criteria are not fulfilled.

Changes Proposed

This gets rid of all the type-change/loss-of-precision warnings by making casts explicit. It's a band-aid.

We may want to instead change the types throughout the code.

DSpeichert avatar Apr 02 '22 17:04 DSpeichert

There's a bunch of inconsistent types.

Some of which I think may be due to the engine trying to handle larger numbers while shortening it for client consumption as the protocol requires, while keeping the ability to support larger number in future or e.g. OTC. However, that is not consistent. It is also inconsistently handled, in some places, when e.g. uint32_t is turned to uint16_t to be sent over the wire, sometimes it is properly handled with static_cast<uint16_t> + std::min<uint32_t>, sometimes it's just chopped off which is not correct.

Similar with levels/magic levels, vocation IDs or item counts. Do we want to allow players to have more than the client can present? Does it make sense for containers to have "hidden" items beyond the slots that client can address?

Does it make sense for market to hold more items (count) in an offer than a client can show? Yes. But how can such an offer be created if client cannot do that?

I remember historically there were many "custom" servers where they wanted higher levels, higher experience counts, etc. I don't know if this was ever the goal natively in forgottenserver but there seems to be inconsistency as compared to what the client can do and modern client is already handling larger numbers of everything. Maybe - for consistency - we should limits our internal engine logic to what the client can address?

Additional issue is related to the special meaning of -1, e.g. for when client has a limit of e.g. uint8 for a value, yet we try to pack additional meaning (e.g. subType of -1 equals "any type"), thus making the type something larger and signed (int32_t), which makes it harder to then properly send to the client (must be cast).

DSpeichert avatar Apr 03 '22 03:04 DSpeichert

Some of which I think may be due to the engine trying to handle larger numbers while shortening it for client consumption as the protocol requires, while keeping the ability to support larger number in future or e.g. OTC.

health and mana are known to go beyond 65k, it's essential for high exp servers Knight hits 65k hp at level 4365, Mage hits 65k mana at level 2192. Currently the highest mage has level 2067 which means that we might see the change in the protocol VERY SOON. There are also several cases in which there is "add" operation performed which can cause overflow so a larger type is being used to call std::min.

However, that is not consistent. It is also inconsistently handled, in some places, when e.g. uint32_t is turned to uint16_t to be sent over the wire, sometimes it is properly handled with static_cast<uint16_t> + std::min<uint32_t>, sometimes it's just chopped off which is not correct.

Skills can only have positive values, but skill buffs can be negative - this is one of situations in which a signed integer is used.

Similar with levels/magic levels, vocation IDs or item counts. Do we want to allow players to have more than the client can present? Does it make sense for containers to have "hidden" items beyond the slots that client can address?

256 vocation ids is more than enough. Even most custom servers aren't using that much (20 vocations + fourth promotion gives 100 at best). Overflown containers are fine. If you nerf a backpack size in items.xml, the players will lose items without it.

Does it make sense for market to hold more items (count) in an offer than a client can show? Yes. But how can such an offer be created if client cannot do that?

I had never seen a scenario in which I'd have to sell 65000 items or more and if they are so easily obtainable, they're probably worth less than 1gp anyway.

I remember historically there were many "custom" servers where they wanted higher levels, higher experience counts, etc. I don't know if this was ever the goal natively in forgottenserver but there seems to be inconsistency as compared to what the client can do and modern client is already handling larger numbers of everything. Maybe - for consistency - we should limits our internal engine logic to what the client can address?

The problem with levels is tied to high exp servers only. A good workaround would be changing the way players level up at some point close to hitting level cap. The best way to achieve this would be creating an event that would return % progress for the skills window and a method to level the player up without depending on exp. I've developed an alternative progress formula that could be used to add level percentiles for killing monsters rather than exp points themselves (it's very close to expected exp for selected level and iirc doesn't depend on looping n times). It's pretty complex and depends on level to level gaps and monster base xp so it'd take another long post like this one to explain.

The problem with skills is tied to high skill rate. Ideally everyone should be able to hit 255 skill, but u64 can't allow that many tries. This would also require calculating skill % in some other way after passing certain point. Luckily we can solve this one with loyalty bonus system.

Zbizu avatar Apr 03 '22 03:04 Zbizu

needs rebase

EPuncker avatar Aug 10 '22 21:08 EPuncker