This project is read-only.

Changeset 8325 / 8327

Dec 8, 2007 at 11:28 PM
Edited Dec 9, 2007 at 12:05 AM
Ok, there are quite a few changes here, although none have affected the main engine, which will make it easy to make changes, add, or remove, depending on a code review.

I'd really like a review on these changes, and they include part of the entity hierarchy and the scene manager and scene types. There will likely need to be some changes from you guys, so it would good to review and discuss now rather than have me keep expanding it. I also need to know if there is something I'm doing wrong early on, I'd prefer not to make a lot of changes and then we find out we'd rather have done it another way.

For example, I can load models into a scene, but Shaw has a render queue set up to use the model and set a material. I know this is only temporary, but we need a way to do this within the scene manager, and I wasn't sure if we should have a 'graphics' member in the scene manager or leave it in the QSgame example we currently have. Decisions like this would more than likely be left up to Shaw, because he's on graphics.

It might be a good idea to set up a standard review process.

Anyway, I've left the example game almost the same, the only difference is that the ship model is being loaded into the scene now. The camera is still in the example game, and the rendering is still being done from the example game, not from the scene manager.

I've allowed for the possibility of multiple scenes, but haven't accomodated it yet, if we decide not to allow multiple scenes then I can remove the code for it.

Here are the new classes that should be reviewed:
QSMatrix.cs
QSCommon.cs
ModelLoader.cs
SceneManager.cs
Scene.cs
StaticEntity.cs

and BaseEntity.cs already existed but needs to be reviewed.

Thanks guys.
Dec 8, 2007 at 11:50 PM

LordIkon wrote:
It might be a good idea to set up a standard review process.

For one do not check in code which hasn't been reviewed, as it tends to stay the way it is, also other will be hit by any change made during the review process. By other I do not mean us but those dl the source and duing implementation.
Dec 9, 2007 at 12:04 AM
Well we'd already agreed that check ins to the source don't require review until the first prototype was released (v0.19), because of the fact that anyone getting the source before then should know that the prototype can and/or will be buggy.
Dec 9, 2007 at 12:13 AM
I have some comments:
  • QSMatrix.cs
    • On QSBaseRotation do not prefix public members with QS, it's implicit in the namespace so might confuse
    • On CreateFromRotationVectors use cref's in comments when referencing types
    • On CreateFromRotationVectors do investigate "// Not sure yet why these have to be negative" as this might have major impact on implementation

  • QSCommon.cs
    • On GraphicsLevel keep each type in a seperate file, this also goes for non provate enums/structs as if can be difficult to locate these
    • Always add xml comments, ordenary comments will not be visible through the intellisense
    • Do not use all caption members (Keep Coding Guideline)
    • Do not use underscore in public members
    • On Multisampling_pref rewrite for better wording, as not everyone will know what pref is
    • On Multisampling_pref use Class.Member for static reference in order to keep consistency

  • ModelLoader.cs
    • Remember xml comments
    • Remember to prefix scope, even if it's implisit (such as private)
    • Initialize does not make sense as you would never have a ModelLoader without a ContentManager
    • On LoadStaticModel you should consider making it a template method as otherwise you would have to create one method for each loadable type (including custom types)
    • On LoadStaticModel you should use try catch and write a message to the console. I don't think it's fatal for the engine that it can't load a specific model

  • SceneManager.cs
    • SceneManager does not need a reference to GraphicsDevice, it's accessible through Game
    • SceneManager does not need a reference to ContentManager, it's accessible through Game
    • On Update do remember to add brackets event for single line if statements
    • I think we should consider having multiple scenes, as you can see it add another level of complexity we need to handle. For the initial release lets just stick to one scene.

  • Scene.cs
    • I feel this is more the SceneManager/SceneGraph, again I think initially we should stick to one scene and restructure this class
Dec 9, 2007 at 12:27 AM
Also the SceneSubSystem namespace shouldn't be there. In my mind the SceneManager(Graph) is at the very base of the engine. Which is why it should just belong to QuickStart namespace (just as QSGame)
Dec 9, 2007 at 12:30 AM
Thinking about it QSCommon is a bad name, as it's not really providing anything common, I think that QSConstants is a better alternative name.
Dec 9, 2007 at 12:54 AM


Sturm wrote:
On CreateFromRotationVectors use cref's in comments when referencing types


What do you mean by crefs? Never used it to my knowledge.


On CreateFromRotationVectors do investigate "// Not sure yet why these have to be negative" as this might have major impact on implementation


