chore(Map) Add open world LOS settings

[pangolp]

Changes Proposed:

This PR proposes changes to:

• [x] Core (units, players, creatures, game systems).

Issues Addressed:

• Closes https://github.com/andeswow/bugtracker/issues/3

Tests Performed:

This PR has been:

• [x] Tested in-game by the author.

How to Test the Changes:

1. Enter with a character in the open world
2. By default the configuration variable will be disabled, so you can traverse mountains, trees and possibly some other structure.
3. By enabling the config variable and reloading the server configuration .reload config, you will notice that it is no longer possible to shoot, heal or perform any type of action, behind a mountain, tree or other structure in the open world.
4. Since this configuration is not blizzlike, by default it would be disabled, and it will depend on each community whether it is enabled or not. Many communities have it, and many communities ask for it. So it would be good to be able to add it.

For default (disabled)

https://github.com/azerothcore/azerothcore-wotlk/assets/2810187/1c810dad-ed2f-4633-8788-4697c303578f

Enabled

https://github.com/azerothcore/azerothcore-wotlk/assets/2810187/d7386c28-61cf-4377-803d-edd614c5c37c

Known Issues and TODO List:

How to Test AzerothCore PRs

When a PR is ready to be tested, it will be marked as [WAITING TO BE TESTED].

You can help by testing PRs and writing your feedback here on the PR's page on GitHub. Follow the instructions here:

http://www.azerothcore.org/wiki/How-to-test-a-PR

REMEMBER: when testing a PR that changes something generic (i.e. a part of code that handles more than one specific thing), the tester should not only check that the PR does its job (e.g. fixing spell XXX) but especially check that the PR does not cause any regression (i.e. introducing new bugs).

For example: if a PR fixes spell X by changing a part of code that handles spells X, Y, and Z, we should not only test X, but we should test Y and Z as well.

[sudlud]

Thanks for the PR!

I feel like this should maybe be somehow combined with the "blizzlike_pvp_los" setting that is right above the new setting?

Or they should feature similar names like

Blizzlike_pvp_los_arena_battlefield And Blizzlike_pvp_los_open_world

As they have the same effect just in different areas of the game.

What do you think?

[pangolp]

Thanks for the PR!

I feel like this should maybe be somehow combined with the "blizzlike_pvp_los" setting that is right above the new setting?

Or they should feature similar names like

Blizzlike_pvp_los_arena_battlefield And Blizzlike_pvp_los_open_world

As they have the same effect just in different areas of the game.

What do you think?

Could be. I didn't want to combine it, because it requires changing the name, since the one you mention says blizzlike, and so, I preferred to directly create a new and independent configuration variable. But I am open to the changes I have to make. The way it is proposed, although it may seem redundant, is totally independent from the rest of the options and it is clear that it is a mechanic, that it is not blizzlike, that by default, it will be disabled, and that it is left to discretion of each server, whether to use it or not. If something needs to be changed, I am willing to improve it.

[pangolp]

