This project is read-only.

Patches!

Dec 19, 2007 at 4:02 AM
I'll review and check in Sturm's code. Shaw hopefully you get your changes in soon. We have a lot of patches floating around and I'll like to get one trunk build with all of our changes (reviewed and cleaned up of course).

Taliesen, I'll try and get yours reviewed and in as well, but I guarantee two patches in one night (plus review).
Dec 19, 2007 at 5:34 AM
Edited Dec 19, 2007 at 5:35 AM
I believe properties should be directly above their respective variables. For example when I opened BaseEntity and save the private QSGame game variable I thought that you'd changed entity structure so that no derived classes could access game. In actuality we have the variable and accessor in two different parts of the class.

Also in some of your classes you have the .this prefix in your accessors, and in other classes they're missing.

InputHandler.cs:
  • Please XML comment all member variables.
  • Do we need the 'this' prefex in front of variables? A variable is a member and not local.

KeyboardHandler.cs:
  • Lists need comments.

IMessage.cs:
  • File needs header (I will add this)

Message.cs:
  • XML Comments missing on some members.

MessageType.cs:
  • If every single message for the engine has to be centralized here, it will become very crowded very quickly. Although we're not using regions throughout the engine, I recommend they at least be used here to organize groups of messages.

ObjectPool.cs:
  • Missing XML comments

QSGame.cs:
  • Missing XML comments
  • Again, we probably don't need .this prefix for functions. (Although I do agree it is okay to use the prefix in front of base class functions, like using this.Exit() because it isn't in QSGame, but inside of Xna.Game)

DebugMessageEntity.cs:
  • As part of a demo game is doesn't need comments as extensive as the core engine, but it should have some comments so that people downloading it know what is going on within it.

That's about it, looks good Sturm. I will apply the patch and submit soon.
Dec 19, 2007 at 5:38 AM
Also, the folder 'Configutation' is spelled incorrectly, should be 'Configuration', however cpc will not let me change it without errors.
Dec 19, 2007 at 5:46 AM
Patch applied, and commited.
Dec 19, 2007 at 10:08 AM

LordIkon wrote:
I believe properties should be directly above their respective variables. For example when I opened BaseEntity and save the private QSGame game variable I thought that you'd changed entity structure so that no derived classes could access game. In actuality we have the variable and accessor in two different parts of the class.

Also in some of your classes you have the .this prefix in your accessors, and in other classes they're missing.


The problem with not having the fields at the top of the type is that VS can have problems identifying them if you have complilation errors throuhtout the file. This will then prevent you from showing the fields in the intellisense, the safe way is to have them at the top of the type. This also has the added benefit of easyli find all fields when doing code review.

If they are missing this prefix it's an error. I'll fix them as I find them (Have to fixup the VS template for that).


LordIkon wrote:
InputHandler.cs:
  • Please XML comment all member variables.


I certainly will never comment varaibles (Unless there is a very good reason). Though I can add xml comments for fields.


LordIkon wrote:
MessageType.cs:
  • If every single message for the engine has to be centralized here, it will become very crowded very quickly. Although we're not using regions throughout the engine, I recommend they at least be used here to organize groups of messages.


That's fine by me, we could also make the class partial an have each section for it's specific group.


LordIkon wrote:
  • Again, we probably don't need .this prefix for functions. (Although I do agree it is okay to use the prefix in front of base class functions, like using this.Exit() because it isn't in QSGame, but inside of Xna.Game)
...
  • Do we need the 'this' prefex in front of variables? A variable is a member and not local.


Variables wont have this prefix, as this would generate a compile error, fields/methods/etc should have this. There is a very good reason to provide this prefix, as not doing so can create some very obscure bugs consider:

public class Test
{
    private int position = 0;
 
    ... A lot of methods doing stuff
 
    public void DoTranslate(int position, int rotation)
    {
        /*this.*/position = /*this.*/position * rotation;
    }
} 
This will give very different results depending on where this is placed. This is a very simple example and easy to fix, not this is not alwways the case. Also it's not always that aparent and the bug might not show initially.

LordIkon wrote:
DebugMessageEntity.cs:
  • As part of a demo game is doesn't need comments as extensive as the core engine, but it should have some comments so that people downloading it know what is going on within it.


I will add some comments to that as well, actually I do not see a reason for not having the same standard of comments here. It is really more important as this is the show case for the engine, kinda our developer documentation.
Dec 19, 2007 at 2:02 PM
Edited Dec 19, 2007 at 2:07 PM

Sturm wrote:

I certainly will never comment varaibles (Unless there is a very good reason). Though I can add xml comments for fields.