I believe I found the problem. The QSEngine was designed as a left-handed coordinate system, I was getting it confused with a right-handed system. In fact, I found some errors in the camera with my cross product math, due to the same reason. I'll fix both of these soon.


On GraphicsLevel keep each type in a seperate file, this also goes for non provate enums/structs as if can be difficult to locate these


What do you mean by this? You want all public enums in a single file?


Always add xml comments, ordenary comments will not be visible through the intellisense


I meant to do this, I'll finish commenting the other classes as well.


Do not use all caption members (Keep Coding Guideline)
Do not use underscore in public members
On Multisampling_pref rewrite for better wording, as not everyone will know what pref is
On Multisampling_pref use Class.Member for static reference in order to keep consistency


I am still used to C++ practices, I'll get on these soon too.


Initialize does not make sense as you would never have a ModelLoader without a ContentManager


You can't call a constructor to a sealed (singleton) class. And this class needs to be able to access the ContentManager, so it needs a reference to it, its not creating its own.


On LoadStaticModel you should consider making it a template method as otherwise you would have to create one method for each loadable type (including custom types)


I thought about this actually, and was thinking it may be better left up to Shaw, as he is creating the special model types.


On LoadStaticModel you should use try catch and write a message to the console. I don't think it's fatal for the engine that it can't load a specific model


Will a console.write do anything without a console window open?


SceneManager.cs
SceneManager does not need a reference to GraphicsDevice, it's accessible through Game
SceneManager does not need a reference to ContentManager, it's accessible through Game


I do it to keep the code concise. Do we really want to type .game in front of every reference to device and content? If so I'm fine by removing it.


I think we should consider having multiple scenes, as you can see it add another level of complexity we need to handle. For the initial release lets just stick to one scene.

I feel this is more the SceneManager/SceneGraph, again I think initially we should stick to one scene and restructure this class


There is only one scene at the moment. We should be able to treat the engine as a single scene until we decide if we need more. It would be more work to remove all the setup for multiple scenes, and then add it back in later if we wanted to keep it, then it would be to leave it as is now, and then remove the ability later if we decide we'll never need multiple scenes.
Dec 9, 2007 at 1:19 AM

LordIkon wrote:
What do you mean by crefs? Never used it to my knowledge.

Inside xml comments you can write <cref which will provide a like to the type/member referenced, later if you change the type/member, you will get a compile error in the xml comment if you didn't change that as well. Something like this:
        /// <summary>
        /// Gets/Sets the <see cref="GameTime"/> for this message
        /// </summary>

LordIkon wrote:
I believe I found the problem. The QSEngine was designed as a left-handed coordinate system, I was getting it confused with a right-handed system. In fact, I found some errors in the camera with my cross product math, due to the same reason. I'll fix both of these soon.

Sound "right" :)

LordIkon wrote:
What do you mean by this? You want all public enums in a single file?

No I mean that every type belong into a seperate file (so if you have 10 enums you have 10 files)

LordIkon wrote:
You can't call a constructor to a sealed (singleton) class. And this class needs to be able to access the ContentManager, so it needs a reference to it, its not creating its own.

Yes you can, the reason you can't is that it's private. You could simply make it internal and instantiate at QSGame.Initialize, this would also remove the need for the static constructor.

LordIkon wrote:
I thought about this actually, and was thinking it may be better left up to Shaw, as he is creating the special model types.

The problem is that now shaw needs to go through every place the method is invoked and change it.

LordIkon wrote:
Will a console.write do anything without a console window open?

You can redirect the input/output to anything you want. So Console would read/write to a socket or the ingame console if you want to.

LordIkon wrote:
I do it to keep the code concise. Do we really want to type .game in front of every reference to device and content? If so I'm fine by removing it.

Yes, because if you do this, then we would have to recreate these on all our classes in order to achieve consistency. Keeping it in game keeps it consistent as most types have a refrence to Game.

LordIkon wrote:
There is only one scene at the moment. We should be able to treat the engine as a single scene until we decide if we need more. It would be more work to remove all the setup for multiple scenes, and then add it back in later if we wanted to keep it, then it would be to leave it as is now, and then remove the ability later if we decide we'll never need multiple scenes.

The problem is again that we would need to change every code referencing this, and also introduces a complexity as users will need to know what to do with the additional methods.
Dec 9, 2007 at 1:32 AM

Sturm wrote:
No I mean that every type belong into a seperate file (so if you have 10 enums you have 10 files)


I don't see the benefit to having many added files simply for enums.


