Improving Xiaolin Wu path finding w/ change factor

[tornadia]

Pull Request Prelude

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

Changes Proposed

Improves upon the path finding algorithm used by Zbizu in PR#3487, i.e. commit 7aa016fdf60bbd7916199d8674872e3a8ad05e18.

Issues addressed:
Initially mwalls positioning, but also throw items sight clear. For reference:

Broken:
https://github.com/otland/forgottenserver/assets/32970897/86ad27c6-d727-4284-9e85-58d65cbbc15e

Fix:
https://github.com/otland/forgottenserver/assets/32970897/029aa5d8-e090-407c-b7cc-51f10e1e17fe
https://github.com/otland/forgottenserver/assets/32970897/eb461fd9-c3fd-4ec5-ab56-011a852cceff

[tornadia]

credits to dc: hide_zadymiarz for finding this bug (Hide)

[tornadia]

@ranisalt @nekiro @MillhioreBT review pl0x

[Zbizu]

not a bug

[tornadia]

not a bug

Did you try that in RL or your own server? It makes more sense that the range of throwable events matches a more cone-shaped one... for reference:
https://i.imgur.com/KJgCbub.mp4

Edit: please also note that pre-1.4.2 this was the default behavior. It was working as displayed in OP (post-fix) meaning you could throw mwalls neighboring the initial one in both axis.

[tornadia]

Also see:

... I am pretty sure this is more accurate to the original formula, specially considering there is no roof above the mwalls

edit: seems like Zbizu is actually right...

Up to the community if we want this 'fix' or not. Thx guys

[ranisalt]

This is a known >bug< in Tibia for ages. I very recently wrote a lengthy response on Reddit to someone who noticed the same in Cipsoft Tibia.

We tried to fix this in the past, but people grew so used to it that they do not want it to be fixed.

[EPuncker]

This is a known >bug< in Tibia for ages. I very recently wrote a lengthy response on Reddit to someone who noticed the same in Cipsoft Tibia.

We tried to fix this in the past, but people grew so used to it that they do not want it to be fixed.

perhaps we could add it with a config.lua option with default value the "buggy" behavior?

[Zbizu]

It's a matter of preference which behaviour of mw you want in your server. Wu algo is 1:1. Old tfs also is known from 8.6 servers and there are also 8.0 players who preferred old asymmetric sightline (same as old tfs but one ray only) for pvp purposes.

@tornadia test was taken on Bona, Venore depot, east exit.

[tornadia]

Interesting opinions in this thread.

perhaps we could add it with a config.lua option with default value the "buggy" behavior?

I agree with @EPuncker ''s post. I will add it to the PR, in case it should be reviewed and merged. I believe it's a good idea to keep cross compatibility with both preferences, and more appealing considering our userbase. If they (the unmentionable ones) do not want to fix broken functionality, good. And, good for them. But we should allow this software's users to decide if they want to make the same foolish choices keep the same modus operandi.

And thanks @Zbizu and @ranisalt for your thorough research.

[ranisalt]

I would rather have a compile-time switch, to avoid paying the cost in runtime. It's not like people will change this a lot. Wdyt @EPuncker

[EPuncker]

I would rather have a compile-time switch, to avoid paying the cost in runtime. It's not like people will change this a lot. Wdyt @EPuncker

sounds better

[DSpeichert]

I would rather have a compile-time switch, to avoid paying the cost in runtime. It's not like people will change this a lot. Wdyt @EPuncker

That's not very friendly for binary distributions :/

[MillhioreBT]

I would rather have a compile-time switch, to avoid paying the cost in runtime. It's not like people will change this a lot. Wdyt @EPuncker

I agree @ranisalt

those little checks will add an extra delay to everything related to line of sight, especially on expansive spells.

Plus it looks ugly.
I think it's time to get used to how Tibia RL is.

This seems more like a change for: Nekiro Downgrades

[tornadia]

I would rather have a compile-time switch, to avoid paying the cost in runtime. It's not like people will change this a lot. Wdyt @EPuncker

That's not very friendly for binary distributions :/

How so? Just need to provide 1 binary distro with the default, being zbizu's original.
I will add the changes soon. I do not agree with the comment above, although I do agree in it's present state the code is not optimal. Compile time variables will fix this.

[MillhioreBT]

In general, I don't like this PR.
and not because of the behavior of the old tibia vs the new one.
if not rather those checks within the line of sight calculations.

[tornadia]

In general, I don't like this PR.

Hey, I guess if you don't like the PR then we can just close it and forget everything. Because that's how things work right?
edit: Sorry, that was uncalled for

and not because of the behavior of the old tibia vs the new one.

Uh-huh. It's not old XXX vs new XXX, it's how TFS used to work and how it works now, more accurately depicting the gameplay sought after by the 1:1 experience playerbase. It was fixed by zbizu and now it's being re-added for people who will it or not.

if not rather those checks within the line of sight calculations.

As I mentioned before, with compile time variables it will not be an issue. I will address your reviews now, thanks for finding those issues and bringing them forth.

[tornadia]

So, we have some decision making to do. Do you guys folks want to:
a) Include precompiler variables from some *.cmake file?

b) Add it as a CMake parameter and visual studio?

c) Hard-code it within the map.cpp file and let ppl decide in-code in true ad hoc fashion?

// COMMENT THE LINE BELOW IF YOU WANT THE CLASSIC PATHFIND
#define _USE_TFS_PATHFINDING

#ifdef _USE_TFS_PATHFINDING
bool Map::checkSteepLine(uint16_t x0, uint16_t y0, uint16_t x1, uint16_t y1, uint8_t z) const
{
// code #1
}
#else
bool Map::checkSteepLine(uint16_t x0, uint16_t y0, uint16_t x1, uint16_t y1, uint8_t z) const
{
// code #2
}
#endif

Let me know which one you people prefer. In any case, we can drop the PR. I don't really care.

[ranisalt]

@tornadia definitely B

[Zbizu]

@tornadia there is no need to adjust cmake settings, this can be put in definitions.h
as for binary distributions concern, 1:1 algo can be kept as default and if someone wants to modify it, an option in definitions could be uncommented

[tornadia]

@Zbizu ok I will see if I add that.
@MillhioreBT sorry for my previous post, that was uncalled for

[felipepips]

AOE spells are passing through the walls with this changes..

Original:

Modified:

original2:
https://jmp.sh/s/RVzafFCxBNfzXGseBGB6
modified2:
https://jmp.sh/s/erBFTo6fVdJrheBdROEX