It is likely that it can be combined. However, the name above would have to be changed. And it should possibly be enabled by default. Because I don't remember if it is enabled or not.

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS))
{
    if (IsBattlegroundOrArena())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

if (sWorld->getBoolConfig(CONFIG_VMAP_PVP_LOS_IN_OPEN_WORLD))
{
    if (IsWorldMap())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

[sudlud]

I thought about something like this:

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS))
{
    if (IsBattlegroundOrArena())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD))
{
    if (IsWorldMap())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

so the setting would be inverted and "active" by default and have a similar name like the existing CONFIG_VMAP_BLIZZLIKE_PVP_LOS?

[pangolp]

I don't know if it would be possible to combine it through an OR perhaps. I'm throwing out ideas, without really trying them.

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS) || sWorld->getBoolConfig(CONFIG_VMAP_PVP_LOS_IN_OPEN_WORLD))
{
    if (IsBattlegroundOrArena() || IsWorldMap())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

Do you say something like this?

[pangolp]

I thought about something like this:

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS))
{
    if (IsBattlegroundOrArena())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD))
{
    if (IsWorldMap())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

so the setting would be inverted and "active" by default and have a similar name like the existing CONFIG_VMAP_BLIZZLIKE_PVP_LOS?

At the time when we discussed it, and they presented the idea to me, they told me that it was not something blizzlike. That we could add it as a configuration variable and that's why I did it this way. However, if I have to change it, I do. But I would like it to be something independent, because perhaps someone wants to enable the blizzlike part, but not the open world part.

[pangolp]

I uploaded 2 videos, but they are no longer displayed for some reason that I don't know. Before they had a preview. They are anchored in the description of the pull request. In one of them, I show what it would be like with the default option, and in the other, what it would be like when enabling the option.

[Nyeriah]

I don`t like the name used in this config. I feel like it describes what it does poorly

[pangolp]

Maybe you are right, and it is possible to combine them within the same if. But I would have to do tests. And you would have to review the default configuration. Because I would have to make it disabled by default. Because it's not a blizzlike mechanic technically.

[pangolp]

I don`t like the name used in this config. I feel like it describes what it does poorly

I accept suggestions, and I will gladly change them.

[sudlud]

I don`t like the name used in this config. I feel like it describes what it does poorly

as mentioned above, my suggestion would be CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD similiar to the existing CONFIG_VMAP_BLIZZLIKE_PVP_LOS for arena / battleground.

[sudlud]

At the time when we discussed it, and they presented the idea to me, they told me that it was not something blizzlike. That we could add it as a configuration variable and that's why I did it this way. However, if I have to change it, I do. But I would like it to be something independent, because perhaps someone wants to enable the blizzlike part, but not the open world part.

Yes which is why the setting would then be inverse.

CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD = false would enable the LOS check for open world.

[pangolp]

I don`t like the name used in this config. I feel like it describes what it does poorly

as mentioned above, my suggestion would be CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD similiar to the existing CONFIG_VMAP_BLIZZLIKE_PVP_LOS for arena / battleground.

It's just that it's not blizzlike, that's why I didn't put that in it.

[pangolp]

At the time when we discussed it, and they presented the idea to me, they told me that it was not something blizzlike. That we could add it as a configuration variable and that's why I did it this way. However, if I have to change it, I do. But I would like it to be something independent, because perhaps someone wants to enable the blizzlike part, but not the open world part.

Yes which is why the setting would then be inverse.

CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD = false would enable the LOS check for open world.

If it can be combined, and you can make them somehow independent, and you have to combine them, I combine them, there is no problem.

[sudlud]

Merge state

Add more commits by pushing to the [los](/pangolp/azerothcore-wotlk/tree/los) branch on pangolp/azerothcore-wotlk.

It's just that it's not blizzlike, that's why I didn't put that in it.

please take a look at the existing vmap.BlizzlikePvPLOS setting.

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS))
{
    if (IsBattlegroundOrArena())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

if vmap.BlizzlikePvPLOS = true the ìf` block will NOT be executed.

Same would happen here:

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD))
{
    if (IsWorldMap())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

if CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD = true, the ìf` block is NOT executed. so blizzlike means NOT calling this part of the code.

[pangolp]

Merge state

Add more commits by pushing to the [los](/pangolp/azerothcore-wotlk/tree/los) branch on pangolp/azerothcore-wotlk.

It's just that it's not blizzlike, that's why I didn't put that in it.

please take a look at the existing vmap.BlizzlikePvPLOS setting.

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS))
{
    if (IsBattlegroundOrArena())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

if vmap.BlizzlikePvPLOS = true the ìf` block will NOT be executed.

Same would happen here:

if (!sWorld->getBoolConfig(CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD))
{
    if (IsWorldMap())
    {
        ignoreFlags = VMAP::ModelIgnoreFlags::Nothing;
    }
}

if CONFIG_VMAP_BLIZZLIKE_PVP_LOS_OPEN_WORLD = true, the ìf` block is NOT executed. so blizzlike means NOT calling this part of the code.

Lets see if I understand. So, we rename it, enable it by default in the configuration file and deny it within the conditional. Would that be the change you propose? If it is that, and it does exactly the same thing, I have no problem making those changes.

[pangolp]

Thanks for the suggestions, I think I understood what he was telling me, although I have to admit that it was a bit difficult for me, possibly because of the language issue haha. If it is necessary to make more changes, let me know. For now, we change the name and deny the conditional.

[Nyeriah]

In this case it doesn't affect PvP only and also affects creature casting iirc. Mobs will also be able to cast through trees etc.

[pangolp]

In this case it doesn't affect PvP only and also affects creature casting iirc. Mobs will also be able to cast through trees etc.

At least in the videos I made, that doesn't happen.

[pangolp]

The videos were outdated, because now the values are inverted.

[sudlud]

In this case it doesn't affect PvP only and also affects creature casting iirc. Mobs will also be able to cast through trees etc.

What name would you suggest for the setting

[Nyeriah]

In this case it doesn't affect PvP only and also affects creature casting iirc. Mobs will also be able to cast through trees etc.

What name would you suggest for the setting

Take PvP off and its fine as is

[pangolp]

I'll remove the PVP from the name then. Can something be done so that on a mountain you also avoid shooting if you are behind it? Because the mountains are not M2. They are part of the map. But for some reason, he doesn't realize that the coordinates prevent you from seeing the target. Anyway, it could be changed another time in some other pull request. I don't want to mix the topics too much. With this, it could already be interesting for some people. Many private servers, which do not share arrangements or respect licenses, have it. So it's something that they demanded that we try to have too, even though it's not blizzlike and it's not a default option.

[pangolp]

@sudlud @Nyeriah I think I'm done. Thanks for the suggestions. If there is anything else, let me know.

[sudlud]

Looks good to me. Can't really test it right now unfortunately, maybe someone else can give this a test please?

[pangolp]

Looks good to me. Can't really test it right now unfortunately, maybe someone else can give this a test please?

I can make a video again if necessary, because now, some things have been reversed, since by default now, it is enabled, and we have to do the opposite step which is to disable it. I think that beyond that, and the names that we changed, eliminating the pvp... I don't know if it can really affect anything else, because on top of that, it only runs in the open world. It is a shame that it does not take the mountains into consideration as well, but their problem is that it is a change in the relief of the ground, it is not a m2, so, obviously, it cannot be executed, at least this part of the code and we have to review that, somewhere else later.