Member variables are fields. Microsoft thought it'd be clever to make up a new word for it. Local variables do not need XML comments.




LordIkon wrote:

Again, we probably don't need .this prefix for functions. (Although I do agree it is okay to use the prefix in front of base class functions, like using this.Exit() because it isn't in QSGame, but inside of Xna.Game)
...

Do we need the 'this' prefex in front of variables? A variable is a member and not local.


Variables wont have this prefix, as this would generate a compile error, fields/methods/etc should have this. There is a very good reason to provide this prefix, as not doing so can create some very obscure bugs consider:
public class Test
{
    private int position = 0;
 
    ... A lot of methods doing stuff
 
    public void DoTranslate(int position, int rotation)
    {
        /*this.*/position = /*this.*/position * rotation;
    }
} 
This will give very different results depending on where this is placed. This is a very simple example and easy to fix, not this is not alwways the case. Also it's not always that aparent and the bug might not show initially.



Again, member variables are the same as fields. But I still do not agree that a function that is a member of that class, and not a base class, should have the .this prefix. How ugly would the code be if every single member (field) and function that was part of that class had the .this prefix. We wan't the code to be readable by others, and it is getting ugly as it is. In 4 years I've never seen a program that used .this in front of everything other than local variables.

Also, in your example above I believe the .this is actually the problem. Most programmers would assume that if you passed in position that you might actually use it. That doesn't mean that .this is bad, it just likely shouldn't have been there.

And you say placement in the class matters. I'm not sure what you mean, C# doesn't care about the order of things, unless for intellisense in some situations. You could have had the position field below that function and it would've worked just as well.
Dec 19, 2007 at 4:09 PM
{quote}
LordIkon wrote:
Member variables are fields. Microsoft thought it'd be clever to make up a new word for it. Local variables do not need XML comments.
{quote}

I think that you just insulted a whole java community. I've been using the term field long before there was anything called C#, and in languages which didn't have anything to do with MS. The terms Class field, instance variable, class variable, field, property are all words that I've have been using to describe it. I find fields the most suited, specially now that it's possible to differentiate between fields and properties.


LordIkon wrote:
Again, member variables are the same as fields. But I still do not agree that a function that is a member of that class, and not a base class, should have the .this prefix. How ugly would the code be if every single member (field) and function that was part of that class had the .this prefix. We wan't the code to be readable by others, and it is getting ugly as it is. In 4 years I've never seen a program that used .this in front of everything other than local variables.

Also, in your example above I believe the .this is actually the problem. Most programmers would assume that if you passed in position that you might actually use it. That doesn't mean that .this is bad, it just likely shouldn't have been there.


The example was just a very simple scenario where it can go wrong. Of cause it's very easy to see that there is something wrong, this by all means is not always the case.


LordIkon wrote:
And you say placement in the class matters. I'm not sure what you mean, C# doesn't care about the order of things, unless for intellisense in some situations. You could have had the position field below that function and it would've worked just as well.


VS can be the source here as it might not be able to resolve the fields following. But that's true for other things as well, and only something you encounter if you have multiple compile errors in your code (I often tend to have that as I do mulriple implementations at the same time. But VS set aside it's also very handy when doing code reviews. I do not use VS for reviews but diff tools/text editor and here having all fields centrally makes it a lot easier to spot them. Using this prefix helps during code review to identify the scope of the item, also without VS it can be difficult to dertermine the scope of an item such as if it's instance or static.
Dec 19, 2007 at 5:18 PM

I think that you just insulted a whole java community.


That's ok, I'm a C++ programmer. :)

If this were J# I would concede. Even though C# is very much like Java, the fact is that Java is very much based off of C++.
Dec 19, 2007 at 6:17 PM
I like the newest update, but my memory usage seems to be high, and I can detect a few dropped frames. I take it these are known about?
Dec 19, 2007 at 7:23 PM
Memory usage is misleading, I originally thought so as well. My memory usage hovers around 250mb, however, that is because Windows is holding on to the memory. If you run the CLR you'll see that memory usage hovers around 100mb, which isn't bad for a 1024x1024 terrain with normals (which is taking up around 80mb of the 100mb I believe).

I didn't know about dropped frames though.
Dec 19, 2007 at 7:48 PM
My memory is about 320mb, but I havent run the clr yet. Ill check out the dropped frames more later. One thing I dont like, is the fardistance plane. The terrain appears way to close for my tastes, but that only a fault of the sample not the engine.
Dec 19, 2007 at 7:53 PM
Personally, I do see a large chunk of my graphics code as "ugly" after having placed "this." in front of nearly everything. I much prefer "m_" variable prefixes, as this also resolves the property naming issue where you can easily miss a capilization somewhere and cause an infinite recursion, especially when you place properties before fields and Intellisense will only see the properly, not the field. Like the following:

