Looking for code feedback?
by Ron Yacketta · in Torque X 3D · 11/19/2006 (8:07 am) · 29 replies
GG,
Are you interested in hearing code feedback? like usage of read/write properties, code formating etc? I have noticed a few oddities in the beta and just wanted to see if that feedback is warrented/welcomed before I post.
-Ron
Are you interested in hearing code feedback? like usage of read/write properties, code formating etc? I have noticed a few oddities in the beta and just wanted to see if that feedback is warrented/welcomed before I post.
-Ron
#2
The views expressed in this message are simply my opinion and not in any way a put-down to the coding ability of the GG staff. I am nit-picky with code I work with and often times critical. I hope this can be taken as constructive criticism instead of a slam.
In no particular order:
* Put methods last, properties/attributes/fields first - Even though anything can go anywhere in C#, it's much easier to read code if I know what everything is before it's being used.
* Put private 1st, protected 2nd, friend/internal 3rd, public 4th - Again, makes things easier to read and you can put them in #regions so all the public/private/etc is segregated.
* Comments (xml and non-xml comments) - There are next to no comments of any use in the code. XML comments on the methods is EMENSELY important to helping us understand (with a simple mouseover) what the heck something is supposed to do. Some commenting exists, but nowhere near complete.
* Have GUI specific class for all GUI code files (not core files, actual GUI files) instead of making everything a partial class of "Game" - This is one of my strongest points. I REALLY think putting everything under one main class is a bad idea. Classes are your friend!
* Check for controller or keyboard, only bind what you need to bind - Execute less code = better.
* Use XML for GUI? - I think that an XML property page would work well with the c# code wrapping it.
* Hate the use of _variableName for anything but class properties. Anything that you inherit from should use regular camel casing so _someVariable should be someVariable unless it's within a class you intend to inherit from, in which case _someVariable is appropriate for properties.
* Hungarian notation = the devil - If I have to elaborate on this, I'm gonna cry.
That's about it for now. Only had about 30 mins to take a peek at it, so will be playing with it more over the next few days. Some codesmith templates would do wonders for quickly building GUIs. If it's something completely out of scope for GG, I may whip something up for the community closer to the launch.
11/20/2006 (10:24 pm)
Welp, you asked for it.In no particular order:
* Put methods last, properties/attributes/fields first - Even though anything can go anywhere in C#, it's much easier to read code if I know what everything is before it's being used.
* Put private 1st, protected 2nd, friend/internal 3rd, public 4th - Again, makes things easier to read and you can put them in #regions so all the public/private/etc is segregated.
* Comments (xml and non-xml comments) - There are next to no comments of any use in the code. XML comments on the methods is EMENSELY important to helping us understand (with a simple mouseover) what the heck something is supposed to do. Some commenting exists, but nowhere near complete.
* Have GUI specific class for all GUI code files (not core files, actual GUI files) instead of making everything a partial class of "Game" - This is one of my strongest points. I REALLY think putting everything under one main class is a bad idea. Classes are your friend!
* Check for controller or keyboard, only bind what you need to bind - Execute less code = better.
* Use XML for GUI? - I think that an XML property page would work well with the c# code wrapping it.
* Hate the use of _variableName for anything but class properties. Anything that you inherit from should use regular camel casing so _someVariable should be someVariable unless it's within a class you intend to inherit from, in which case _someVariable is appropriate for properties.
* Hungarian notation = the devil - If I have to elaborate on this, I'm gonna cry.
That's about it for now. Only had about 30 mins to take a peek at it, so will be playing with it more over the next few days. Some codesmith templates would do wonders for quickly building GUIs. If it's something completely out of scope for GG, I may whip something up for the community closer to the launch.
#3
Let me explain where we are at on each of these:
* Put methods last, properties/attributes/fields first - Even though anything can go anywhere in C#, it's much easier to read code if I know what everything is before it's being used.
Our order should be clear from the source. We put public properties first, then public methods, then non-public methods, then non-public fields. Our guide here is that the most public things should go first. Obviously we aren't going to change this at this stage, but I understand not everyone will think it's the cat's meow.
* Put private 1st, protected 2nd, friend/internal 3rd, public 4th - Again, makes things easier to read and you can put them in #regions so all the public/private/etc is segregated.
That's certainly always been our convention in C++. But that's mostly a result of private being the default. For C# we decided we liked the most public things at the top, since that saves you from digging through the file for the public interface. I hope you did notice that we use regions extensively.
* Comments (xml and non-xml comments) - There are next to no comments of any use in the code. XML comments on the methods is EMENSELY important to helping us understand (with a simple mouseover) what the heck something is supposed to do. Some commenting exists, but nowhere near complete.
API comments are one of the last things we add. No sense commenting the interface when it's still in flux. In beta 2 core, sim, and t2d should have api comments (t2d may have been after beta 2). Rest assured we'll have complete api docs for release. BTW, we did most of this work without XNA documentation and for a while no api comments, so you're ahead of us on that one.
In terms of coding comments, we'll have to look at that more as we go forward. People vary on how much they think code needs to be commented. Sometimes over commenting is worse than not commenting at all because comments get out of date. I'm sure we could use some more comments though.
* Have GUI specific class for all GUI code files (not core files, actual GUI files) instead of making everything a partial class of "Game" - This is one of my strongest points. I REALLY think putting everything under one main class is a bad idea. Classes are your friend!
Not following you on this one. Partial class of game? I didn't do the gui so I may just be missing the context.
* Check for controller or keyboard, only bind what you need to bind - Execute less code = better.
Define "bind". We send input events through torque for many input events, if that's what you mean. Realize that input events don't happen except when there is input. In terms of actual bindings of input events to execution of code, that would only happen if the game programmer makes a binding.
* Use XML for GUI? - I think that an XML property page would work well with the c# code wrapping it.
Maybe you can explain this one a little more. Not following you. Maybe Robert (who wrote the gui layer) will know what you mean.
* Hate the use of _variableName for anything but class properties. Anything that you inherit from should use regular camel casing so _someVariable should be someVariable unless it's within a class you intend to inherit from, in which case _someVariable is appropriate for properties.
We use _variableName for private/protected/internal fields and _FunctionName for private/protected/internal methods. In my experience, there is no reason to get too tied up in any one coding standard. One is no better worse than another. What's important is that it is consistent throughout. Certainly if you spot places where we diverge from our own style, let us know because that we do want to correct. But I do like member variables to be distinct from stack variables, which the _ convention achieves (as does the mVariableName convention used in TGE -- why we aren't using that is a long story I won't bore you with).
* Hungarian notation = the devil - If I have to elaborate on this, I'm gonna cry.
Are you just giving us your pet peaves or do we use hungarian notation somewhere. If so, please let us know. We do have a company wide coding guide and it specifically says no Hungarian notation. I imagine a pSomeVar may have snuck in someplace simply from ported code, but nothing worse than that (and if we have that anywhere we'll fix it if you let us know).
Thanks for the comments John. I know it was only a 30 minute peek, but if you have other comments let us know. Certainly we're not going to pay much heed to suggestions of what order to put our variables (unless you make a really good case, but even then it wouldn't be worth the time to change) but any bugs, performance issues, or clarity issues we'd be very excited to hear about. Any weird naming issues too.
11/20/2006 (10:58 pm)
We're looking for any feedback you want to give, but realize coding style feedback isn't going to help much unless it's a really salient important point. People tend to have their own styles (they are like other things everyone has, if you know the phrase I refer to). Let me explain where we are at on each of these:
* Put methods last, properties/attributes/fields first - Even though anything can go anywhere in C#, it's much easier to read code if I know what everything is before it's being used.
Our order should be clear from the source. We put public properties first, then public methods, then non-public methods, then non-public fields. Our guide here is that the most public things should go first. Obviously we aren't going to change this at this stage, but I understand not everyone will think it's the cat's meow.
* Put private 1st, protected 2nd, friend/internal 3rd, public 4th - Again, makes things easier to read and you can put them in #regions so all the public/private/etc is segregated.
That's certainly always been our convention in C++. But that's mostly a result of private being the default. For C# we decided we liked the most public things at the top, since that saves you from digging through the file for the public interface. I hope you did notice that we use regions extensively.
* Comments (xml and non-xml comments) - There are next to no comments of any use in the code. XML comments on the methods is EMENSELY important to helping us understand (with a simple mouseover) what the heck something is supposed to do. Some commenting exists, but nowhere near complete.
API comments are one of the last things we add. No sense commenting the interface when it's still in flux. In beta 2 core, sim, and t2d should have api comments (t2d may have been after beta 2). Rest assured we'll have complete api docs for release. BTW, we did most of this work without XNA documentation and for a while no api comments, so you're ahead of us on that one.
In terms of coding comments, we'll have to look at that more as we go forward. People vary on how much they think code needs to be commented. Sometimes over commenting is worse than not commenting at all because comments get out of date. I'm sure we could use some more comments though.
* Have GUI specific class for all GUI code files (not core files, actual GUI files) instead of making everything a partial class of "Game" - This is one of my strongest points. I REALLY think putting everything under one main class is a bad idea. Classes are your friend!
Not following you on this one. Partial class of game? I didn't do the gui so I may just be missing the context.
* Check for controller or keyboard, only bind what you need to bind - Execute less code = better.
Define "bind". We send input events through torque for many input events, if that's what you mean. Realize that input events don't happen except when there is input. In terms of actual bindings of input events to execution of code, that would only happen if the game programmer makes a binding.
* Use XML for GUI? - I think that an XML property page would work well with the c# code wrapping it.
Maybe you can explain this one a little more. Not following you. Maybe Robert (who wrote the gui layer) will know what you mean.
* Hate the use of _variableName for anything but class properties. Anything that you inherit from should use regular camel casing so _someVariable should be someVariable unless it's within a class you intend to inherit from, in which case _someVariable is appropriate for properties.
We use _variableName for private/protected/internal fields and _FunctionName for private/protected/internal methods. In my experience, there is no reason to get too tied up in any one coding standard. One is no better worse than another. What's important is that it is consistent throughout. Certainly if you spot places where we diverge from our own style, let us know because that we do want to correct. But I do like member variables to be distinct from stack variables, which the _ convention achieves (as does the mVariableName convention used in TGE -- why we aren't using that is a long story I won't bore you with).
* Hungarian notation = the devil - If I have to elaborate on this, I'm gonna cry.
Are you just giving us your pet peaves or do we use hungarian notation somewhere. If so, please let us know. We do have a company wide coding guide and it specifically says no Hungarian notation. I imagine a pSomeVar may have snuck in someplace simply from ported code, but nothing worse than that (and if we have that anywhere we'll fix it if you let us know).
Thanks for the comments John. I know it was only a 30 minute peek, but if you have other comments let us know. Certainly we're not going to pay much heed to suggestions of what order to put our variables (unless you make a really good case, but even then it wouldn't be worth the time to change) but any bugs, performance issues, or clarity issues we'd be very excited to hear about. Any weird naming issues too.
#4
The following is not bashing, flame or whatever the 'negative term of the day is' against you. These are just my personal viewes and opinions when I look at the code.
At work we follow what MS does in their code (using reflector), just to make it all mesh and flow. I am composing a list of items that seem odd to me and would take some getting used to for other seasoned C# developers.
Here is a couple that stick out like a sore thumb.
Reading through the source code in B2 and noticed in several places you combine classes and interfaces in a single file, yes their is absolutly nothing wrong with this at all. The norm I have seen from reflecting MS code (as well as C# standard) is to split interfaces and classes into seperate files.
If your going to make a read/write property then why not use it? I have seen multiple public read/write propteries in many classes that are rarely used within the class itself. Yes; the public read/write exposses itself for external usage, but it is also available for internal class usage as well.
Those are just two of the many oddities I have picked out so far. Yah, I am looking at the code with MS coding standards in mind and as a C# developer not C++. This is the standard I have followed and used the past 3+ years doing ASP.NET C# development
11/21/2006 (5:34 am)
GG,The following is not bashing, flame or whatever the 'negative term of the day is' against you. These are just my personal viewes and opinions when I look at the code.
At work we follow what MS does in their code (using reflector), just to make it all mesh and flow. I am composing a list of items that seem odd to me and would take some getting used to for other seasoned C# developers.
Here is a couple that stick out like a sore thumb.
Reading through the source code in B2 and noticed in several places you combine classes and interfaces in a single file, yes their is absolutly nothing wrong with this at all. The norm I have seen from reflecting MS code (as well as C# standard) is to split interfaces and classes into seperate files.
If your going to make a read/write property then why not use it? I have seen multiple public read/write propteries in many classes that are rarely used within the class itself. Yes; the public read/write exposses itself for external usage, but it is also available for internal class usage as well.
Those are just two of the many oddities I have picked out so far. Yah, I am looking at the code with MS coding standards in mind and as a C# developer not C++. This is the standard I have followed and used the past 3+ years doing ASP.NET C# development
#5
Re: read/write properties. Read/write properties do not always get inline. There have been quite a few profiler guided decisions to use the internal version. One thing I've noticed is that once a method gets long the compiler doesn't want to inline anything, even simple properties. Of coures, a lot of what you are seeing might just be one of us using the internal version rather than the property (maybe me I've never thought about it) because it felt more natural in that case. We don't have a standard on this. To be honest, though, I don't think it matters (I understand that you follow a coding convention that has a standard for this, but that's ok, conventions will vary).
11/21/2006 (8:56 am)
Ron, thanks for your feedback. Confused though. Won't reflector show you class information in it's own file no matter what? Have you seen it do otherwise? Certainly the reflection built into the ide works this way. In any case, most classes are certainly alone in their file, but when they have highly bundled interfaces with them I tend to like them close to there use. We'll look at this more as we get close to ship though.Re: read/write properties. Read/write properties do not always get inline. There have been quite a few profiler guided decisions to use the internal version. One thing I've noticed is that once a method gets long the compiler doesn't want to inline anything, even simple properties. Of coures, a lot of what you are seeing might just be one of us using the internal version rather than the property (maybe me I've never thought about it) because it felt more natural in that case. We don't have a standard on this. To be honest, though, I don't think it matters (I understand that you follow a coding convention that has a standard for this, but that's ok, conventions will vary).
#6
I do not rely on the MS IDE for much of the reflection information.. MS likes to hide things.. those sneaky little umm err developers ;)
Try the following too www.aisto.com/roeder/dotnet/Download.aspx?File=Reflector
expand mscorlib and then expand CommonLanguageRuntimeLibrary and finaly expand System.Collections and poke around, you will notice a few interfaces like System.Collections.ICollection, System.Collections.IComparer and System.Collections.IDictionary.
I agree regarding properties, it is more of an oddity to me than anything. Never have come across an issue were my property names are to long and causing issues.
I am liking what I see in B2, just need to wrap my head around a different style of reading the code. I can see it as a definate short term set back for most C# and C++ developers, but nothing that will hold up game dev.
Oh!
Another tool you might find useful is JetBrains ReSharper.
-Ron
11/21/2006 (9:21 am)
Clark,I do not rely on the MS IDE for much of the reflection information.. MS likes to hide things.. those sneaky little umm err developers ;)
Try the following too www.aisto.com/roeder/dotnet/Download.aspx?File=Reflector
expand mscorlib and then expand CommonLanguageRuntimeLibrary and finaly expand System.Collections and poke around, you will notice a few interfaces like System.Collections.ICollection, System.Collections.IComparer and System.Collections.IDictionary.
I agree regarding properties, it is more of an oddity to me than anything. Never have come across an issue were my property names are to long and causing issues.
I am liking what I see in B2, just need to wrap my head around a different style of reading the code. I can see it as a definate short term set back for most C# and C++ developers, but nothing that will hold up game dev.
Oh!
Another tool you might find useful is JetBrains ReSharper.
-Ron
#7
Would try ReSharper except we are using VS Express (just like our audience). We develop indie style. Have been using JetBrains profiler quite a bit though. It's pretty good (the new version allows you to take the difference between two profiles, which can be handy for highlighting subtle effects).
Anyone have a profiler they like to use? I know the team version of VS has a nice one built in.
11/21/2006 (10:07 am)
Hey Ron. Yeah, I used reflector quite a bit at the start because we had no docs on xna and had to read the MS mind. But if you use it on ProcessList, e.g., you'll see that ITickObject isn't inside process list even though it's declared in the file. Not important though.Would try ReSharper except we are using VS Express (just like our audience). We develop indie style. Have been using JetBrains profiler quite a bit though. It's pretty good (the new version allows you to take the difference between two profiles, which can be handy for highlighting subtle effects).
Anyone have a profiler they like to use? I know the team version of VS has a nice one built in.
#8
Okie, here we go:
Why put fields (public or not) last? You are using them in the methods, so wouldn't it make more sense to put the fields first so we know what data type they are and exactly how they are getting declared/initialized?
Yes, I noticed a nice usage of regions. This order is more of a preference than anything outside of what was posted above.
OK. Just wanted to make sure because I saw some things commented and some not (xml comments) so wanted to make sure in the end you guys at least knew that it was a good idea.
I agree here about too much commenting. My example of good commenting and bad commenting would be thus:
BAD COMMENTING: GUIBitmap.cs OnRender method
The only comments in the above snippet of code are on the easiest to figure out sections. Although this code is fairly self-explanatory, it wont be to everyone and is merely 1 example.
11/21/2006 (1:32 pm)
Resharper FTW.Okie, here we go:
Quote:Our order should be clear from the source. We put public properties first, then public methods, then non-public methods, then non-public fields. Our guide here is that the most public things should go first. Obviously we aren't going to change this at this stage, but I understand not everyone will think it's the cat's meow.
Why put fields (public or not) last? You are using them in the methods, so wouldn't it make more sense to put the fields first so we know what data type they are and exactly how they are getting declared/initialized?
Quote:That's certainly always been our convention in C++. But that's mostly a result of private being the default. For C# we decided we liked the most public things at the top, since that saves you from digging through the file for the public interface. I hope you did notice that we use regions extensively.
Yes, I noticed a nice usage of regions. This order is more of a preference than anything outside of what was posted above.
Quote:API comments are one of the last things we add. No sense commenting the interface when it's still in flux. In beta 2 core, sim, and t2d should have api comments (t2d may have been after beta 2). Rest assured we'll have complete api docs for release. BTW, we did most of this work without XNA documentation and for a while no api comments, so you're ahead of us on that one.
OK. Just wanted to make sure because I saw some things commented and some not (xml comments) so wanted to make sure in the end you guys at least knew that it was a good idea.
Quote:In terms of coding comments, we'll have to look at that more as we go forward. People vary on how much they think code needs to be commented. Sometimes over commenting is worse than not commenting at all because comments get out of date. I'm sure we could use some more comments though.
I agree here about too much commenting. My example of good commenting and bad commenting would be thus:
BAD COMMENTING: GUIBitmap.cs OnRender method
if (_bitmap != null)
{
if (_style.TextureWrap)
{
RectF srcRegion = new RectF();
RectF dstRegion = new RectF();
float xDone = (_bounds.Size.Width / bitmapSize.Width) + 1;
float yDone = (_bounds.Size.Height / bitmapSize.Height) + 1;
int xShift = (int)_wrapStart.X % (int)bitmapSize.Width;
int yShift = (int)_wrapStart.Y % (int)bitmapSize.Height;
for (int y = 0; y < yDone; ++y)
{
for (int x = 0; x < xDone; ++x)
{
srcRegion.X = 0;
srcRegion.Y = 0;
srcRegion.Width = bitmapSize.Width;
srcRegion.Height = bitmapSize.Height;
dstRegion.X = ((bitmapSize.Width * x) + offset.X) - xShift;
dstRegion.Y = ((bitmapSize.Height * y) + offset.Y) - yShift;
dstRegion.Width = bitmapSize.Width;
dstRegion.Height = bitmapSize.Height;
// draw the bitmap
DrawUtil.BitmapStretchSR(_bitmapName, dstRegion, srcRegion, BitmapFlip.None);
}
}
}
else
{
// draw the bitmap
DrawUtil.BitmapStretch(_bitmapName, ctrlRect, bitmapSize, BitmapFlip.None);
}
}The only comments in the above snippet of code are on the easiest to figure out sections. Although this code is fairly self-explanatory, it wont be to everyone and is merely 1 example.
#9
This is a very good use and non-overuse of comments. The only change I would make to the above would be to have the 'black to image, image to black' etc. comments as xml comments on the actual properties versus here. That way, when you mouse over 'fadeinsec' you see the description.
What I meant here is that if you look at all the different files they are all in the "game" class: GarageGames.Torque.Examples.TankBuster.Game. I would break the GUI files out of the game class and put them into a GUI class GarageGames.Torque.Examples.TankBuster.GUI (not to be confused with the GUI control classes located: GarageGames.Torque.GUI).
In the above snippet, it is binding the gamepad's back button to the actionmap. Then, below it (not posted) you bind a keyboard command as well. This is what I meant, bind one or the other based on if they are using the xbox or not. Just need to do a:
The web.config and app.config files are xml files where you can load properties during runtime for your applications. I propose the same setup for GUI files. Much like you guys use .gui for GUI files in TGE. The code would stay in c# files to utilize the GUI and wire functionality, but the actual files themselves could be xml. I wouldn't expect this for a December release at all, but something to think about moving forward.
MainScreen.cs:
_btnImageStyle
_btnImageA
_btnImageB
Just a few bits here and there. Mostly you didn't use hungarian, but there are a few spots. All the buttons I found in the GUIs are using hungarian.
Of note as well, making the variables mean something would be ideal too if possible. (again, this is nitpicky stuff, so don't sweat it to much as it's just my opinion for easier to read code)
For instance, you put _btnImageA. That means nothing to us except to tell us it may be a button image. For that, you could have put: _padButtonB which would associate with the graphic: data\images\pad_button_b.
And I must BEG that you take care of things like such:
S as a variable name! arg! ;)
Just my buck fifty of opinion again!
11/21/2006 (1:32 pm)
Good use of comments: SplashScreen.cs _createSplashScreen method// create the Style for our splash ad
_splashStyle = new GUISplashStyle();
_splashStyle.FadeInSec = 2; // black to image 2 seconds
_splashStyle.FadeOutSec = 2; // image to black 2 seconds
_splashStyle.FadeWaitSec = 4; // still image for 4 seconds
_splashStyle.Bitmap = @"data\images\GarageGames";
_splashStyle.Anchor = AnchorFlags.All;
// now create the splash ad control
_splashGUI = new GUISplash();
_splashGUI.Name = "SplashScreen";
_splashGUI.Style = _splashStyle;
_splashGUI.OnFadeFinished = Game.Instance.OnSplashFinished;This is a very good use and non-overuse of comments. The only change I would make to the above would be to have the 'black to image, image to black' etc. comments as xml comments on the actual properties versus here. That way, when you mouse over 'fadeinsec' you see the description.
Quote:* Have GUI specific class for all GUI code files (not core files, actual GUI files) instead of making everything a partial class of "Game" - This is one of my strongest points. I REALLY think putting everything under one main class is a bad idea. Classes are your friend!
Not following you on this one. Partial class of game? I didn't do the gui so I may just be missing the context.
What I meant here is that if you look at all the different files they are all in the "game" class: GarageGames.Torque.Examples.TankBuster.Game. I would break the GUI files out of the game class and put them into a GUI class GarageGames.Torque.Examples.TankBuster.GUI (not to be confused with the GUI control classes located: GarageGames.Torque.GUI).
Quote:Define "bind". We send input events through torque for many input events, if that's what you mean. Realize that input events don't happen except when there is input. In terms of actual bindings of input events to execution of code, that would only happen if the game programmer makes a binding.
_playGUI.ActionMap.BindCommand(gamepadId, (int)XGamePadDevice.GamePadObjects.Back, null, Game.Instance.UnloadLevel);
In the above snippet, it is binding the gamepad's back button to the actionmap. Then, below it (not posted) you bind a keyboard command as well. This is what I meant, bind one or the other based on if they are using the xbox or not. Just need to do a:
#if XBOX Bind xbox controls here #else Bind keyboard controls #endif
Quote:Maybe you can explain this one a little more. Not following you. Maybe Robert (who wrote the gui layer) will know what you mean.
The web.config and app.config files are xml files where you can load properties during runtime for your applications. I propose the same setup for GUI files. Much like you guys use .gui for GUI files in TGE. The code would stay in c# files to utilize the GUI and wire functionality, but the actual files themselves could be xml. I wouldn't expect this for a December release at all, but something to think about moving forward.
Quote:Are you just giving us your pet peaves or do we use hungarian notation somewhere. If so, please let us know. We do have a company wide coding guide and it specifically says no Hungarian notation. I imagine a pSomeVar may have snuck in someplace simply from ported code, but nothing worse than that (and if we have that anywhere we'll fix it if you let us know).
MainScreen.cs:
_btnImageStyle
_btnImageA
_btnImageB
Just a few bits here and there. Mostly you didn't use hungarian, but there are a few spots. All the buttons I found in the GUIs are using hungarian.
Of note as well, making the variables mean something would be ideal too if possible. (again, this is nitpicky stuff, so don't sweat it to much as it's just my opinion for easier to read code)
For instance, you put _btnImageA. That means nothing to us except to tell us it may be a button image. For that, you could have put: _padButtonB which would associate with the graphic: data\images\pad_button_b.
And I must BEG that you take care of things like such:
T2DScroller s = _ground;
_groundSet.Object.AddItem(s);
s = (T2DScroller)TorqueObjectDatabase.Instance.FindObject("Hills");
_groundSet.Object.AddItem(s);
s = (T2DScroller)TorqueObjectDatabase.Instance.FindObject("Mountains");
_groundSet.Object.AddItem(s);S as a variable name! arg! ;)
Just my buck fifty of opinion again!
#10
We have no non-public fields (except for some data only classes which are that way for speed). We put all non-public fields/methods/properties last because they are not part of the public interface, they are part of the internal working of the class.
That makes sense, but remember that TankBuster is merely an example game. But we do want to write our demos as we expect our audience to write their games so this is a good point.
Re: binds. We do that so that one can use either keyboard or gamepad. It really isn't a significant cost to add an extra bind if no extra input events are generated. Part of the charm of combining the action maps and move managers is that you can map keyboard and game pad to the same game object and not have to worry about the real source of the move on your game object. But again, in your own game you don't need to make both these binds.
We'll be sure to consider your other suggestions as we continue working on the engine.
Thanks again for your input.
11/21/2006 (1:47 pm)
Quote:
Why put fields (public or not) last? You are using them in the methods, so wouldn't it make more sense to put the fields first so we know what data type they are and exactly how they are getting declared/initialized?
We have no non-public fields (except for some data only classes which are that way for speed). We put all non-public fields/methods/properties last because they are not part of the public interface, they are part of the internal working of the class.
Quote:
What I meant here is that if you look at all the different files they are all in the "game" class: GarageGames.Torque.Examples.TankBuster.Game. I would break the GUI files out of the game class and put them into a GUI class GarageGames.Torque.Examples.TankBuster.GUI (not to be confused with the GUI control classes located: GarageGames.Torque.GUI).
That makes sense, but remember that TankBuster is merely an example game. But we do want to write our demos as we expect our audience to write their games so this is a good point.
Re: binds. We do that so that one can use either keyboard or gamepad. It really isn't a significant cost to add an extra bind if no extra input events are generated. Part of the charm of combining the action maps and move managers is that you can map keyboard and game pad to the same game object and not have to worry about the real source of the move on your game object. But again, in your own game you don't need to make both these binds.
We'll be sure to consider your other suggestions as we continue working on the engine.
Thanks again for your input.
#11
are you 100% sure that you will get a 'Hills' object back? what happens if you dont in the above code?
why not somehting like this
or maybe a try/catch block to handle cases when there might not be a 'Hills' or 'Mountains' object returned
Not sure about the re-usage of s in the above block as well, looks a tad suspect at first glance.
11/21/2006 (2:10 pm)
*Might be taking this out of context, just reply from the snippet of code I seen above and not the actual .cs file*T2DScroller s = _ground;
_groundSet.Object.AddItem(s);
s = (T2DScroller)TorqueObjectDatabase.Instance.FindObject("Hills");
_groundSet.Object.AddItem(s);
s = (T2DScroller)TorqueObjectDatabase.Instance.FindObject("Mountains");
_groundSet.Object.AddItem(s);are you 100% sure that you will get a 'Hills' object back? what happens if you dont in the above code?
why not somehting like this
T2DScroller s = _ground;
_groundSet.Object.AddItem(s);
s = TorqueObjectDatabase.Instance.FindObject("Hills") as T2DScroller;
if ( s != null )
_groundSet.Object.AddItem(s);
s = TorqueObjectDatabase.Instance.FindObject("Mountains") as T2DScroller;
if ( s != null )
_groundSet.Object.AddItem(s);or maybe a try/catch block to handle cases when there might not be a 'Hills' or 'Mountains' object returned
Not sure about the re-usage of s in the above block as well, looks a tad suspect at first glance.
#12
As an example of where we want to go, here is a snippet of xml that will load a car into the game with no external code. It works by attaching forces to link points and inputs controllers to link points. BTW, this won't quite work in beta 2 because the force component and process component don't deserialize in beta 2 (they do now).
We see more and more coding logic getting pushed out to the editor (and xml) due to components being able to do general tasks that you would before special case code (like mapping a gamepad stick to a link point rotation).
But that's getting ahead of things. Still more work to do before you can make a game in the editor out of pre-existing components, so take that as a taste of where we are going.
11/21/2006 (2:20 pm)
Ron & John, you are both right about the issues with TankBuster. This is a sample app we wrote back in May or so and it hasn't been touched much since except to keep up to date with engine changes. The issue with returning null objects that Ron points out is valid, and that even bit us at an event where peole wanted to export their own scenes into TankBuster. In fact, we see more and more moving into the level editor and level file and less and less in the actual code (ultimately exporting entire working games from xml into a LevelLoad app or something such as that).As an example of where we want to go, here is a snippet of xml that will load a car into the game with no external code. It works by attaching forces to link points and inputs controllers to link points. BTW, this won't quite work in beta 2 because the force component and process component don't deserialize in beta 2 (they do now).
<StaticSprite name="PlayerShip" type="GarageGames.Torque.T2D.T2DStaticSprite"> <Name>PlayerShip</Name> <ObjectType objTypeRef="player"/> <Layer>3</Layer> <Size> <X>3.25</X> <Y>6.5</Y> </Size> <CollisionsEnabled>true</CollisionsEnabled> <IsTemplate>true</IsTemplate> <Visible>true</Visible> <Material nameRef="PlayerShipMaterial"/> <Components> <T2DCollisionComponent type="GarageGames.Torque.T2D.T2DCollisionComponent" inPlace="true"> <CollidesWith> </CollidesWith> <Images> <T2DPolyImage> </T2DPolyImage> </Images> </T2DCollisionComponent> <T2DWorldLimitComponent type="GarageGames.Torque.T2D.T2DWorldLimitComponent"> <MoveLimitMin valueOf="ForceTest.MyGame.WorldEdgeMin"/> <MoveLimitMax valueOf="ForceTest.MyGame.WorldEdgeMax"/> <WorldLimitResolveCollision valueOf="GarageGames.Torque.T2D.T2DPhysicsComponent.RigidCollision"/> </T2DWorldLimitComponent> <T2DLinkPointComponent inPlace="true"> <LinkPoints> <LinkPoint> <Name>FrontWheels</Name> <Position> <X>0</X> <Y>-0.4615</Y> </Position> </LinkPoint> <LinkPoint> <Name>BackWheels</Name> <Position> <X>0</X> <Y>0.4615</Y> </Position> </LinkPoint> </LinkPoints> </T2DLinkPointComponent> <T2DForceComponent> <Forces> <T2DForce> <Name>thrust</Name> <ConstantDirection>0.0</ConstantDirection> <MinStrength>0.0</MinStrength> <MaxStrength>30.0</MaxStrength> <InitialStrength>1.0</InitialStrength> </T2DForce> <T2DDragForceBidirectional> <LinkName>FrontWheels</LinkName> <UseLinkDirection>true</UseLinkDirection> <MinStrength>7.0</MinStrength> <MaxStrength>7.0</MaxStrength> <AbsoluteDragCap>45.0</AbsoluteDragCap> <AbsoluteDragFactor>0.85</AbsoluteDragFactor> <RotationOffset>90.0</RotationOffset> </T2DDragForceBidirectional> <T2DDragForceBidirectional> <LinkName>BackWheels</LinkName> <UseLinkDirection>true</UseLinkDirection> <MinStrength>5.0</MinStrength> <MaxStrength>5.0</MaxStrength> <AbsoluteDragCap>45.0</AbsoluteDragCap> <AbsoluteDragFactor>0.85</AbsoluteDragFactor> <RotationOffset>90.0</RotationOffset> </T2DDragForceBidirectional> <T2DDragForce> <UseLinkDirection>false</UseLinkDirection> <MinStrength>0.1</MinStrength> <MaxStrength>0.1</MaxStrength> <MinimumRotationDrag>1.0</MinimumRotationDrag> <MaximumRotationDrag>1.0</MaximumRotationDrag> </T2DDragForce> </Forces> </T2DForceComponent> <T2DProcessComponent> <ProcessNodes> <T2DStickProcessor> <Cycle>false</Cycle> <InterfaceName>FrontWheels</InterfaceName> <Axis>0</Axis> <StickIndex>0</StickIndex> <MinValue>-20.0</MinValue> <MaxValue>20.0</MaxValue> <Mode>Direct</Mode> </T2DStickProcessor> <T2DStickProcessor> <Cycle>false</Cycle> <InterfaceName>thrust</InterfaceName> <Axis>1</Axis> <StickIndex>0</StickIndex> <MinValue>-1.0</MinValue> <MaxValue>1.0</MaxValue> <Mode>Direct</Mode> </T2DStickProcessor> </ProcessNodes> </T2DProcessComponent> </Components> <Mounts> <Mount> <Object nameRef="FrontWheels"/> <OwnedByMount>true</OwnedByMount> <SnapToMount>true</SnapToMount> <LinkPoint>FrontWheels</LinkPoint> <TrackMountRotation>true</TrackMountRotation> <InheritMountVisibility>true</InheritMountVisibility> </Mount> </Mounts> </StaticSprite> <StaticSprite name="FrontWheels" type="GarageGames.Torque.T2D.T2DStaticSprite"> <Name>FrontWheels</Name> <Size> <X>4</X> <Y>1.5</Y> </Size> <Layer>4</Layer> <Material nameRef="PlayerShipMaterial"/> <Visible>true</Visible> <IsTemplate>true</IsTemplate> </StaticSprite>
We see more and more coding logic getting pushed out to the editor (and xml) due to components being able to do general tasks that you would before special case code (like mapping a gamepad stick to a link point rotation).
But that's getting ahead of things. Still more work to do before you can make a game in the editor out of pre-existing components, so take that as a taste of where we are going.
#13
I agree that using xml files for configuration, objects etc is the way to go. I have been mucking around with GRAW (Ghost Recon: Advance Warfighter) and evrything is XML based, from the player config, weapons, objects, maps etc. It is rather simple to modify attributes of specific in game items for usage next time the map is loaded.
-Ron
11/21/2006 (2:47 pm)
I appologise for not locating the code snippet above and commenting on TankBuster example code. It is not (nor has been) my goal or intention to comment on code outside the actual engine source.I agree that using xml files for configuration, objects etc is the way to go. I have been mucking around with GRAW (Ghost Recon: Advance Warfighter) and evrything is XML based, from the player config, weapons, objects, maps etc. It is rather simple to modify attributes of specific in game items for usage next time the map is loaded.
-Ron
#14
Do you guys want me to do things such as create a new GUI control to test ease of adding things or just stick to trying to break the beta and find bugs?
11/21/2006 (2:53 pm)
Sounds good. I mostly was nitpicking at tankbuster due to the fact that that's what was originally asked. To see if the binary was enough. Over the next several days, I'll be able to hack more at the actual engine code and see what i can stir up. I did notice sound isn't hooked up yet, is that correct?Do you guys want me to do things such as create a new GUI control to test ease of adding things or just stick to trying to break the beta and find bugs?
#15
Jonathon, test what you can. Certainly feedback on the gui would be good. But trying to find bugs and usability issues elsewhere would also be great.
11/21/2006 (3:10 pm)
Ron, don't sweat it.Jonathon, test what you can. Certainly feedback on the gui would be good. But trying to find bugs and usability issues elsewhere would also be great.
#16
*cackles*
11/21/2006 (3:32 pm)
My favorite comment thus far in the source:Quote:// not a fast operation
*cackles*
#17
- We won't have xml guis for 1.0
- Tank Buster UI has been refactored post beta 2 to have separate class files for each gui (its not all in Game anymore)
- We will refactor Tank Buster to behave gracefully if objects are missing
11/21/2006 (4:41 pm)
Whoa, lotta material in this thread. Some comments:- We won't have xml guis for 1.0
- Tank Buster UI has been refactored post beta 2 to have separate class files for each gui (its not all in Game anymore)
- We will refactor Tank Buster to behave gracefully if objects are missing
#18
Anyways, I'm responsible for the GUI port, so blame me!
Some things I would like to note:
First, the Beta2 that shipped did indeed have the Game GUI all in one class, as a partial class of Game actually. Bad design I know, and I even said as much when I checked it in. Let me give a little history and shed some light on this. The TorqueX GUI is essentially a port of the TGE GUI. We had intended for a game's ui to be described entirely in an XML file. Unfortunately this isn't completely possible, as we found out because there is no way for a user, for instance, in the xml to tell a button to execute a particular function. Now there are ways of doing this on the PC using reflection, but the Compact Framework on the Xbox360 does not contain the necessary functions to do this (at least not in a way that is performant). This means that in order to bind functions to buttons, etc, you would have to define the ui in the xml and then in code write a class that looked up your buttons and set the bindings manually. So we decided to hold off on GUI xml loading/unloading for the time being. Currently the GUI should deserialize to an xml file (I havn't tested this but I know John did some work in this area), but we don't have any loading functionality in place at this time.
For TankBuster I have already refactored the way the Game GUI is done, removing the partial class aspect. While I was working on the refactor I realized there were two aspects to the TGE GUI that made it easier to work with that we finally managed to get right for TorqueX. Basically, the TGE GUI does two things: because it is done through the scripting the GUI objects are automatically created, aside from the exec(file); calls. Second, you could reference GUI controls by name if needed. This makes working with the GUI really easy. I don't have to call a function to create a shell screen, like the main menu or options menus for instance. I can also reference important components of those shells by name, like a button that saves the settings in an option menu.
With these things in mind, I've added two things to the core GUI. First, you can now tell the Canvas to set the content control by name instead of by reference. Second, by implementing a 'dummy' interface in your custom Game GUI you can hint to TorqueX to load that GUI automatically for you. No more need for 'CreateMainMenu()', 'CreatePlayScreen()' functions.
So how does this work? Below is a simple example.
In SimpleScreen.cs
In Game.cs:
This was John Q.'s idea and I think it works wonderfully, so thank him. Now you can create as many GUI's for your game as you like, in as many files as you like, under any namespace you like and to create all of the GUI's all you have to do is call GUIUtil.InitGUIScreens.
11/21/2006 (4:44 pm)
So I just found my way to this forum. It was showing up as blank posts for me!Anyways, I'm responsible for the GUI port, so blame me!
Some things I would like to note:
First, the Beta2 that shipped did indeed have the Game GUI all in one class, as a partial class of Game actually. Bad design I know, and I even said as much when I checked it in. Let me give a little history and shed some light on this. The TorqueX GUI is essentially a port of the TGE GUI. We had intended for a game's ui to be described entirely in an XML file. Unfortunately this isn't completely possible, as we found out because there is no way for a user, for instance, in the xml to tell a button to execute a particular function. Now there are ways of doing this on the PC using reflection, but the Compact Framework on the Xbox360 does not contain the necessary functions to do this (at least not in a way that is performant). This means that in order to bind functions to buttons, etc, you would have to define the ui in the xml and then in code write a class that looked up your buttons and set the bindings manually. So we decided to hold off on GUI xml loading/unloading for the time being. Currently the GUI should deserialize to an xml file (I havn't tested this but I know John did some work in this area), but we don't have any loading functionality in place at this time.
For TankBuster I have already refactored the way the Game GUI is done, removing the partial class aspect. While I was working on the refactor I realized there were two aspects to the TGE GUI that made it easier to work with that we finally managed to get right for TorqueX. Basically, the TGE GUI does two things: because it is done through the scripting the GUI objects are automatically created, aside from the exec(file); calls. Second, you could reference GUI controls by name if needed. This makes working with the GUI really easy. I don't have to call a function to create a shell screen, like the main menu or options menus for instance. I can also reference important components of those shells by name, like a button that saves the settings in an option menu.
With these things in mind, I've added two things to the core GUI. First, you can now tell the Canvas to set the content control by name instead of by reference. Second, by implementing a 'dummy' interface in your custom Game GUI you can hint to TorqueX to load that GUI automatically for you. No more need for 'CreateMainMenu()', 'CreatePlayScreen()' functions.
So how does this work? Below is a simple example.
In SimpleScreen.cs
namespace GarageGames.Torque.Examples.SimpleGame.UI
{
public class SimpleScreen : GUIControl, IGUIScreen
{
//======================================================
#region Constructors
public SimpleScreen()
{
// create the Style for this simple example
GUIStyle style = new GUIStyle();
style.IsOpaque = true;
style.FillColor = Color.Black;
// name this content control something
Name = "MySimpleScreen";
// describe how it looks
Style = style;
Size = new SizeF(800, 600);
}
#endregion
}
}In Game.cs:
namespace GarageGames.Torque.Examples.SimpleGames
{
public class MyGame
{
public void SomeFunction()
{
// searches the specified namespace for any class implementing IGUIScreen and
// creates an instance of that class, therego automatically creating the game ui
GUIUtil.InitGUIScreens("GarageGames.Torque.Examples.SimpleGame.UI");
// look up the control by name and set it as the content
GUICanvas.Instance.SetContentControl("MySimpleScreen");
}
}
}This was John Q.'s idea and I think it works wonderfully, so thank him. Now you can create as many GUI's for your game as you like, in as many files as you like, under any namespace you like and to create all of the GUI's all you have to do is call GUIUtil.InitGUIScreens.
#19
We can agree to disagree. I don't like your method because it assumes that if I'm on a PC I am not using a gamepad, but what if I am? This is why, in TankBuster at least, I am binding for both the keyboard and gamepad. It gives me the option of playing the game anyway I like.
11/21/2006 (4:59 pm)
Quote:
Jonathon Stevens wrote:
In the above snippet, it is binding the gamepad's back button to the actionmap. Then, below it (not posted) you bind a keyboard command as well. This is what I meant, bind one or the other based on if they are using the xbox or not. Just need to do a:
#if XBOX Bind xbox controls here #else Bind keyboard controls #endif
We can agree to disagree. I don't like your method because it assumes that if I'm on a PC I am not using a gamepad, but what if I am? This is why, in TankBuster at least, I am binding for both the keyboard and gamepad. It gives me the option of playing the game anyway I like.
#20
For example, the following
could be replaced with
This is more force of habit than anything else, c++ developers are used to the ?: way of doing things, might be worth while to read up on nullable types etc.
-Ron
11/21/2006 (6:01 pm)
Treading through the bowels of the engine source I notice a fair amount of ?: being used, especialy for checking null types. With the release of .NET 2.0 MS implemented nullable types and a new operator (??) to handle such cases.For example, the following
public static object CreateObject(Type type)
{
object obj = FindPooledObject(type);
return obj != null ? obj : Construct(type);
}could be replaced with
public static object CreateObject(Type type)
{
object obj = FindPooledObject(type);
return obj ?? Construct(type);
}This is more force of habit than anything else, c++ developers are used to the ?: way of doing things, might be worth while to read up on nullable types etc.
-Ron
Torque Owner Clark Fagot