Sturm wrote:
Yes you can, the reason you can't is that it's private. You could simply make it internal and instantiate at QSGame.Initialize, this would also remove the need for the static constructor.


I've never seen it done this way, feel free to make the change. Sounds like your way may be better.


Sturm wrote:
The problem is that now shaw needs to go through every place the method is invoked and change it.


It is only used in one place in StaticEntity.cs, as we're only loading one type of model right now. All StaticEntities would call the same method.


Sturm wrote:
You can redirect the input/output to anything you want. So Console would read/write to a socket or the ingame console if you want to.


Yes, the problem is we haven't created an in-game console yet, in fact, I created an 'issue' for this earliar.


Sturm wrote:
Yes, because if you do this, then we would have to recreate these on all our classes in order to achieve consistency. Keeping it in game keeps it consistent as most types have a refrence to Game.


Ok, I'll make the change.


Sturm wrote:
The problem is again that we would need to change every code referencing this, and also introduces a complexity as users will need to know what to do with the additional methods.


We don't have users until the prototype is released, and it won't likely be released before we've made the decision to support multiple scenes or not.
Dec 9, 2007 at 1:39 AM

LordIkon wrote:
I don't see the benefit to having many added files simply for enums.

Consistency, you could quicky argue the same for input controls as most of these will only differ in template parameter, but they should still belong to a seperate file.

LordIkon wrote:
I've never seen it done this way, feel free to make the change. Sounds like your way may be better.

I will.

LordIkon wrote:
It is only used in one place in StaticEntity.cs, as we're only loading one type of model right now. All StaticEntities would call the same method.

If you have agreed on that I'm fine with it.

LordIkon wrote:
We don't have users until the prototype is released, and it won't likely be released before we've made the decision to support multiple scenes or not.

I would just have perferred that it hadn't been there as it also adds to code review.
Dec 9, 2007 at 3:13 AM
I've commited a new changeset that includes most of guys' recommended changes. Thanks for the review and input.
Dec 9, 2007 at 6:30 PM
Good work!

I'm a little confused on the overall goal of the ModelLoader class. Is this class going to be extended to eventually load unprocessed models as well (for editors and other tools)? Otherwise, the content pipeline code will always be sufficient for model loading. For loading any type of model and using it, you just need:

content.Load<ModelType>(path);

That's not going to change to require additional initialization steps.

Also, one thing I've been meaning to ask you: is your coordinate system left-handed or right-handed? I've heard you mention that its just an X rotation from the standard XNA coordinate system, but then it seems like you implied it's a left-handed system, where XNA is a right-handed system and their matrix computations assume right-handed coordinates, particularly LookAt and CreatePerspective. While coding some of the graphics parts, I've run into weird little inconsistencies that usually result from handedness-differences, so I want to be very clear about this. Can you explain precisely what your coordinate system is? And if its really left-handed, then we need to roll our own Matrix methods as well for LookAt, CreatePerspective, etc.
Dec 9, 2007 at 6:35 PM
We have to use right-handed, it's the default for Xna, and we really do not want to force everyone else to switch. Though DirectX devs might love it as they are prob used to left-handed (Welcome to the managed world ;))
Dec 9, 2007 at 6:38 PM
The lookat and createperspective methods seem to work fine. In fact they're both being used in the current changeset. Of course, there may be things we're not using yet that are based on these matrices.

What is being used now is a Left-handed coordinate system with a world 'up' of +Z.

Forward is +Y
Right is +X
Up is +Z

If you look in the QSVector class you'll see all of our standard vectors.

If you believe we really need a right-handed system I could go through and convert all the math to account for this. It would require swaping cross product orders and a few negative signs in the QSVector class. And a few things in the QSMatrix class I believe.

I chose Left-handed because it seems counter-intuitive to me that right X is negative. On a standard graph, like you'd use for algebra or calculus, +X is always to the right. But I've learned that just because it is counter-intuitive to me, doesn't mean a left-handed system might not be counter-intuitive to someone else.
Dec 9, 2007 at 6:42 PM
I was thinking of a loader class, so that we didn't have to create a LoadModel function for every entity type. I couldn't put it into BaseEntity because BaseEntity doesn't have a model.

Like Sturm said, I think just a single LoadModel function based on a template, where you pass it a path, and get a model back, would be great. It was just an idea, if you guys can find an easier, or cleaner way, that'd be awesome.
Dec 9, 2007 at 6:42 PM
Edited Dec 9, 2007 at 6:49 PM
I'll switch to a right-handed system, I want to keep things similar to XNA standards to make it easier to adopt the engine.

