julien-pires / pulsar Goto Github PK

Engine project and demo project are in the same solution. It will be better to separate both in two distinct solution.

Namespaces should look like this:

• Pulsar
  • Asset (Asset management)
    • Material
    • Mesh
    • Shader
  • Graphics (Graphic engine)
    • Rendering (Rendering engine)
      • RenderPass
    • Graph (Scene graph engine)
  • Core (Core function)
  • Math
  • Component (Component system)
  • Game (Game specific function)

When a sub mesh is added, mesh informations aren't updated.

Some source code aren't commented.

Create separate file for Mesh class and SubMesh class.

The constructor of GameObject class take a parameter of type GameObjectManager called owner. It defines the object manager in which the game object is contained.

This parameter should be removed. Instead, the Owner property is filled by the GameObjectManager when a game object is added.

Asset can be modified by anyone. In some case it will be useful to lock an asset from modifications to avoid undesirable modifications. Locking an asset prevents asset properties to be modified by developers but it's still possible to get the value. Assets manually created aren't locked so developers has a full access on what they create.

For example, a mesh loaded from a file should not be modified and the material associated too.

Currently a Mesh can have sub-meshes only if we load it through a file or a prefab factory. Creating an empty mesh is useless since we can't add sub-meshes.

Mesh should have a method to add sub-meshes in it. Developer provides some data composing the sub-mesh (rendering info, bounds, ...).

Currently the function which creates checkerboard texture made always the same, a texture of 256x256 with white/blue square.

Adding a size and colors parameters allow developers to create more personalized checkerboard texture.

The ID of SubMesh class is generated by SubMesh or Mesh class themselves. It will be better to generate this ID only by RenderingInfo class. SubMesh just has to return ID of RenderingInfo instance.

Instancing use the ID of SubMesh to create different instancing (one per sub mesh). So this ID has to be unique.

A node has a RemoveNode function to remove a child node. This is basic task of a RemoveNode function. But some node are specialized node such as scene node. In this case removing a child node implies more than removing it in the child list.

By creating a RemoveNodeIntern function, a specialized node can provide a specific functionality when a child is removed. For example with the scene node, the child is removed from the child list and the RemoveNodeIntern removes also the child in the scene graph.

RemoveNode function still exists and is responsible for calling RemoveNodeIntern.

BaseEffect class is no longer needed with the new way shader work in the engine.

Each time MeshBoundingBox is updated with a new BoundingBox, vertex buffer is filled with new vertices. Instead of create new verticeswe can use them and just update the Position property.

Add a function to get a SubEntity by his name from an Entity instance.

InstanceBatch and InstanceBatchManager is more rendering dependent than scenegraph dependent.

So these two classes should be moved to Rendering namespace.

Material properties can be modified by anyone. Adding the ability to lock a material can be useful to avoid unintended modifications.

For example, sub-meshes have a material property and some sub-mesh are loaded from file. We would like that material to be not modifiable because the sub-meshes can be used at many places in the game. In this case if the developer wants a different material, he has to create new one which will not be locked because created by himself and not by the asset manager.

Mesh vertex buffer and index buffer are public. This should be internal to limit acces to class inside Pulsar namespace.

Each submesh instance in a Mesh instance should have different name.

A mesh shares his vertex buffer with all his sub-meshes. In order to do this with customized mesh, empty mesh has to provide a vertex buffer. Developers have to use this buffer to create sub mesh rendering info. A check is performed when an attempt to add a sub-mesh in the mesh instance is done.

Rename Graph namespace to SceneGraph.

Rename SceneGraph class to not collide with it's own namespace.

RenderingInfo is (or will be) an internal class. Now SubMesh has to provide a method to modify some data such as:

• Primitive type
• Start index
• Vertex offset (in index notation, not byte to make it easier)
• Vertex count

Others data will be compute from these values.

Move the CreateMaterial function from the MeshManager to the MaterialManager.

ComponentHandlerSystem do a search before removing a ComponentHandler. This search is useless, remove function will return false if the ComponentHandler doesn't exist.

Add a function, in Mesh class, to remove a submesh with his name.

