Update of patch #859 (project wesnoth):
Status: None => Wont Do
_______________________________________________________
Follow-up Comment #1:
Héhé you seems to spot all my unclean (but correct, I think) iterator use.
In this case, the goal of the code was to have any one iterator pointing to
the best object that we can find. That means, the cached fogged version (if
needed) and if not already cached, the normal cached version (we will fog it
later) and if never cached, we build it from scratch.
Now, I admit take an iterator pointing to somewhere into fog_cache and
comparing it with cache.end() is not clean. I could verify with STL, but I
think it's safe. When following the control flow, at this point the iterator
is either valid in fog_cache and then i hope it will correctly fails the
test==cache.end(), or if invalid in fog_cache, it was defined in cache 2
lines before.
Now for your patch, there is two problems (the first is little and the second
is more serious):
- the last 2 new lines cause a search in the not-fogged cache when we already
found what we want in the fogged cache. Searching in cache just for initialize
the iterator is not good for performance. This minimap cache is usually small,
but can possibly be bigger on a map with a lot of various terrain. Also, note
that this operation is done for every hex at each refresh of the minimap, so
big map amplify the effect.
- secondly, this redundant initialization of "i" doesn't work with the
following code. We later use surf=i->second, assuming that "i" points to what
we want (fogged or unfogged with a possible fogging needed). But since you now
use f_i for the fogged version, surf will never be the cached fogged surface
that we want. And so the minimap is not fogged anymore (as i tested). Well,
in theory the first hexes of each terrain type will maybe be fogged, but when
the fog cache will be fully initialized, it will trigger the bug.
We can maybe fix this and continue in this 2 iterators way, but it is what I
tried to avoid with my (unclean) use of only one. I will commit a version
where it's the cache that we switch not the iterator and so keep the use of
end() consistent, I think it will be cleaner.
Again thanks to spot the flaw in the code-design and so push me to clean it
with more comments explaining how it works. And sorry that we couldn't apply
your patch.
PS: But i will fix the comment typos ;)
_______________________________________________________
Reply to this item at:
<http://gna.org/patch/?859>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
Wesnoth-bugs mailing list
[email protected]
https://mail.gna.org/listinfo/wesnoth-bugs