EDIT: Done! I'll submit it sometime tonight hopefully. I'm hoping to get a reliable camera system design soon as well.
Dec 9, 2007 at 7:30 PM
Left or right handed doesn't really matter to me, it's just that the XNA created Perspective and View matrices can't be used in a left-handed system (at least not without some conversion). If your current system is left-handed and you're using Matrix.CreateLookAt and Matrix.CreatePerspectiveFieldofView, then there is a fundamental inconsistency in your system. However, it might not be noticable if all you're going to is placing and drawing models.
Dec 9, 2007 at 7:41 PM
Its cool either way, I've already switched everything over to a right-handed system.
Dec 9, 2007 at 7:57 PM
Alright, cool. I'm hoping that solves some of the consistency issues I'm having, especially with lighting and shadow mapping.
Dec 9, 2007 at 8:04 PM
Edited Dec 9, 2007 at 8:05 PM
I'd offer to help, but shaders just aren't my thing. Feel free to look at any of the shaders in the template, although at your own risk. Not sure if they'd be of any help.
Dec 9, 2007 at 8:14 PM
It's not the shaders, its how positions are interpreted. That's why I'm thinking its a coordinate system issue with how the projection/view matrices are formed.
Dec 9, 2007 at 8:20 PM
I'm not sure. I guess I've never had a problem with the matrices before, whether it was left or right handed.
Dec 10, 2007 at 6:57 AM
Edited Dec 10, 2007 at 6:58 AM
Looking into coodinate systems, I realized how little I really knew about the default ones. I didn't know that the right-handed system has forward as coming toward the screen by default. In fact, the only difference between them is that the Z direction for 'forward' is flipped. +Y is always Up, and +X is always right.

I'll have to do quite a few more tests with the QSVector3 and QSMatrix classes.
Dec 10, 2007 at 7:26 AM
I just realized the entire engine right now is running off of the standard XNA coordinate system, a right handed one. All my math is going to be off. I'm so used to the template I didn't realize we hadn't been on the template's system. DOH!
Dec 10, 2007 at 7:37 AM
That's one of the reasons I've been advocating the original XNA coordinate system. Not only are people used to it, but you never have to worry about inconsistencies when you're using the XNA matrix routines. It may not be the most intuitive coordinate system in the world (though you can certainly argue it's a decent system), but its best selling point is that people will be used to it. All of the Matrix math routines, model importers, and sample code is written in one coordinate system, and by defining our own, we're sort of going against our own principle of keeping this as easy on users as possible. Now, we have to explicitly tell users which methods in Vector3/4 and Matrix that can be safely used, and which ones must be replaced with our own versions in QSVector3/4 and QSMatrix. But I've already voiced my opinion on this awhile ago...


Dec 10, 2007 at 8:06 AM
I've considered a few times changing to the XNA default, but then I go back over all the code I would have to re-write, and always throw up in my mouth a little at the though. :oP

I cannot believe how confused I am becoming over this whole thing.

Here is what gets me:
XNA is on a standard right-handed coordinate system, with a world 'up' of Y. If this is the case, why does rotating about Y axis by 180 degrees turn my model upside down?!! Rotating about the Z axis by 180 turns my model toward the camera. This is only something I would expect in the QSEngine's coordinate system, however, I was just under the impression we hadn't converted to that yet.

I'm wondering if the camera changes I made are confusing the shit out of me. More tests I guess.
Dec 10, 2007 at 8:55 AM
Edited Dec 10, 2007 at 8:56 AM
Alright, that's it, I'm done with the conversion.

I can't tell if the new model importer is changing the coordinate system, or what. When I leave rotation at Matrix.Identity, I would think I would get the model either in our coordinate system, or XNA's coordinate system right? Well I get XNA coordinates for the forward, right, up, etc....but the damn ship is facing away from the camera instead of towards it.

I've made the decision of going with the standard coordinate system, no matter how much I hate it. I'll have to rework the particle systems, weather systems, terrain and water components as well. But it'll be a good chance to clean things up as I go. I just want the engine to be as easy as possible, and having different system is already causing us to have issues, so I'll just take the plunge now, rather than later.

I will need you guys to make sure that anything you've done for the Z-up coordinate system is set to work with a standard right-handed system.

Tomorrow I will go through an remove the QSVector3 and QSMatrix classes, all references to them, and then I will begin fixing the camera system which may not be happy with the changes.

