New Patch (438)

Nov 1, 2007 at 11:32 PM
Edited Nov 1, 2007 at 11:36 PM
I would have checked it in, but Shaw have some of the files locked for checkout so I submitted a patch, which Shaw can commit once he has merged his changes into it.

Do not apply this patch to an existing source tree where you have changes as they might be deleted. Create a new source tree and apply this patch to it, then move the changes over to the new tree.

I will create a detailed post with all (or at least most) of the details for this, but here are some to make your teeth wet:
  • Can build both Template and QuickStart from the root
  • Contains solutions at the root for both code
  • Clear cut where code is placed
  • Ability to set up alias commands
  • Ability to build help files (requires Sandcastle and Sandcastle Help File Builder - both have to be in Program Files)
  • Ability to view those help files (just type helpview once they are build)
Nov 2, 2007 at 12:07 AM
Interesting, I did a shared checkout so it shouldn't block changes. I'm working on the physics code, so I'm not too worried about merging.

I'll take a look at it.
Nov 2, 2007 at 12:22 AM
I undid my changes and tried to apply the patch, but now you have several locks on the source tree. :)

My locks should be released, so you should be able to apply the patch now.
Nov 2, 2007 at 12:53 AM
New source tree structure is now uploaded. If you dl it into a folder structure containing a space you will not be able to use the command line, using visual studio works fine.
Nov 2, 2007 at 9:22 AM
It might be just me but im a little confused as to what state both parts of the project are in now and what is actually in the source code download.
No sooner was i updating my new structure 2 new patches came in with conflicting instructions :S . like i say it might be just me who needs the clarification, being a bit special, but im fairly confident a new comer would probably miss all the discussions before downloading thesource package.
Nov 2, 2007 at 10:55 AM
Edited Nov 2, 2007 at 10:56 AM
Sorry for any confusion.

All patches have been applied so there is not need to apply these if you get the newest source. The release and the source are not the same as the release only contains a part of the source, specifically the framework/template code.

My advice to anyone, current and new developers is to decide if you want to use the new build environment or just use visual studio and do checkins as you've always done. The reason there is a difference is that the build environment introduced into the new bits is console based. Console base applications do have a challenge using paths containing spaces (or rather batch files do).

If you just want to use Visual Studio and submit patches/code the ways you've done uptil now there are no special considerations, just get the source into any folder (i.e. c:\My CodePlex\QuickStartEngine). If you want to use the new build environment (and moving forward developers will be required to) you will have to get the source into a folder that does not contain any spaces (i.e. c:\CodePlex\QuickStartEngine) once you've done that you should run the setup.cmd file in the root folder, close the console (Just press enter when setup finishes) and launch the build environment from the desktop shortcut.

The build environment provides a few services which makes life checking in a bit easier. There will be an Discussion on this later.

After you've got the new source there are two solutions at the root: QuickStart.sln which contains the source for the new engine and Template.sln which contains the source for the current template engine, both of these can be run regardsless of folder structure and directly from explorer.

I hope this helps.
Nov 2, 2007 at 12:11 PM
Yeah that's cleared things up, thanks. I think ill go with the command line system and begin to get familiar with the new framework layout. Is anyone currently working on porting code to the new structure? is there anythign specific that you would like me to help with ? im taking a look at the water feature LordIkon asked for, not promising anything but its certainly keeping me interested :)
Nov 2, 2007 at 12:42 PM
There is currently no tasks assigned to port any of the current template code, so if you are going to do something like the new water you should do that in the Template code.

If you want to help porting then create a new Issue and do the port, one idea for porting could be terrain, then water and then implement the new water code. If you want to do the port, create a new Discussion for the area ported as well and give your comments feedback on it there. Though be aware that Shaw is currently doing some more reworking which might affect you, but create the Discussion and take issues up there
Nov 2, 2007 at 12:55 PM
Ok sounds good :)
Nov 3, 2007 at 6:42 PM
I had a chance to take a closer look at the changes, and I have some questions:

  1. Why is every field in EngineEvent now private with public properties? Its just a collection of values, I see no reason to make these all properties.
  2. Is there a reason for changing all of the member variables in base classes like SceneNode to private instead of protected? Derived classes should have full access to these variables, and not have to go through properties.