public int Count
{
    get { return this.Count; }
    set { this.Count = value; }
}
 
private int count;

Notice that the get/set methods actually refer to the property, so any call to them will result in infinite recursion. Is this just a programmer typo? Yes and no. Yes, because the programmer should catch this type of error. No, because the programmer physically typed "this.count" and Intellisense in all it's wisdom replaced count with Count when the semi-colon key was pressed. Also, the only difference between property and field here is the case of the first-letter. That's not a lot of difference. A prefix solves both of these problems:

public int Count
{
    get { return this.m_count; }
    set { this.m_count = value; }
}
 
private int m_count;

When typing the property, Intellisense will not find mcount, but it won't replace it either since it can't match another field, so the programmer is freed from Intellisense errors. It also makes the code a little cleaner by writing "mcount" instead of "this.count".

Regarding this:

public void DoTranslate(int position, int rotation)
{
    /*this.*/position = /*this.*/position * rotation;
}

I would consider this bad form. I always try to prevent name collision between method parameters and class fields. I would either abbreviate the parameters, or change them to "newPosition" and "newRotation" or something like that.

These are just my personal feelings, however, and do not in any way reflect the quality or performance of the compiled code.
Dec 19, 2007 at 8:25 PM
I agree for the most part. I'm actually ok with neither the 'm_' prefix or 'this.' prefix, because intellisense in C# will always tell you if it is a member (it does say member, not field), or local.

However, I agree having this in thousands of places is starting to drive me crazy. I'm usually not a big fan of hungarian notation, but I would be willing to compromise and do a 'm' prefix, without the underscore. The 'm' would be lowercase, and the first letter of the word itself would be uppercase. It would pretty much be PascalCase with an 'm' prefix. This prefix would be required on all member variables (not local), and I would also like to require that no other members have prefixes.

As such:

public int Count
{
    get { return this.mCount; }
    set { this.mCount = value; }
}
 
private int mCount;

Just my suggestion. I do think we should come to some agreement on this soon, before we have more code.
Dec 19, 2007 at 8:34 PM
Having "m" as a prefix solves the Intellisense error issue, but you're still left with only one change between properties and fields. Just like with the original naming, you miss the m and now you're referring to the property.
Dec 19, 2007 at 8:35 PM
The naming isn't a replacement for Intellisense, it's more for quickly scanning through large chunks of code. You're not going to hover over each variable to see what it is. Seeing "m" lets you know exactly what it is. I don't advocate Hungarian notation, just the "m" prefix on member variables.
Dec 19, 2007 at 9:41 PM

LordIkon wrote:
The 'm' would be lowercase, and the first letter of the word itself would be uppercase. It would pretty much be PascalCase with an 'm' prefix. This prefix would be required on all member variables (not local), and I would also like to require that no other members have prefixes.


Hehe, if you used the term field for class variabled you wouldn't have to differ between class/local variables :)

Prefixing with m just becomes wierd (and I actually don't know many frameworks who uses this term). A few examples mMaximum/mIsClosed also how would you handle statics, prefixing with s? Then how about static consts or consts? Just not having any prefixes and using proven design norms (Phillips/IDesing/MSDN) I think proves so much more value, instead of inventing our own.
Dec 20, 2007 at 12:58 AM
I was thinking members have 'm', no matter what they are, and everything else is unprefixed. My personal preference is that PascalCase denotes members. Worked pretty good in the template.
Dec 20, 2007 at 2:41 AM
I'm still not convinced that it's a good idea going against proved coding guidelines and going against the general .net-ness (fields are camelCase and private, all other members are PascalCase). But if you think that there are more people used to the Template style than the .net style, I guess that we should follow that.
Dec 20, 2007 at 2:00 PM
In my opinion, every company, every person, has their own style. Some people choose to make their style to that of a set coding guidelines. I'm ok with a set of guidelines if it is beneficial. If following a standard means that clean looking, understandable, and generally safe code suddenly looks more confusing, or just plain ugly in some cases. In some cases I've see that by adding .this prefix makes lines of code long enough to become two lines. I try and avoid two line statements when possible just because I think it makes code less clean, although making code take multiple lines is sometimes necessary.
Dec 20, 2007 at 2:46 PM
I'm not going to argue this any more. I'm will follow the guidelines set out in the Code Guidelines.
Dec 20, 2007 at 3:19 PM
Fair enough.