Follow-up Comment #3, patch #1046 (project wesnoth):
Yeah, I was going back and forth between having it set the id or not. Also, I
have changed both the underlying id generation and the unit_id generation(in
the unit_map) to use a counter rather than the random number generator. Just
makes more sense that way.
Primarily though this patch was about cleaning up the unit_map a bit. This
required however that there be no id clashes and so I included that small
change.
To make your analysis of the changes a bit easier, I can give a quick rundown
of the inner workings of the unit_map.
First, it is a bit cluttered with all the different iterators-- I am
considering cleaning that up some next. Once you get past the iterators it
isn't too bad.
You have two maps
map<location, string> lmap
map<string, <bool, pair<location, unit>* > > map
This is a bit complicated but, I believe, it actually works relatively
nicely.
We want to do lookups by location, so we use the first map, basically it just
maps locations to the key for the second map. Why not just use a multi-key
container, well, I considered that and it would add some special case code
that I don't think is worth it, though I have not ruled that possibility
out.
The complexity comes from the fact that we don't want existing iterators to
break as we change the map and delete things from it. If an item is removed
and there is an iterator pointing to it we want there to be some way for that
iterator to know that it is no longer valid. One suggestion was to keep a
revision number in the map and in the iterators and if they didn't match,
basically refind what the iterator should point to. Since the map changes a
lot, this would be rather inefficient. So, how about a flag in the iterator
that keeps track of validity and only gets marked invalid when what it is
pointing to changes. Well, that would require a lot of keeping track of
iterators.
So I put the flag into what the iterator points to. That way, when say
erase() or extract() is called and the pair<...>* gets removed, we don't
actually delete that entry from the map, we just flip the flag to false. (and
delete the pointer if erase() is called). In this way we don't break
iterators, they just need to check the boolean flag before trying to access
the pointer. Also, we don't ever have to actually refind the unit we are
pointing to as we now index by id rather than location, so we can just update
the pair pointer rather than remove the entry and re-add it in the right
location.
Now one problem with this is that you don't ever remove entries from the map,
you just flip the bool to false. Now, I thought of two options to deal with
that. First you could have a public method that removes the invalid entries
and say that it is somebody else's responsibility to call that method when
there are no iterators. I didn't like that option, so instead I have the unit
keep a count of the number of iterators and when there are no iterators, if
there are sufficiently many invalid entries, it cleans them. The
iterator_counter may be able to be replaced with something from the boost
library w/ the same functionality, honestly, I am not sure.
So, since we don't automatically remove entries when a unit is taken off a
position, if we used a multi-key container it would require duplicate
location entries and that's where we would need special cases for that.
And to address the actual changes, previously I allowed duplicate
underlying_id() and just used some special case code to deal with it, and I
didn't like the way I did that. This change makes it a lot cleaner. In the
iterator_counter the boolean was just redundant.
So yeah, I guess I have a tendency to go on and on... so I'll stop now.
_______________________________________________________
Reply to this item at:
<http://gna.org/patch/?1046>
_______________________________________________
Message sent via/by Gna!
http://gna.org/
_______________________________________________
Wesnoth-bugs mailing list
[email protected]
https://mail.gna.org/listinfo/wesnoth-bugs
