Differences in Source Code
by Deozaan · in RTS Starter Kit · 09/21/2007 (12:35 pm) · 29 replies
While I was working on the Merge of TGE 1.5.2 and the RTSKit and thinking things weren't working, someone was kind enough to send me a copy of a clean RTSKit TGE 1.5.2 merge that was working successfully for them.
Once I realized (through this post) that my merge was fine but there are known bugs, I decided to do a diff with Beyond Compare of the merged source I had made and the merged source I was sent by another person.
I found a lot of various, minor differences, such as capitalization of variables, etc. But I also found a couple of significant differences. So I'd like to know which is correct. Both versions compile without errors and seems to run the same, but I have a feeling that some of this stuff may come back to bite me once I delve deeper into the development of RTS Kit for my game(s).
So for starters, I'll make it clear that my own merged code was made following the instructions and patch files from the TDN Article. The code that was sent to me by someone else was merged following instructions on this thread.
For one instance of the changed code:
In My source of engine\game\RTS\RTSConnection.cc @ 254
In the other source:
I'm no expert in C++ but I know that those two statements will get opposite answers, yet the code executed in the loop is exactly the same for both of them.
This is just one example. There are other files that have entire segments of differences between them, one of which comprises about 30 lines.
EDIT: Added links to corresponding posts/TDN.
Once I realized (through this post) that my merge was fine but there are known bugs, I decided to do a diff with Beyond Compare of the merged source I had made and the merged source I was sent by another person.
I found a lot of various, minor differences, such as capitalization of variables, etc. But I also found a couple of significant differences. So I'd like to know which is correct. Both versions compile without errors and seems to run the same, but I have a feeling that some of this stuff may come back to bite me once I delve deeper into the development of RTS Kit for my game(s).
So for starters, I'll make it clear that my own merged code was made following the instructions and patch files from the TDN Article. The code that was sent to me by someone else was merged following instructions on this thread.
For one instance of the changed code:
In My source of engine\game\RTS\RTSConnection.cc @ 254
if ( isConnectionToServer() )
{
gServerVisManager->handleConnectionDrop(this);
}In the other source:
if ( !isConnectionToServer() )
{
gServerVisManager->handleConnectionDrop(this);
}I'm no expert in C++ but I know that those two statements will get opposite answers, yet the code executed in the loop is exactly the same for both of them.
This is just one example. There are other files that have entire segments of differences between them, one of which comprises about 30 lines.
EDIT: Added links to corresponding posts/TDN.
#2
if( !isServerConnection() )
So I think that may be the correct way to do it. Otherwise it never seems to call the gServerVisManager stuff.
09/21/2007 (1:40 pm)
I just ran this through a debugger with the breakpoint at the code inside the loop. I joined my own lobby (from a separate PC) and the only way the breakpoint was hit was when the code was:if( !isServerConnection() )
So I think that may be the correct way to do it. Otherwise it never seems to call the gServerVisManager stuff.
#3
Ive tested disconnecting with both cases and makes no differences, but Im only local.
09/21/2007 (1:46 pm)
What did you do? disconnect the client by command or just cut the network connection?Ive tested disconnecting with both cases and makes no differences, but Im only local.
#4
Well eventually I got tired of jumping and read the code (and read the manual).
What that line do, it is actually clear the gServerVisManager, that is the server side list of visible units. But only if isServerConnection is true, which means that is a server connection (would be false, if isLocalConnection is true)
Edit: sorry, what that means, is that I think that negation "!" must NOT be there
09/21/2007 (2:09 pm)
Did you know what lazy means? To jump over the code from here to there like a monkey without actually reading it, and try to obtain a solution by doing that (Me).Well eventually I got tired of jumping and read the code (and read the manual).
Quote:The relevant functions are VisManager::processServer() and VisManager::processClient(), contained in engine/game/RTS/visManager.cc. There are client and server VisManager instances, contained in global variables, gServerVisManager and gClientVisManager.
What that line do, it is actually clear the gServerVisManager, that is the server side list of visible units. But only if isServerConnection is true, which means that is a server connection (would be false, if isLocalConnection is true)
Edit: sorry, what that means, is that I think that negation "!" must NOT be there
#5
Admittedly I don't understand the inner workings of C++ or this code, but I think it's the other way around. It needs to handle the disconnect because a client just disconnected from the server.
If the patch was simply to change the name of the function from isServerConnection to isConnectionToServer then maybe the ! got left out inadvertently.
EDIT: So I just re-read what you wrote. You're saying that the function is a list of visible units in the game? That might explain why the units kept disappearing on me anytime they moved. I'll do some testing and report back.
09/21/2007 (6:18 pm)
I connected to myself via my home network (2 PCs) and then joined my own lobby and disconnected from the lobby from the client.Admittedly I don't understand the inner workings of C++ or this code, but I think it's the other way around. It needs to handle the disconnect because a client just disconnected from the server.
If the patch was simply to change the name of the function from isServerConnection to isConnectionToServer then maybe the ! got left out inadvertently.
EDIT: So I just re-read what you wrote. You're saying that the function is a list of visible units in the game? That might explain why the units kept disappearing on me anytime they moved. I'll do some testing and report back.
#6
It could be that the "!" was inadvertently left out in the original work.
09/21/2007 (6:28 pm)
Deozaan, I think you might be correct: do a search here on this thread for !isServerConnection().It could be that the "!" was inadvertently left out in the original work.
#7
I've done some testing.
Without the ! causes some problems. When a player who is not hosting the game disconnects, it crashes TGE for the player who was the server.
When I disconnected in the lobby it was fine (probably because there were no visible units) but in the game with units there, it crashed it. So I've come to the conclusion that the ! needs to be in there to prevent a crash.
On another note, I am getting some weird visibility problems with units. Your own units look fine, but the other team's units disappear and reappear sporadically, even when they're right next to my own units. But only when they are moving.
Also, is fog of war enabled by default? Because I have to move my units within a few spaces of the other teams before they show up. Visibility is way too limited.
09/21/2007 (7:27 pm)
EDIT: Read more posts below as my findings here are not conclusive.I've done some testing.
Without the ! causes some problems. When a player who is not hosting the game disconnects, it crashes TGE for the player who was the server.
When I disconnected in the lobby it was fine (probably because there were no visible units) but in the game with units there, it crashed it. So I've come to the conclusion that the ! needs to be in there to prevent a crash.
On another note, I am getting some weird visibility problems with units. Your own units look fine, but the other team's units disappear and reappear sporadically, even when they're right next to my own units. But only when they are moving.
Also, is fog of war enabled by default? Because I have to move my units within a few spaces of the other teams before they show up. Visibility is way too limited.
#8
So maybe that's not the cause. . . Has anyone else who has merged the RTS Kit with 1.5.2 actually connected to a (non-local) client?
09/21/2007 (8:18 pm)
Well after changing my own code to have the ! and rebuilding, it still crashes the server when a client disconnects.So maybe that's not the cause. . . Has anyone else who has merged the RTS Kit with 1.5.2 actually connected to a (non-local) client?
#9
BTW, does anyone have any idea what bugs from the bugs forum have been fixed in any of the versions we are trying to work with?
This seems like a huge fucking mess.
09/21/2007 (9:20 pm)
I have been following this thread with interest, since I am interested ;) FYI, the original RTS kit code (based on TGE 1.3) has the !, which doesn't help a lot. Does it mean the original code was correct or was it fixed at some point? BTW, does anyone have any idea what bugs from the bugs forum have been fixed in any of the versions we are trying to work with?
This seems like a huge fucking mess.
#10
I didn't know how riddled with problems it was since I just bought it a couple days ago and went straight to work trying to merge it with 1.5.2.
So somehow the codebase that was sent to me fixed the crash error that happens in the original/fresh/clean RTS Kit and my own updated RTS Kit in TGE 1.5.2.
09/21/2007 (9:26 pm)
Another update: Just for fun I tried out a fresh install of the RTS Kit (with it's TGE 1.3 codebase) and it has exactly the same problems, with unit visibility acting a little funny and crashing when the non-local client disconnects from the server in the middle of the game, so I guess that's just a problem with the RTS Kit.I didn't know how riddled with problems it was since I just bought it a couple days ago and went straight to work trying to merge it with 1.5.2.
So somehow the codebase that was sent to me fixed the crash error that happens in the original/fresh/clean RTS Kit and my own updated RTS Kit in TGE 1.5.2.
#11
It does boogle my mind that the original code should crash when used multiplayer. You would think they would have tested that.
09/21/2007 (9:31 pm)
What we could use here is some input from Brian Kirchgessner on the origins of the patch and code he provided.It does boogle my mind that the original code should crash when used multiplayer. You would think they would have tested that.
#12
As I mentioned in the first post of this thread, I have other source files with big differences between the two. I was hoping for a quick answer to what I thought was a simple question (whether or not the ! needed to be in there) before I moved on to another source file. Should I start mentioning some of the other differences? They might provide more insight into what causes one to crash and the other not to.
09/21/2007 (9:42 pm)
Derry: The crashing during multiplayer might be unique to me (and my two PCs) since I don't know if anyone else has confirmed this.As I mentioned in the first post of this thread, I have other source files with big differences between the two. I was hoping for a quick answer to what I thought was a simple question (whether or not the ! needed to be in there) before I moved on to another source file. Should I start mentioning some of the other differences? They might provide more insight into what causes one to crash and the other not to.
#13
We now have several issues here, lets do some clasification work.
1) the insidious "!"
We dont know what this exactly do... although, the fantastic detective labour of James could already give us the answer. If you all agree, we should restore that "!" on the second revision of the patch.
2) the odd unit visibility behaviour
This is actually an issue already discussed on the bugs forum, I think it was resolved, but the solution is not yet integrated on the codebase. Be patient, after a functional port to TGE 1.5.2, we will work on that.
3) the several differences between the how-to code, with the Brian, and with the Deozzaan code
(Brian patch code == Deozaan code? please Deo, clarify this)
So! we have some work to do here gentlemen :D
Im going to sleep now, but tomorrow I'll be here like trooper again
Im really happy to see some debate and work here people :)
09/21/2007 (10:45 pm)
I go away for a couple of hours and look what you have done! :PWe now have several issues here, lets do some clasification work.
1) the insidious "!"
We dont know what this exactly do... although, the fantastic detective labour of James could already give us the answer. If you all agree, we should restore that "!" on the second revision of the patch.
2) the odd unit visibility behaviour
This is actually an issue already discussed on the bugs forum, I think it was resolved, but the solution is not yet integrated on the codebase. Be patient, after a functional port to TGE 1.5.2, we will work on that.
3) the several differences between the how-to code, with the Brian, and with the Deozzaan code
(Brian patch code == Deozaan code? please Deo, clarify this)
So! we have some work to do here gentlemen :D
Im going to sleep now, but tomorrow I'll be here like trooper again
Im really happy to see some debate and work here people :)
#14
1) I have no new news about it. I'll just reiterate that adding it hasn't seemed to affect anything I've noticed, though when I set breakpoints in the loop that only gets executed if the if statement is true, the only time it hits the breakpoint is with the !, so I think if whatever that code does in there is important, the ! needs to go in.
2) As I mentioned, I haven't played around with the RTS Kit prior to merging it with TGE 1.5.2 so I figured some of these funky glitches were a result of the merge. I didn't know these strange behaviors occurred with the original release of the RTS Kit. I have briefly browsed the Bug Report forum and didn't see any other mentions of strange visibility. Admittedly, I didn't have an in depth look at it.
3) I mentioned this in the original post, but I'll restate it here for clarity: My source is the source produced by following the instructions on the new TDN. The other source I've been comparing to was sent to me by Derry, who compiled it following the instructions in the old RTS Kit and TGE 1.5 How-To thread, which went through a couple of iterations from 1.5 to 1.5.1 to (where things became too confusing for me) some sketchy 1.5.2 merges. Novack is the one who posted the patch from the original 1.5 patch to 1.5.1, and Brian posted a patch meant to change it to 1.5.2.
So, a quick summary: My source follows the TDN article. The other source I'm comparing it to uses the thread linked above with patches of patches, which was ultimately posted by Brian Kirchgessner.
My source crashes when the client disconnects and the one Derry sent me doesn't.
EDIT: minor changes for clarity
09/21/2007 (11:13 pm)
In response to your 3 points:1) I have no new news about it. I'll just reiterate that adding it hasn't seemed to affect anything I've noticed, though when I set breakpoints in the loop that only gets executed if the if statement is true, the only time it hits the breakpoint is with the !, so I think if whatever that code does in there is important, the ! needs to go in.
2) As I mentioned, I haven't played around with the RTS Kit prior to merging it with TGE 1.5.2 so I figured some of these funky glitches were a result of the merge. I didn't know these strange behaviors occurred with the original release of the RTS Kit. I have briefly browsed the Bug Report forum and didn't see any other mentions of strange visibility. Admittedly, I didn't have an in depth look at it.
3) I mentioned this in the original post, but I'll restate it here for clarity: My source is the source produced by following the instructions on the new TDN. The other source I've been comparing to was sent to me by Derry, who compiled it following the instructions in the old RTS Kit and TGE 1.5 How-To thread, which went through a couple of iterations from 1.5 to 1.5.1 to (where things became too confusing for me) some sketchy 1.5.2 merges. Novack is the one who posted the patch from the original 1.5 patch to 1.5.1, and Brian posted a patch meant to change it to 1.5.2.
So, a quick summary: My source follows the TDN article. The other source I'm comparing it to uses the thread linked above with patches of patches, which was ultimately posted by Brian Kirchgessner.
My source crashes when the client disconnects and the one Derry sent me doesn't.
EDIT: minor changes for clarity
#15
1) Looking on the bug forums I finally found the definitive solution, you were right, the "!" goes there:
RTSConnection::onDisconnect() never called
RTSConnection::onDisconnect()--question
2) The visManager issues are discussed here, here and here on the Bugs forum and here and here on the General forums
I didnt read all of them, but for practical reasons I link here all what I could find.
SADLY the patch that was on the forums in no longer available. It seems that it got lost after a server crash or something like that. Maybe someone still have it. Anyone?
Anyway Josh Williams wrote the better definition: Visibility Manager... not behaving as designed (implementation is not so hot). Needs love. Well, I think it need A LOT of love, so when the time comes, we will need all the help we can get.
3) Perfect, point clarified, so we have two code versions to compare.
Ill definitively try to sleep a little.
09/22/2007 (12:03 am)
@Deo:1) Looking on the bug forums I finally found the definitive solution, you were right, the "!" goes there:
RTSConnection::onDisconnect() never called
RTSConnection::onDisconnect()--question
2) The visManager issues are discussed here, here and here on the Bugs forum and here and here on the General forums
I didnt read all of them, but for practical reasons I link here all what I could find.
SADLY the patch that was on the forums in no longer available. It seems that it got lost after a server crash or something like that. Maybe someone still have it. Anyone?
Anyway Josh Williams wrote the better definition: Visibility Manager... not behaving as designed (implementation is not so hot). Needs love. Well, I think it need A LOT of love, so when the time comes, we will need all the help we can get.
3) Perfect, point clarified, so we have two code versions to compare.
Ill definitively try to sleep a little.
#16
2) This post had some good information to make the units not blind, unfortunately, as is described here, the way the visManager works really breaks the way units show up.
Stephen Zepp clearly describes the problem:
So, without knowing where the code is or what it looks like, it seems to me like it is taking each enemy RTSUnit and looping through all of your RTSUnits to see if any of your guys can see it. If that's the way it works, it seems to me it just needs to loop through all units until it finds one that can see it, then break/continue out of the loop moving on to check the next Enemy's visibility. If it reaches the end of the loop and no units can see it, then set it to invisible.
To summarize: There are two things broken with unit visibility. The first is that your units are basically blind and can't see anything farther away than the end of their nose. The second is that an enemy unit could be standing right next to one of your units and still be marked as not visible because one of your units can't see it. You can fix the first problem (blindness) following the instructions found here but the second problem prevents you from noticing any great change in visibility that occur when fixing the first problem.
EDITED: for clarity
09/22/2007 (1:38 am)
1) Those threads indicate that putting in the ! will solve the client disconnect crash, but it hasn't for me. Unless I just don't know how to rebuild the project properly, like I said, I don't know much of anything about C++, that includes the IDE.2) This post had some good information to make the units not blind, unfortunately, as is described here, the way the visManager works really breaks the way units show up.
Stephen Zepp clearly describes the problem:
Quote: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.
Quote: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.
So, without knowing where the code is or what it looks like, it seems to me like it is taking each enemy RTSUnit and looping through all of your RTSUnits to see if any of your guys can see it. If that's the way it works, it seems to me it just needs to loop through all units until it finds one that can see it, then break/continue out of the loop moving on to check the next Enemy's visibility. If it reaches the end of the loop and no units can see it, then set it to invisible.
To summarize: There are two things broken with unit visibility. The first is that your units are basically blind and can't see anything farther away than the end of their nose. The second is that an enemy unit could be standing right next to one of your units and still be marked as not visible because one of your units can't see it. You can fix the first problem (blindness) following the instructions found here but the second problem prevents you from noticing any great change in visibility that occur when fixing the first problem.
EDITED: for clarity
#17
It does seem like a mess but both Novack and I are trying to take this in stages to ensure we capture everything.
No bug fixes from the forums have been implemented into the patch on TDN. The patch on TDN was a first step to get a base level "out of the box" kit working. Through community testing, we would fix any mistakes and any other base issues that might arise from a straight upgrade before apply bug fixes (which in themselves MIGHT cause issues).
Once down, we were going to apply the forum updates and put it up as something like "Forum bug fix patch" so that if any issues arouse from that we would know where to look.
I know this seems frustrating but I know Novack is committed (as well as I) to sorting this out with your help.
@ everyone - I suspect that Brian's code implements a number if not all the forum bug fixes. We really do need to hear from him to verify this.
Please be patient, we will sort this out and then we'll be able to move forward with the confidence we all share a solid RTS engine base.
09/22/2007 (3:15 am)
@ Derry BrysonQuote:BTW, does anyone have any idea what bugs from the bugs forum have been fixed in any of the versions we are trying to work with?
This seems like a huge fucking mess.
It does seem like a mess but both Novack and I are trying to take this in stages to ensure we capture everything.
No bug fixes from the forums have been implemented into the patch on TDN. The patch on TDN was a first step to get a base level "out of the box" kit working. Through community testing, we would fix any mistakes and any other base issues that might arise from a straight upgrade before apply bug fixes (which in themselves MIGHT cause issues).
Once down, we were going to apply the forum updates and put it up as something like "Forum bug fix patch" so that if any issues arouse from that we would know where to look.
I know this seems frustrating but I know Novack is committed (as well as I) to sorting this out with your help.
@ everyone - I suspect that Brian's code implements a number if not all the forum bug fixes. We really do need to hear from him to verify this.
Please be patient, we will sort this out and then we'll be able to move forward with the confidence we all share a solid RTS engine base.
#18
I fixed the visManager problems. It still has a couple of funky things about it, but they no longer flash in and out of visibility and you can actually see them from quite a distance across the map. I don't have time now, but I'll post the code changes later today and update this thread.
09/22/2007 (2:20 pm)
Good news everyone!I fixed the visManager problems. It still has a couple of funky things about it, but they no longer flash in and out of visibility and you can actually see them from quite a distance across the map. I don't have time now, but I'll post the code changes later today and update this thread.
#19
09/22/2007 (2:22 pm)
Out of curiosity, how bad is the performance (search order specifically) on your new algorithm? It always had a decently acceptable solution, but required a double run through of a very large set of nested for loops, so I'm curious which route you went.
#20
Stephen do you have the patch file mentioned on the visManager solution? the link is dead, and maybe that could help us.
With James we are working right now on trying to make a definitive merge between patches and versions. Im kind of lost on some of the lines Brian added to his code (I cant figure out his criteria).
We also found that the common folder of the stock RTS has a different structure than the TGE (and that witout mention the updates/changes there)
But overall, i could say we are almost there.
09/22/2007 (2:33 pm)
Great to know about advances on that.Stephen do you have the patch file mentioned on the visManager solution? the link is dead, and maybe that could help us.
With James we are working right now on trying to make a definitive merge between patches and versions. Im kind of lost on some of the lines Brian added to his code (I cant figure out his criteria).
We also found that the common folder of the stock RTS has a different structure than the TGE (and that witout mention the updates/changes there)
But overall, i could say we are almost there.
Torque 3D Owner Novack
CyberianSoftware
- if( !isServerConnection() )
+ if( isConnectionToServer() )
and I think they figured out some functional changes behind isServerConnection by isConnectionToServer and made that change on purpose, while possibly Brian just made a search and replace on the code.
Could you confirm this Brian?