Nov 3, 2007 at 8:31 PM
There is no reason not to. The real reason for doing this is to have a certain contract.
  • You can always trust the value of a field (unless you allow it to).
  • All validation is done when assigning the value to a field, hence in the property.
  • Don't trurst derived classes, public access is always dangerous.

Also the programming model for the OM is now unified users only have access using properties and methods.
Coordinator
Nov 3, 2007 at 8:49 PM
I'm a firm believer there should only be accessor classes when required. There should only be a 'set' property is an outside class absolutely needs write access, and only a 'get' if an outside class absolutely needs read access. Finally, there shouldn't be an accessor at all if the variable is private or protected only to its class or derived class, or in the case where you want the accessor to do some work before returning something.

If we create an accessor with a set and a get then we might as well have made the variable public.
Nov 3, 2007 at 8:54 PM
Derived class access is not public access. I'm not saying make everything public, I'm saying make the base class members protected. If a derived class can't trust its own methods, then there's a much larger problem here. A part of the contract that derived classes must honor is consistent state of the member variables. It is not the responsibility of the base class to verify everything that derived classes do.


Nov 3, 2007 at 11:03 PM
Well I'm not going to fight this for any existing code, if you wan't public accessible fields that then the design decission. Though I'm prob going to checkin code which won't honor this as I've been using that design pattern for quite some years.