When a transformation, particularly rotation, is applied to a mesh, associated axis aligned bounding box doesn't fit the entire mesh. The method used to compute the aabb seems incomplete.

When a component is added to a GameObject, the ComponentHandlerSystem get notified by the GameObjectManager. The CHS dispatch the new component to every ComponentHandler.

One improvement would be to assign one or more Component type to a CH and dispatch the component only to interested CH. CH can expose a property which is an array of Type class corresponding to a type of Component or class derived of Component. A null value for this property can be interpreted as "Send me all Component".

A game object manager is responsible to keep track of all game objects and events related to them (added, removed, ...).

The demo project should be separated from the engine project, outside of the source folder. We can create a demo folder with a new VS solution only for demos.

Texture class name collide with the xna Texture class. Renaming will resolve this issue.

Material has a IsSolid and a IsTransparent properties. This means that if we assigned true to both properties, the material is "solid-transparent" at the same time.

It will be better to only have an IsSolid or an IsTransparent properties but not the two. With one we can have a solid or transparent material. Having only one allow the scene graph engine to sort correctly the material in the render queue.

The content parameter of Root class constructor is useless since it's the asset system which create ContentManager.

SubMesh material cannot be modified. It can be useful to allow doing this.

Move the GetArcRotation function from Vector3Helpers to QuaternionHelpers.

For each component type there is a component handler responsible for managing and updating them. A component handler define the behavior of a component.

All components handler have to be managed by a unique manager. In most common case there will be only one instance of a component manager in a scene (but having more is possible).

RenderingInfo instances are only used by the renderer system. Let it public isn't usefull. Instead we had to provide methods to modify information on it. Making it internal will hide buffers from outside.

The solution is composed of multiple project. One for the engine, one for the extension pipeline and one for the demo. The extension pipeline library uses some class found inside the engine library. This means the pipeline references the engine.

A better solution will be to create a third library containing classes shared used by the engine and the pipeline. This will make the engine and pipeline more independent from each others.

LazyRenderQueue class uses a List of LazyBatchInfo and a Dictionary for keep track of LazyBatchInfo instances. Instead of using these two structures, this class can use a Dictionary of LazyBatchInfo and the key will be the combination of batch ID and queue ID.

Out modifiers isn't compatible with C# version of Xbox 360. Interfaces using out have to be refactored to work as far as possible like if they have the modifier.

These namespaces contain interfaces with out modifiers:

• Asset

GeometryBatchManager class stores a List of GeometryBatch in a Dictionary by renderqueue ID. This list must be replaced by a Dictionary storing GeometryBatch by batch ID for fast search.

For the moment, Pulsar works only on Windows. The Xbox 360 version has to be created.

RenderingInfo instance of SubMesh class should be created internally.

ComponentHandler class has a constructor with a ComponentHandlerSystem parameter. This parameter is useless since it's the role of the HandlerSystem to assign the Owner property of a ComponentHandler.

Add a function to get a SubMesh instance in a Mesh by its name or index.

Rename GeometryBatch and GeometryBatchManager class with a name containing instancing or instance words.

When we add to a GameObjectManager an existing GameObject with Components, components aren't send to the associated ComponentSystemHandler. It seems that the ComponentAdded event isn't fired.

IMovable interface has a Visible property and an IsVisible property. These two names are relatively close and this can be confusing.

IsVisible property name need to be changed.

When you call Dispose of a ComponentHandler, it remove itself from its owner. Only the owner can remove a ComponentHandler. Dispose method should not remove it.

Some part of code use C# default parameters in function calls. This is incompatible with Xbox 360 C# version. These default parameters have to:

• be simply removed and it's up to the developer to assign each parameters when he uses the function
• be replaced by different version of the function each one having more and more parameters to match the equivalent one with default parameters

When Remove function of ComponentHandlerSystem is called, Dispose method of ComponentHandler should be called.

Currently the asset system use one ContentManager for all assets. This reduce the level of granularity in ressource management. Assets are load and keep in memory during the entire lifecycle of the application.

The asset management can be improved by providing a ContentManager for each asset storage. An asset storage can be used per scene. In rare case a scene needs more than one asset storage (except system asset storage). With that, there is one ContentManager per scene and unloading a scene will unload assets.