New patch uploaded #591

Coordinator
Dec 18, 2007 at 6:31 AM
Edited Dec 18, 2007 at 6:32 AM
The patch has been uploaded (for real this time). Whoever would like to review and/or apply it, and/or shoot it down and crush my dreams can do so now.

http://www.codeplex.com/Project/Download/FileDownload.aspx?ProjectName=QuickStartEngine&DownloadId=23973
Dec 18, 2007 at 6:34 AM
Do you want me to review it or are you planning on other updates before checkin?
Coordinator
Dec 18, 2007 at 6:36 AM
Edited Dec 18, 2007 at 6:39 AM
Its ready for review and application (pending it passes review of course). Bear in mind Shaw wanted it applied with a couple of temporary things in there so he could get an idea of what the graphics end would need. The exact entity draw and render queue setup hasn't been determined yet, so that may change.

EDIT: I'm off to bed for the night, I'll read the review in the morning.

Sturm, while I'm thinking about it, did I see your email said Denmark on it? Are you from Denmark, or currently in Denmark? My company is currently working directly with a company from Denmark. Just thought that was an interesting coincedence, if it is in a coincedence at all. Just wondering.
Coordinator
Dec 18, 2007 at 4:20 PM
I'd be sweet if I could get the review before 9pm MST, so I could start on new stuff. Thanks guys.
Dec 18, 2007 at 4:36 PM
Ill look at it, but dont expect a comment. I dont know enough about that aspect of game design, so Im happy to trust what youve come up with.
Dec 18, 2007 at 5:36 PM
Looks great! Allocations are fine, it's just BoundingFrustum.Intersect that's generating garbage each frame. It would be nice if the XNA team provided functionality to recreate the frustum without having to instantiate a new object each time. Though I would request that future patches/commits not have full-screen mode enabled. Full-screen DirectX and the Visual Studio debugger dont always play well together on single-monitor systems. :)

While we're waiting on Sturm's comments, I'll start integrating the new graphics code so you can take a look at it.
Dec 18, 2007 at 6:06 PM
hey, how did that land on my table :p

I'm updating my system as there seem to be something fishy with the .net framework. I'll get on once I can run cpc again.
Dec 18, 2007 at 6:22 PM
You don't have to, you just seem to enjoy doing code quality reviews. :)
Dec 18, 2007 at 7:10 PM
Since I do care about the quality of the code I have to do it :)

(Denmark it is, though I'm working in a U.S. based company with a subsiderary in denmark, so I guess pure coincedence)

In BaseEntity.cs
  • Do not expose fields, Create methods if you need by ref assignment
  • Do not have set only properties, this totally invalidates design experience
  • Are we going to use the Is... pattern for boolean fields, if not then rename IsVisbile
  • You are introducing Update, you should instead introduce IUpdateable and implement a virtual UpdateCore method

In Camera.cs
  • I really dislike the CameraType enumeration, this forces others to change the source code. We need a better scheme here
  • Do not expose properties byond the private scope. If perfromance gets an issue there are other patterns we can use to solve this
  • Remember to use this prefix for instance members
  • Remember xml comments for all member (at least for public/protected)

In FixedCamera.cs
  • Remember to use this prefix for instance members

In FreeCamera.cs
  • Remember to use this prefix for instance members
  • Do honor naming conversion, const member should be PascalCase
  • Do not expose properties byond the private scope. If perfromance gets an issue there are other patterns we can use to solve this
  • defaultFOV isn't assigned to except on construction are you sure you do not want to make it readonly?
  • Wouldn't FOV be camera dependent? FOV should be determined by the viewer and should be exposed through it.

In StaticEntity.cs
  • Remember to use this prefix for instance members
  • In the second constructor you do not add the quereEntry to the instance and the entry is lost

In CameraInterface.cs
  • Remember to use this prefix for instance members
  • In GetCamera how do you retrieve the second FPS camera? The code always returns the first, and there is no rule that there can't be multiple cameras of the same type

In QSConstants.cs
  • You've changed the game to run in widescreen in 1280, but not everyone might support that.
  • You've disabled VSync and FixedTimeStep, any reason for this?
  • I think you should rename DefaultTerrainElevStr to DefaultTerrainElevationStrength

In Scene.cs
  • Remember to use this prefix for instance members
  • Do not expose properties byond the private scope. If perfromance gets an issue there are other patterns we can use to solve this, If it's temporary you should still keep coding guidelines, it's public code and we should show consistency and show how strongly we think about it.

In SceneManager.cs
  • Remember to use this prefix for instance members
  • In update remember bracket even for single line if statements
  • We should have a FPS class which did all the fps calculation

In FPSCamera.cs
  • Remember to use this prefix for instance members
  • This is identical to the FreeCamera you should revise it.

In Light.cs
  • Remember to use this prefix for instance members
  • Remember xml comments
  • Do not expose properties byond the private scope. If perfromance gets an issue there are other patterns we can use to solve this

In Terrain.cs
  • Remember to use this prefix for instance members
  • Remember xml comments
  • Do not expose properties byond the private scope. If perfromance gets an issue there are other patterns we can use to solve this
  • In SetupTerrainVertices remember bracket even for single line if/for statements

In Terrain.cs
  • Remember to use this prefix for instance members
  • Remember xml comments
  • Do not expose properties byond the private scope. If perfromance gets an issue there are other patterns we can use to solve this
  • Keep method size small, specific and contained, i.e. SmoothTerrain could be split into left and right method

In TerrainPatch.cs
  • Remember to use this prefix for instance members
  • Do not expose properties byond the private scope. If perfromance gets an issue there are other patterns we can use to solve this

IN QSMathHelper.cs
  • I think you should rename this to QSMath as it actually does the math for you
  • In Random5050 remember bracket even for single line if statements
Dec 18, 2007 at 9:50 PM
I don't know why, but terrains in wire-frame mode look so freakin' sweet. :)