Also, Shaw, I'll need you to make sure the model processor imports the models with a +y up axis and a forward of -Z, otherwise I won't know if my changes are working. We may have to re-import the model as well.
Dec 10, 2007 at 3:14 PM
The model processor for StaticModel does in fact transform the coordinate system to my best guess as to what coordinate system you were using. ;)

If you check in the content processor options for ship.fbx, you'll see an option to turn off the transformation. With it off, it won't do any coordinate system transform besides the ones implicit in the model (different model parts defined in different local systems). So, just make sure that option is off (for every model) and you'll have the model in the standard XNA coordinate system (right-handed, Y-up).

You know the "trick" to determine handed-ness, right? Hold your hand out with all fingers aligned. Make your thumb perpendicular to your fingers (if your hand is palm up with fingers facing forward, thumb will be pointing to the side). Point your fingers towards +X, curl your middle/ring/pinky fingers towards +Y. Wherever your thumb is pointing is +Z. If you used your left hand, that's a left-handed coordinate system. If you used your right hand, that's a right-handed coordinate system.

Dec 10, 2007 at 10:03 PM
I do understand the systems now, I just need to determine what is causing the rotation issues.

Currently with a rotation of Matrix.Identity given to entry.World, the model's vector directions are reporting as I would expect, on the right-handed system. However the model is facing 'backward', which is +Z in a right handed system. This basically means that the model itself would have to have been imported backwards, or processed differently, or the world has been rotated 180 degrees in the view, or a whole slew of other things, one of which could be that I was extremely tired, or just a lack of experience.

I may take a deeper look into it before truly re-writing all the old template stuff for a Y up system.
Dec 11, 2007 at 12:50 AM
Whats the status of the current committed code? Is the camera supposed to set up an XNA coordinate-system, or still your custom system?
Dec 11, 2007 at 12:57 AM
In the current codebase, the positioning is correct (proper +/- axes direction) but the camera up vector isn't quite right. Its perpendicular to what it should be.
Dec 11, 2007 at 1:21 AM
Edited Dec 11, 2007 at 1:26 AM
The current status is the custom system.

The perpendicular camera may be due to the fact its designed for a Z-up system, which a 90 degree rotation from the standard system. Hence the QSMatrix.BaseRotation is a 90 degree rotation on the X axis.
I'll look into the up vector when I'm back in front of the code, thanks Shaw.
Dec 11, 2007 at 5:26 AM
Free camera's up is currently +Z, which is desired with the current coord system. I'll keep looking.
Dec 11, 2007 at 5:38 AM
It was indeed the content processor, good call. Turns out this could be a very good thing though. After processing the models like that we're still able to use all the rotation techniques built in to XNA. The only things we can't use, from what I can tell are the matrix.(direction) commands.

All the view and projection matrices seem to be working out fine.

Knowing this I will have a working fix soon most likely. If we do run into any major issues like this again I may have to seriously consider a standard right handed system. But I'm reluctant to refactor most of the original template, as it will add weeks (at least) to the engine release.
Dec 11, 2007 at 5:51 AM
Then add weeks. We should use the standard handiness of Xna, unless you can come up with a really good reason why that handiness is wrong.
Dec 11, 2007 at 6:04 AM
The reason it still works is that the coordinate system transform isn't from right-handed to left-handed, it just rotates the coordinate frame to right-handed z-up, which is what I originally thought you were using because you still used Matrix.CreateLookAt and Matrix.CreatePerspectiveFieldOfView.

I'm with Sturm on this one. If we were working in native OpenGL or DirectX, it wouldn't really matter as we'd have to write all our own math code and model importers anyway. But with XNA, all you get by changing the coordinate system is the inability to use a significant part of the built-in functionality, including select matrix methods and the model importers. There's a post by Shawn in reply to one of my posts where he states the model importers will always adjust the model geometry to be right-handed Y-up, regardless of exporter options. By default, OpenGL (through gluPerspective/gluLookAt) and DirectX (through D3DXMatrix...) are both Y-up, so the native people and XNA people are all going to be use to a Y-up system (either left-handed or right-handed).

Dec 11, 2007 at 6:28 AM
Ok, then it is settled.....let the conversion begin. Time to bend my mind a little bit....
Dec 11, 2007 at 6:33 AM
Done! Wow, that wasn't so bad. Although I'm not considering any other the stuff from the template.

By the way, Shaw, were you having rendering issues? I noticed before the conversion some green flames and odd lightly on the model, that has all gone away.
Dec 11, 2007 at 6:36 AM
You should be able to get the changeset now with the standard right-handed coordinate system. Have fun with it.