I've come to the realization that it provides a clean uniform consistent programming model. This is important for those who are extending the framework. A good example of this pattern is the .net framework, as everything is exposed as properties not public/protected fields.
Nov 3, 2007 at 11:34 PM
An example of where this makes sense is the Visible property on controls (Well controls in the gui project (Haven't checked it in yet).
public bool Visible 
{
    get
    {
        if (this.visible == false || (this.Parent != null && this.Parent.Visible == false)
        {
            return false;
        }
 
        return true;
    }
    set 
    {
        this.visible = value;
    }
}

Or in another project I worked on the screen graphs had a reference to the root node which looked something like this:
public SceneNode Root 
{
    get
    {
        if (this.Parent != null)
        {
            return this.Parent.Root;
        }
 
        return this.root;
    }
    internal set 
    {
        if (this.Parent != null)
        {
            throw new ParentSetException();
        }
 
        this.root = value;
    }
}
Coordinator
Nov 4, 2007 at 12:37 AM
Both of the examples given above were probably fine, as both of those cases have some work being done within them, and the 'set' in the second example is somewhat limited (to a namespace?).

What wouldn't make sense is this:

public bool isVisible
{
get { return IsVisible; }
set { IsVisible = value; }
}
private bool IsVisible;

In this cause there is no need for an accessor and IsVisible may as well have been public in the first place. If IsVisible was truly private, or even protected, then it shouldn't have a set accessor like shown in my example either, or it isn't really private. However, in your accessors there is logic within them, which gives a reason to have them. For examples like you gave I would say you're fine. The biggest thing here is not to break O.O. by exposing things that shouldn't be seen by other classes, and only exposing what absolutely needs to be exposed. Exposing too much can create hassles during debug.
Nov 4, 2007 at 1:37 AM
It's all a question about coherence throughout the OM. I'm used to an all property based model, because I'm used to .net and the model. Also in the above example I wouldn't expose IsVisible setter before the need to do so were there. During the rework of the common project I found some fields which were exposed more than required by the field, I.e. made protected when only a protected getter was required.

The main thing when using properties is to only expose the functionallity when needed, as you mentioned, an option you do not have with public/protected fields. It's also very difficult to go back and change the behavior on released code.

I think you are mismatching the guideline in the property/field naming, Properties/Methods should use PascalCase where as fields/variables should use camelCase.

The property with a internal setter the setter is restricted to the assembly as it's internal.
Nov 4, 2007 at 2:39 AM
Edited Nov 4, 2007 at 3:02 AM
I'm not opposed to properties in general, just in the two cases I mentioned above. First, I see no reason why a derived class should have to access it's base class' member variables through properties. Second, a class that only contains data (no methods or processing logic) should just have public members.

In the majority of cases, private members with properties is just fine, its just these specific cases that I disagree with.

Don't forget we also have performance constraints. There will be an awful lot of wasteful data copying if a derived class needs to copy a matrix twice just to edit one element of the matrix in the base class.
Nov 4, 2007 at 3:17 AM
And just to be clear, I am not advocating making everything public. To the contrary, the vast majority of member variables should be private/protected. I only advocate public member variables in a few conditions:

  1. The variable should truly be arbitrarily changed by anyone.
  2. Performance considerations, i.e. being able to access value-type class data by ref without incurring the cost of unnecessarily copying the data potentially twice.

Coordinator
Nov 4, 2007 at 5:53 AM
Edited Nov 4, 2007 at 5:55 AM
If a variable is given a 'get' accessor that performs no special function, and simply returns the variable, and the accessor itself is public, and the 'get' is public, then the variable itself should just be made public or we're simply wasting time to access something, just as Shaw said.

It is basic O.O. standard to hide access to class data members that shouldn't be accessed from other places.

From a programmers point of view, having to go through an accessor to access something from a base class seems like poor design. Think back to when you first learned Object Oriented programming, at one point in time I'm sure it was put into context for you. Let us say I have a base class that represents a Car, and this class (Car) has an Engine (protected Engine). Then I have a derived class Ferrari. The Ferrari isa Car is it not? Should the Ferrari have to request to some other object if it can access its Engine? Not unless there is a good reason. However, if an accessor is having to modify data for a derived class to use then I say there is something likely wrong with the design. We should only be using accessors when required, in your examples above for instance, there was a big O from 2 to (5 * number of Parents) simply to return true or false, now in this example that may be acceptable because it may be required. But it is acceptable only if it were required. If there were a more efficient way, like setting the state of 'isVisible' to false whenever the parent was not visible then you would only have to set that once, and then access to 'isVisible' would no longer require an accessor. Whenever a parent became invisible it could simply tell all of the children to become invisible, because in your example when a parent isn't visible then the accessor will return false.

If we could find out more about how they are being used I believe we could understand the logic more. Your code sample isn't enough for me to get the big picture, I am simply making suggestions off of a small piece of code I've seen. My point is just that if there is any way around having to do 4 conditional comparisons (1 which is recursive) to access a variable, then it should be taken. If not then so be it.

Nov 4, 2007 at 1:15 PM
I'm not forcing you to do it. I'm just saying that following a design paradim is a good thing, every .net developer will be used to it, you access only properties or members, unless it's your own code in which case you access fields as well.

But to give some feedback on your comments:

shawmishrak Wrote:
First, I see no reason why a derived class should have to access it's base class' member variables through properties.

LordIkon Wrote:
From a programmers point of view, having to go through an accessor to access something from a base class seems like poor design.

As I'm saying it's a design pattern (PIMPL), it simply states that you should keep your internals to yourself. It's used widely through the .net framework, and it does provide you with a clear consistent public OM.


shawmishrak Wrote:
Second, a class that only contains data (no methods or processing logic) should just have public members.

Sounds much more like a data structure, why would you create a class if there isn't any functionallity connected with the data? If you implement a struct you can just expose the fields, as there isn't any behavior assiciated with them. This is done widely as well, just have a look at the Vector structs.


LordIkon Wrote:
Think back to when you first learned Object Oriented programming, at one point in time I'm sure it was put into context for you ...

Well though it's been some time since I've learned OO, I do recal those days, I hated writing all those get/set methods in C++, as back then there wasn't anything called properties or attributed programming. So the code contained a lot of setter and getter methods. So moving to C# and loosing all those ugly methods did come as a nice price. Later when VS supported the language and snippets writing fields and properties are simply a joy.


LordIkon Wrote:
Let us say I have a base class that represents a Car, and this class (Car) has an Engine (protected Engine).

Your analogy with car and engine is just as flawed as it was back then. Imagine I would like to change the engine inside a car (and many people want bigger engines in their cars, while not having to buy a new car), I would have to build a new car in order to do that. Also you will want to expose a lot of properties of the engine, like Volume/HP/MaxRev/etc so it should be public. But you most def want to limit the set activity as only certain engines will fit inside the car. Most properties you actually want to get even outside your class hirachy. Also those inheriting from Ferrari will need to abide by the same rules as Ferrari does, making engine protected makes it possible for child classes to circumvent any validation you've set on the base class.


LordIkon Wrote:
We should only be using accessors when required, in your examples above for instance, there was a big O from 2 to (5 * number of Parents) simply to return true or false

Yes it gets more complicated, it is. Also you can not solve it like that, because when you reenable the parent the child should revert to the state it had before. Of cause you could add another property ParentVisible, but I really think this makes a unuseable OM as everyone would now have to ask 2 properties. Also I'm not too big a fan of Big O, while it does provide valueable information for input effect and explosion, used by itself it's useless because there is no relation to the problem solved by the algorithm.

So the final question is more if you wan't to follow a design pattern that all .net developers are used to, or you want to go in a different direction, and have our own pattern.
Coordinator
Nov 4, 2007 at 5:57 PM
Edited Nov 4, 2007 at 5:59 PM
I don't believe the engine idea is flawed. You are saying that if we had 50 cars with different engines that each one would have to have an entire engine uniquely designed for it. When in reality you would likely much rather have an engine class which could be derived from. Every engine (not motor) has an intake, and moving parts, and uses a source of fuel (be it water, or gasoline, or ethonol). From that engine you would have either a piston engine or a rotary engine derived from that. A piston engine would have bore and piston size variables, a type of cam, compression ratio variable, etc. The ferrari would derive from a piston engine and could define its own values of each.

Now there are still some very good uses for accessors however. For instance you could use the accessor to set limitations for all engines. For instance piston size must always be positive, so you could have the 'set' accessor check the incoming value for errors.

public float pistonSize
{
protected set
{
if ( value <= 0 )
return .....error message here....
else
PistonSize = value;
}
}
protected float PistonSize;

But I see no reason that PistonSize should be private and require a 'get' accessor for the FerrariEngine to access PistonEngine.

I should mention that while I am interested in .net practices to an extent, I am also interested in efficiency, less code, and easy a very easy to understand engine.
Nov 4, 2007 at 6:14 PM
Edited Nov 4, 2007 at 6:17 PM
Again, my main argument is in the case of derived classes, and it's mainly a performance argument. Everytime you want to read, perform an action on, then write-back a member variable in the base class, you incur the penalty of copying the data twice. This isn't an issue with simple data types like ints, floats, etc., but they you start talking about vectors, matrices, and even larger structures this can easily add up to a significant performance hit.

For example, I created a very simple benchmark that just multiplies a random translation matrix by a rotation matrix (carefully chosen so that floating-point exceptions never occur). The translation matrix is a member variable of a class, and the rotation matrix is just created in the test code. This multiplication is carried out 10 million times in a loop. In one case, the matrix member variable is accessed through a simple get property, copied to a local variable, and passed by ref to the Matrix.Multiply() method. In the second case the matrix is directly accessed and passed by ref with no copying. What's the effect? (Results are in milliseconds)

            Property    Direct
    PC      1186        693
    XBox    5359        4222

Unless there's a property by reference syntax I'm missing somewhere, I'm just not comfortable with all of this extra copying with translates into an almost 50% drop in performance on the desktop platform just for the sake of following the .NET design methodology. I realize the real performance drop would be considerably less across the entire project because a lot of the code isn't so dependent on fast memory access, but in the performance-critical sections of code this performance drop is a possibility.

Now I agree that client code (i.e. users of classes) will access data through properties or methods (maybe provide a getter with an 'out' parameter for larger value-types). There are very few cases were public access is needed (the internals of my physics code being one of these cases, but that won't be exposed to clients).

Theoretically, I understand why properties might be used everywhere and the benefits they could provide. In a perfect world, I can see how this level of validation is good. Practically, however, it's not really feasible for performance-critical code. If you're writing a WinForms application and you pass around maybe a few kilobytes of data per second, it's perfectly acceptable. But when you rely on direct memory access to a large chunk of data without causing a cache miss nearly every access due to copying, it's not so feasible.
Nov 4, 2007 at 7:00 PM
LordIkon, you actually just prove my point, why would you allow a sub classes to create pistons with negative value, should the validation always be triggered?

Shaw I totally understand the performance implication of using the property overhead. I do believe that that will be improved for version 2. But not into any degree which will make it attractive. Though I think that the code shouldn't ever do 10m assignment to an entity at any time, since updating it more than updates/sec doesn't make sense. The physics engine will have its own internal data structures and update those, these would be passed by ref in order to optimize for performance. If updating entities is the major bottleneck in our engine I would use much more time in investigating how to limit the updates, rather than trying to improve the way.
Nov 4, 2007 at 7:19 PM
Actually, the property overhead has nothing to do with XNA, its the .NET CLR. It's up to both CLR teams in that regard. The Desktop CLR hasn't been updated since .NET 2.0. However, there's really nothing they can do in this case. You would need new syntax to implement properties that return a reference to a value type, as the current property syntax operates on copies for value types, by definition.

The 10 million figure is just a benchmark. I realize we're not going to do 10 million updates per frame. The point was the relative time measurements, not the absolute measurements. The figures show us that having to copy just one of the matrices twice increases the matrix multiply time by nearly 2 times on the Desktop CLR. That's my point.

Also, there are quantities that will be modified many times in a given frame. Iterative solvers can go through 20-30 iterations per frame per object, and if each iteration requires multiple accesses to matrices, vectors, etc., this can easily add up.
Nov 4, 2007 at 7:49 PM
Sorry for not being clear, I did mean the CLR, but as I believe that the CLR will grow together with XNA (as they are the only drivers on the XBox platform atm) I often tend to refer both as just Xna improvements. I will be more clear in the future.

I do see where you are going, and I agree. Though I do believe that the update mechanism should be solved in another way, but right not I haven't got the solution.

My point was, and your perf data supports this, that the major gain here isn't ref vs property the main gain is for the CLR to get their act together and deliver the performance we need. Going from 5359 to 4222 ms isn't a "impressive" improvement going from 5359 to 1186 is.
Nov 4, 2007 at 8:24 PM

Sturm wrote:
Sorry for not being clear, I did mean the CLR, but as I believe that the CLR will grow together with XNA (as they are the only drivers on the XBox platform atm) I often tend to refer both as just Xna improvements. I will be more clear in the future.


That's true on Xbox, but not on Windows (where apparently the performance gap is even greater).


Sturm wrote:
My point was, and your perf data supports this, that the major gain here isn't ref vs property the main gain is for the CLR to get their act together and deliver the performance we need. Going from 5359 to 4222 ms isn't a "impressive" improvement going from 5359 to 1186 is.


I'm not sure what you mean here. Accessing a class member directly vs. through a property is inherently different. With properties, a copy has to be made. The CLR cannot optimize out the copy as that would violate the definition of value types.

Obviously, due to memory latency differences and other architectural differences (load/store RISC architecture vs CISC architecture, etc.) the numbers won't be very comparable between PC and Xbox. What matters are the relative differences on a particular platform. So it does come down to direct member access vs. property access. Though, I do agree that there is a lot of room for improvement in the Xbox CLR in a lot of other areas.
Coordinator
Nov 5, 2007 at 5:20 AM


Going from 5359 to 4222 ms isn't a "impressive" improvement going from 5359 to 1186 is.


From 5359 to 4222 means about a 20% improvement. I'd say that is pretty decent, granted still nowhere near the 80% of PC (which is even more of a reason to avoid them).

I think this argument when astray. We both seem to agree that accessors should be used in certain situations. I believe they are fine in situations where they won't be needed every single frame/loop. In cases like the piston engine discussion, that would only need to be done once, or at least very rarely, and will have almost no impact on the game. In cases where it would need to be called every frame, and may have possible conditionals within it, we know now it will have anywhere from a 20%-80% impact, and therefore should be avoided whenever possible. Clean code is important, but if the engine simply doesn't perform than it is worthless other than a learning tool. I get the feeling like many of these classes will never need to be touched once they're properly finished. For instance, if the physics engine covers the physics capabilities of any perspective users, I don't see anyone else modifying the code other than to learn from it. Now that doesn't mean we should have nasty code, that just makes for difficulties whenever others need to work with it, or when you go back 3 months later and try and understand what you did in the first place, but I don't think it warrants having accessors just for the sake of .net standards. An engine must be a high performance program, and unfortunately that means sacrifices.
Nov 5, 2007 at 4:02 PM
<rant>
Here's my take on the whole ".NET Movement". I think there are plenty of nice features in the .NET languages (namely, C#), properties being one of them, but the whole methodology relies on the assumption that the JIT compiler can produce highly-optimized code (on the same level, if not better than, native compilers). In theory, this is possible as the JIT compiler has knowledge not only of the code but of the underlying hardware on which it will be run, including cache line sizes, SIMD functionality, and many other details that are not available at compile-time (at least not without compiling a separate version for every possible hardware configuration). In practice, however, the JIT is no-where near this point, and the target hardware in most games is a small enough set that you can optimize at compile-time, especially for consoles. This will most likely change in the future with later CLR versions, but at the moment we're stuck with what we have.
</rant>

However, as I mentioned above, properties specifically do not quite fall into this category. The reason you have the copy penalty with value-type properties is due to the semantics of the language/run-time, not the JIT compiler. A value-type property is semantically equivalent to the following C++ code:

C++:
Matrix MyClass::GetOrientation() { return m_orientation; }
 
C#:
public Matrix Orientation { get { return m_orientation; } }

The point is that the copy is inherently necessary, and cannot be optimized out by the compiler or run-time (in both cases, C++ and C#). The only way to prevent the copy is to access the variable directly (C#: public access or returning by ref or out, C++: public access or return by reference). Now, the big question on the C# side of things with properties is whether one or two copies are made. It's entirely possible that one copy will be made from the member variable to the return value of the property 'get' method, then another copy from the return value of the property to the local caller variable. Now that could be optimized by the CLR and I'm not sure if it currently is.
Coordinator
Nov 5, 2007 at 11:15 PM
Shaw, there are some guys on the XNA forums interested in accessor performance, I think they have a bone to pick with your performance results, this could be helpful in our discussion on whether or not, and how often to use accessors.

http://forums.xna.com/ShowThread.aspx?PostID=30999#31022
Nov 6, 2007 at 12:44 AM
Perhaps you should have provided a little more context to the XNA forum people, lol :)

And where did you get 80%? At worst, worst case I'm only seeing 71%.

It's possible I just wasn't clear enough in my earlier posts, but those performance numbers represent value-type properties with "larger" data types. Reference-type properties and value-type properties on small data types (ints, floats, etc.) will not have a performance impact. The hit is in the copying.

Coordinator
Nov 6, 2007 at 1:00 AM
whoops :oP

Got their attention at least. heheh
Nov 6, 2007 at 6:27 PM
Unless there are any violent objections, I'm going to convert some of the classes to protected variables, namely SceneNode for my next commit in the next couple of days.
Nov 6, 2007 at 7:59 PM
What do you mean by classes to protected variables?
Nov 6, 2007 at 10:11 PM
private Matrix orientation;
 
public Matrix Orientation
{
    get; set;
}
 
to
 
protected Matrix orientation;
 
public Matrix Orientation
{
    get; set;
}

Coordinator
Nov 6, 2007 at 11:45 PM
So you mean private variables to protected variables?

I don't know enough about the code, but I would say that is probably not a problem if you're designing your class heirarchy right.
Nov 7, 2007 at 12:12 AM
I'm not going to go deeper into this as we have already discussed this throughly. There is no programatically errors in doing this. It violates PIMPL and raises requirements to those who interit from you. But this has already been discussed.
Nov 7, 2007 at 12:49 AM
My gut feeling is that the savings we can get from not copying a vector/matrix everytime we use it in a derived class is well worth the "PIMPL violation," regardless of my own personal feelings towards that convention.