When converting over to the new graphics system, I found the following artifact in the terrain geometry? Is this from the terrain stitching, or something else?

TerrainArtifact
Coordinator
Dec 18, 2007 at 10:21 PM
I actually wasn't aware of this. I designed the quad-tree about 4 months ago, so it may be something I fixed back then, I do remember an issue of having a gap between quads. I know there is a better way to do this, so I can probably get it fixed.
Coordinator
Dec 18, 2007 at 11:08 PM
Sturm, the review is helpful, but it might save us time if when you saw something without this. prefix you simply added it before you applied the patch. This goes for single-line if statements without brackets.

I can post comments your review later when I have time, thank you again for the full review.
Dec 18, 2007 at 11:38 PM
The new GraphicsSystem has been merged into the terrain system. As of right now, I just commented our the drawing code in Terrain and replaced it with the new drawing methods. LordIkon, I'll let you organize that how you like. I'm going to clean it up a bit then submit a patch, which will actually be a "super" patch of LordIkon's patch and my patch, since I don't believe you can create a patch of a patch, lol.
Coordinator
Dec 19, 2007 at 12:40 AM
New drawing methods? I'm curious. Mostly because I won't code in front of me for a few more hours.
Dec 19, 2007 at 1:03 AM
The new system works like this:

Every "manager" that can supply renderable data implements the IRenderChunkProvider interface. This interface has one method: QueryForRenderChunks(ref RenderPassDesc). Whenever the graphics system is requested to render a new frame, it takes its list of IRenderChunkProviders, and calls QueryForRenderChunks on each of them, passing along a RenderPassDesc structure which defines properties for the given rendering pass. Right now, this structure only contains a Camera instance. The implementors of IRenderChunkProvider will internally do a visibility query, GUI generation, or whatever else it needs to in order to generate the RenderChunks that are potentially visible for the render pass, be it world geometry, GUI panels, etc.

Right now, SceneManager is the only derived type of IRenderChunkProvider. It will pass the query down to each Scene, which passes the query to each Terrain, which passes the query to the root QuadTree, which ultimately passes the query to the leaf nodes. For example, QuadTree.QueryForRenderChunks is as follows:

        public void QueryForRenderChunks(BoundingFrustum bFrustum)  // Slightly different signature to match what was passed from Terrain.Draw and QuadTree.Draw
        {
            // View is kept track of for later when vegetation is drawn.
            // This keeps the program from having to calculate the frustum intersections
            // again for each node.
            inView = false;
 
            // Check if QuadTree bounding box intersection the current view frustum
            if(bFrustum.Intersects(boundingBox))
            {
                InView = true;
 
                // Only draw leaves on the tree, never the main tree branches themselves.
                if(isLeaf)
                {
                    int detail = (int)terrain.Detail;
 
                    RenderChunk chunk = Game.Graphics.AllocateRenderChunk();
 
                    //Game.GraphicsDevice.Indices = leafPatch.indexBuffers[detail];
                    chunk.Indices = leafPatch.indexBuffers[detail];
                    chunk.VertexStreams.Add(terrain.VertexBuffer);
                    chunk.Declaration = terrain.VertexDeclaration;
                    chunk.WorldTransform = Matrix.Identity;
                    chunk.VertexCount = width * width;
                    chunk.StartIndex = 0;
                    chunk.VertexStreamOffset = vertexBufferOffset;
                    chunk.PrimitiveCount = leafPatch.numTris[detail];
                    chunk.Material = terrain.Material;
                    chunk.Type = PrimitiveType.TriangleList;
                }
                // If there are branches on this node, move down through them recursively
                else if(childQuadTrees.Length > 0)
                {
                    for(int i = 0; i < childQuadTrees.Length; i++)
                    {
                        childQuadTrees[i].QueryForRenderChunks(bFrustum);
                    }
                }
            }
        }

You can change the name and parameter to whatever is most appropriate to you. The only thing that matters is that SceneManager implements the interface method and then recurses to its children.
Coordinator
Dec 19, 2007 at 1:29 AM
Sounds cool enough, I'll have to get the code in front of me and get the full picture.

I'm wondering if I should continue with terrain, or move to a new area of the engine for now. I keep thinking about geomipmapping and other stuff, but at the moment I'm getting about 250fps on average with the terrain, so I think it might be more important to get some more essentials out of the way so we can make faster progress. I'd really like the input system in the trunk so we can get gamepad, mouse, and keyboard going, which make testing easier.