Patch - Introduce mapviewcontrol based on Mehah method

[bakasuradbl]

No description provided.

[diath]

Hi, I pushed some minor changes to your PR branch but since the original PR was closed I'll address the concerns that @iryont had here.

Aware range is static now defined by constant values.
ViewportControl class uses aware range while it should use drawing dimension of the MapView it belongs to.

As far as I can tell at a quick glance it's only used when resetting and then it's not even updated when AwareRange is changed but as far as I can tell neither is the MapView::m_optimizedSize variable, furthermore I don't think it's updated at all when Map::setAwareRange is called, plus we're also using hardcoded default visible dimension values. I agree that it should probably properly share the dimensions with the view but I feel like it belongs to a separate issue where we could clean these things up altogether.

All of it should be done within updateVisibleTilesCache and not within drawing logic function which adds unnecessary indirection and logic to be calculated each frame.

This is a fair point, I just tried by doing the following, however there seems to be inconsistent black tile flickering on the edges of the map view when walking:

diff --git a/src/client/mapview.cpp b/src/client/mapview.cpp
index 24186244..83425b36 100644
--- a/src/client/mapview.cpp
+++ b/src/client/mapview.cpp
@@ -133,10 +133,6 @@ void MapView::draw(const Rect& rect)
         }
         g_painter->setColor(Color::white);
 
-        const LocalPlayerPtr player = g_game.getLocalPlayer();
-        const bool isWalking = player->isWalking() || player->isPreWalking() || player->isServerWalking();
-        const auto& mapViewControl = isWalking ? m_mapViewControl[player->getDirection()] : m_mapViewControl[Otc::InvalidDirection];
-
         auto it = m_cachedVisibleTiles.begin();
         auto end = m_cachedVisibleTiles.end();
         for(int z=m_cachedLastVisibleFloor;z>=m_cachedFirstVisibleFloor;--z) {
@@ -148,9 +144,6 @@ void MapView::draw(const Rect& rect)
                 else
                     ++it;
 
-                if(!mapViewControl.isValid(tile, cameraPosition))
-                    continue;
-
                 if (g_map.isCovered(tilePos, m_cachedFirstVisibleFloor))
                     tile->draw(transformPositionTo2D(tilePos, cameraPosition), scaleFactor, drawFlags);
                 else
@@ -322,6 +315,10 @@ void MapView::updateVisibleTilesCache(int start)
     if(!cameraPosition.isValid())
         return;
 
+    const LocalPlayerPtr player = g_game.getLocalPlayer();
+    const bool isWalking = player->isWalking() || player->isPreWalking() || player->isServerWalking();
+    const auto& mapViewControl = isWalking ? m_mapViewControl[player->getDirection()] : m_mapViewControl[Otc::InvalidDirection];
+
     bool stop = false;
 
     // clear current visible tiles cache
@@ -363,6 +360,8 @@ void MapView::updateVisibleTilesCache(int start)
                         // skip tiles that are completely behind another tile
                         if(g_map.isCompletelyCovered(tilePos, m_cachedFirstVisibleFloor))
                             continue;
+                        if(!mapViewControl.isValid(tile, cameraPosition))
+                            continue;
                         m_cachedVisibleTiles.push_back(tile);
                     }
                     m_updateTilesPos++;
@@ -412,7 +411,7 @@ void MapView::updateVisibleTilesCache(int start)
                 Position tilePos = cameraPosition.translated(p.x - m_virtualCenterOffset.x, p.y - m_virtualCenterOffset.y);
                 tilePos.coveredUp(cameraPosition.z - iz);
                 if(const TilePtr& tile = g_map.getTile(tilePos)) {
-                    if(tile->isDrawable())
+                    if(tile->isDrawable() && mapViewControl.isValid(tile, cameraPosition))
                         m_cachedVisibleTiles.push_back(tile);
                 }
             }

[iryont]

As far as I can tell at a quick glance it's only used when resetting and then it's not even updated when AwareRange is changed but as far as I can tell neither is the MapView::m_optimizedSize variable, furthermore I don't think it's updated at all when Map::setAwareRange is called

Then we might as well just take the aware range itself and make calculations based on values within it.

I agree that it should probably properly share the dimensions with the view but I feel like it belongs to a separate issue where we could clean these things up altogether.

The issue here might be with views which do not rely on the same ratio as Tibia does compared to its aware range. The current code assumes that, but we should only rely on the visible range of MapView since tiles beyond that viewport are the ones which need to be discarded. Imagine a scenario where you have zoomed in view, but tiles just beyond the view are still being drawn because the code takes aware range rather than the view range itself.

This is a fair point, I just tried by doing the following, however there seems to be inconsistent black tile flickering on the edges of the map view when walking:

The cache must be refreshed when the creature begins the walk and after it is done with the walk.

[diath]

The cache must be refreshed when the creature begins the walk and after it is done with the walk.

That seems to have worked but I'm not sure if I missed any places where it should be called.

[4drik]

@iryont
Can you review it now? Diath has rewritten the code based on your instructions.

[iryont]

@4drik I could, but I honestly don't like this code at all. Personally I believe this repository should be left alone with such questionable "fixes" and only quality PRs should be merged. This is why forks should be used instead, so no one is stopping anyone from using it. Though the idea behind this code is good, the implementation is rather bad.

The issue with @diath example is that MapView has a follow creature and there can be multiple map views with different follow creatures, so at first glance it is obvious that the only the MapView with specified follow creature (LocalPlayer) should be updated (requestVisibleTilesCacheUpdate) and not all of them.

Anyway, this whole idea can be really simplified and put under requestVisibleTilesCacheUpdate without the need for an extra class and unnecessary constants defining aware range which has nothing to do with MapView and its m_drawDimension (which is the one we need to check in the first place). In MapView, m_followingCreature should be checked for walking and not g_game.getLocalPlayer() which doesn't even have to be connected with the current MapView.

So basically with this PR you are breaking more things which will eventually have to fixed by someone in the future.

Though using this idea with a proper implementation is a good thing. If I find some time I will gladly create PR with with my own implementation.

[4drik]

@iryont
It has gotten to the point that new OTC versions are appearing, and yet I think we all want the original to be the main one - even if developed slowly. Bakasura, Diath and I tried, but you said you had better ideas and we believed in you. It's worth doing something once in a while.

[iryont]

@4drik I am sorry, but I am no longer interested in working on OTC. A lot of things have changed, unfortunately. I don't have time nor willingness to work on it.

You are free to do whatever you want at this point. I am out of this repository.