VisManager possible bug
by Stephen Zepp · in RTS Starter Kit · 12/03/2004 (1:57 pm) · 14 replies
Ok, we've been working on multi-play across the internet (two remote clients on a dedicated server) and noticed some really wonky stuff. The biggest thing noted was that units tended to "pop" in and out in a visible way, just about all at the same time.
In other words, I could walk my unit up to an enemy formation of 20 or so units, and then watch them all blink invisible, .5 second or so delay, and then all blink visible, .5 seconds again, then all go visible. This wasn't completely consistent, but it happened much more often than not.
We wanted to test our multi-player "detect enemy unit and attack" code, which depends on the vis manager to maintain the list of current "visible" units. We check each active unit (server side) during it's processTick, and if an enemy unit is within that friendly unit's "vis range", it would initiate an attack (all server side, and we set up all the wrappers, methods, and calls to do so).
Since we were only concerned about checking the functionality of the detect/select/attack code, we went ahead and set gFogofWarIsOn to false in visManager.cc. This had the net effect of not being able to -ever- see any enemy units, regardless of how close they were, so that for sure is a bug.
This of course led me into digging into the visManager code more closely, and I found something that doesn't really make any sense at all:
VisManager::processServer() looks pretty basic. cycle through each unit, and then run a test against every other unit in the game (with some logical exceptions) to see if the two units can see each other, and set the isVisible appropriately. Other than performance issues (for one, you actually check each set of unit pairs twice with this nested loop, something that can be optimized later), it makes sense and looks correct.
Now, VisManager::processClient() does some funky things that I can't help but think are typos.
The first thing that is pretty wierd is that we are on (for example) Client X's process, and parsing through the units Client X can see, but the very first check we do is:
if (target->getTeam() != conn->getTeam())
continue;
Now, keep in mind that we've already marked this particular unit as "Invisible". Now we're basically telling the client (as far as I can tell), that any unit that is not on your team stays invisible. Either I'm not reading the purpose of this code right, or this check is just wrong.
The second majorly wonky thing I don't understand is why are we testing the target's vision range, and not the unit's vision range? We're on a client, and we're stepping through all the units we think we should be able to see and confirming that--so we don't care what the target's vision range is (because the target client will be running this same check on his comp). What seems important to me is if our unit can see the enemy unit.
Has anyone taken a good look at the visManager stuff and understands it better than I do currently, or am I really seeing what I think I'm seeing?
In other words, I could walk my unit up to an enemy formation of 20 or so units, and then watch them all blink invisible, .5 second or so delay, and then all blink visible, .5 seconds again, then all go visible. This wasn't completely consistent, but it happened much more often than not.
We wanted to test our multi-player "detect enemy unit and attack" code, which depends on the vis manager to maintain the list of current "visible" units. We check each active unit (server side) during it's processTick, and if an enemy unit is within that friendly unit's "vis range", it would initiate an attack (all server side, and we set up all the wrappers, methods, and calls to do so).
Since we were only concerned about checking the functionality of the detect/select/attack code, we went ahead and set gFogofWarIsOn to false in visManager.cc. This had the net effect of not being able to -ever- see any enemy units, regardless of how close they were, so that for sure is a bug.
This of course led me into digging into the visManager code more closely, and I found something that doesn't really make any sense at all:
VisManager::processServer() looks pretty basic. cycle through each unit, and then run a test against every other unit in the game (with some logical exceptions) to see if the two units can see each other, and set the isVisible appropriately. Other than performance issues (for one, you actually check each set of unit pairs twice with this nested loop, something that can be optimized later), it makes sense and looks correct.
Now, VisManager::processClient() does some funky things that I can't help but think are typos.
The first thing that is pretty wierd is that we are on (for example) Client X's process, and parsing through the units Client X can see, but the very first check we do is:
if (target->getTeam() != conn->getTeam())
continue;
Now, keep in mind that we've already marked this particular unit as "Invisible". Now we're basically telling the client (as far as I can tell), that any unit that is not on your team stays invisible. Either I'm not reading the purpose of this code right, or this check is just wrong.
The second majorly wonky thing I don't understand is why are we testing the target's vision range, and not the unit's vision range? We're on a client, and we're stepping through all the units we think we should be able to see and confirming that--so we don't care what the target's vision range is (because the target client will be running this same check on his comp). What seems important to me is if our unit can see the enemy unit.
Has anyone taken a good look at the visManager stuff and understands it better than I do currently, or am I really seeing what I think I'm seeing?
#2
I re-wrote it so that it did a first pass to mark all units as invis except your own, and then did a second pass checking each unit that is yours against each unit that is an enemy, and only marking a unit as visible, never setting it back to invis (since we initialized the set to all invis in the first place).
The performance lost in having two passes is somewhat negligable due to the fact that we now only have to test one side of the unit pair, instead of both. (I think, your explanation tends to negate the understanding I had of the original code)
The new processor works fine for seeing your own units, waiting now for Greg to get done with dinner so we can test the multi-player aspect.
12/03/2004 (3:02 pm)
You could very well be correct, but as I said, it's majorly broken in any case :)I re-wrote it so that it did a first pass to mark all units as invis except your own, and then did a second pass checking each unit that is yours against each unit that is an enemy, and only marking a unit as visible, never setting it back to invis (since we initialized the set to all invis in the first place).
The performance lost in having two passes is somewhat negligable due to the fact that we now only have to test one side of the unit pair, instead of both. (I think, your explanation tends to negate the understanding I had of the original code)
The new processor works fine for seeing your own units, waiting now for Greg to get done with dinner so we can test the multi-player aspect.
#3
Basically, any improved implementation of the visManager is going to have to take into account that a unit's "seen" or "unseen" state is an additive set of all a client's units, not just based on the last client unit in the list.
The implementation I pointed out above is obviously not very performance optimized, but the concept it demonstrates is what I'm getting at. There are a lot of different ways to approach this problem, some of them better than others!
12/04/2004 (9:53 pm)
One of the big things we found in this area was how the way units are traversed and marked vis/not vis regardless of any changes in that state during the same process cycle. A unit could be marked as "seen" by one of your friendly units, but then get marked as "unseen" a few units later, negating the fact that one of your longer range (or closer) units had already spotted that particular enemy.Basically, any improved implementation of the visManager is going to have to take into account that a unit's "seen" or "unseen" state is an additive set of all a client's units, not just based on the last client unit in the list.
The implementation I pointed out above is obviously not very performance optimized, but the concept it demonstrates is what I'm getting at. There are a lot of different ways to approach this problem, some of them better than others!
#4
If you guys re-work the vis manager, awesome. If not, I plan to take a look at it after the holidays (can't promise a date). The design for it is great, but the implementation doesn't match up with the design, so I'll just implement our intended design when I get a chance (or maybe Ben will before me, but none of us will really have much time for the RTS SK until after we make it through the present push on other major projects).
12/06/2004 (3:47 pm)
The design for the vis manager was not properly implemented. I haven't seen problems as bad as you describe Stephen (despite extensive testing in various multiplayer settings), but it's certainly not all it could be. It is not broken, from all we could tell testing, no way we would've shipped knowing that. But, it does leave quite some to be desired.If you guys re-work the vis manager, awesome. If not, I plan to take a look at it after the holidays (can't promise a date). The design for it is great, but the implementation doesn't match up with the design, so I'll just implement our intended design when I get a chance (or maybe Ben will before me, but none of us will really have much time for the RTS SK until after we make it through the present push on other major projects).
#5
Would you be open to posting the design in the private forums? Maybe someone would take a crack at it for you. If not, I understand.
12/07/2004 (3:14 am)
@JoshWould you be open to posting the design in the private forums? Maybe someone would take a crack at it for you. If not, I understand.
#6
I fear you're operating under the incorrect assumption that we had reams of design docs for this kit. :(
12/07/2004 (9:09 pm)
@Jeff:I fear you're operating under the incorrect assumption that we had reams of design docs for this kit. :(
#7
Sounded like Josh was saying the design for the Vis Manager was very well laid out (maybe just in his head though).
12/08/2004 (3:18 am)
My Bad.Sounded like Josh was saying the design for the Vis Manager was very well laid out (maybe just in his head though).
#8
12/08/2004 (8:56 am)
Well, during the course of the RTS SK development we had semi-daily meetings to keep everyone up to date, do planning, etc. So in the course of those meetings we did get a design nailed down, but we never prepared a formal document. So yeah, you're more or less right about the head thing.
#9
Now, of course we've changed the visManager to meet our needs, and it is probable that this specific bug is our fault, but the only reason we changed it was to get it working properly in our dedicated setup.
Our biggest issue was trying to reverse-engineer exactly what it was -supposed- to do, and then reimplement it so it did that...even a simple 1 or 2 pager as to how it was originally supposed to function would give us a big headstart on making it more robust.
12/08/2004 (9:28 am)
@Ben: I've talked to Josh about this via email, but my team would be more than happy to help re-implement the visManager and give back the results. For the milestone we released last night we wound up having to switch to fog of war off because we just couldn't get the visManager fine tuned enough to stop causing 30-45 second freezes (hard freeze, can't even hit ~ to go to the console).Now, of course we've changed the visManager to meet our needs, and it is probable that this specific bug is our fault, but the only reason we changed it was to get it working properly in our dedicated setup.
Our biggest issue was trying to reverse-engineer exactly what it was -supposed- to do, and then reimplement it so it did that...even a simple 1 or 2 pager as to how it was originally supposed to function would give us a big headstart on making it more robust.
#10
12/08/2004 (10:43 am)
Sounds like an awesome plan. I'll talk to Josh about this, see what we can come up with.
#11
So, I wrote it up, but it seems it still wasn't clear, and we obviously didn't do a good enough job checking out the implementation. Well, there are lots of times that something like this comes up in development, it's just a result of imperfect human communication. :)
Anyway, I expect this issue to be resolved pretty quickly, once Stephen has the design. Or, if he takes a crack but feels that he won't have time to do it properly, we won't mind posting it here semi-publically.
Stephen, thanks again for offering to help out. It would probably be at least a couple weeks before one of us would have time to do this ourselves (at least, to do a non-hacky job of it).
12/10/2004 (4:00 am)
This is one thing that we did actually have a pretty detailed design doc for, which I will send to Stephen tomorrow when I'm back in the office. The reason this one got a pretty detailed written design was because it seemed like, no matter how many times Ben and I tried to communicate the design, it wasn't being understood totally. So, I wrote it up, but it seems it still wasn't clear, and we obviously didn't do a good enough job checking out the implementation. Well, there are lots of times that something like this comes up in development, it's just a result of imperfect human communication. :)
Anyway, I expect this issue to be resolved pretty quickly, once Stephen has the design. Or, if he takes a crack but feels that he won't have time to do it properly, we won't mind posting it here semi-publically.
Stephen, thanks again for offering to help out. It would probably be at least a couple weeks before one of us would have time to do this ourselves (at least, to do a non-hacky job of it).
#12
12/10/2004 (9:56 am)
Me and my snide implications we fly by the seat of our pants. ;) Sorry guys. 
#13
03/21/2010 (10:10 pm)
How about releasing the fix guys ? Its been "waiting for GG" approval for years now ...
#14
03/22/2010 (4:26 am)
Yes please (Fix IT) 
Torque 3D Owner Martin "Founder" Hoover
The first part of the loop skips your own units, thus when it gets to the nested loop, the unit it's processing is an enemy unit. Then in the nest, it skips enemy units, but any of your units it finds, it does the distance check to see if the original enemy unit is visible to any of them.
This is assuming that conn is refering to your own client, which I am pretty sure it is, since otherwise it would make the function rather arbitrary.