Skip to main content
Code Review

Return to Answer

deleted 1 character in body
Source Link
glampert
glampert
  • 17.2k
  • 4
  • 30
  • 89

The previous review already made some pretty good points, so I'd just like to point out a few other things.

First a couple design considerations:

What you're trying to implement is collectively known as a Scene Graph, you probably already know that. This is a big subject in itself, and an important part of many 3D rendering applications. The three main tasks of a scene graph are:

  • Fast queries of objects in the scene. E.g.: Scene object A might need to ask the Scene Graph for the world positions of objects B and C at every update.

  • Space partitioning for culling. I.e.: Define which parts of the scene and which geometries are visiblyvisible and send only those to the renderer, avoiding wasted processing drawing invisible stuff.

  • Represent some hierarchy or ordering of objects. This is very useful for object transforms and animations / attaching stuff together so that the transforms are relative.

As you can see, the job of a scene graph is not trivial, but very important nonetheless. It is also not uncommon for a 3D rendering app to have separate graphs for different concerns. For instance, games usually have a separate graph just for world partitioning and culling, which is part of the "rendering system", while another graph just stores the hierarchies and relationships of game objects. This is a smart approach, since it better separates concerns. An interesting piece discussing a similar subject: GameArchitect.

Now the code, it looks good, but there's one tiny bit that is a real eye sore to me:

assert((Entity*)this != component);

I don't think you need that type cast at all, since Node is a child of Entity. Nevertheless, you should never, ever use a C-style cast to cast between related class types. A C-style pointer cast is just a bitwise reinterpretation of the data, so you can end up doing something like this (very contrived example, I know):

int * xyz = new int[...];

// Many lines and function calls latterlater...

Entity * ent = (Entity *)xyz;

And the compiler wouldn't mutter a word about it. Always use dynamic_cast or static_cast to type cast pointers to objects.

  • dynamic_cast is a "smart cast", it fails with a nullptr if there's no viable conversion between the types, so you should use this one when you're not sure if the types are compatible.

  • static_cast is more slack, it only performs some compile-time type inference, but no runtime error checking. Performance-wise, it is faster, but you can only use it in instances when you are 100% sure the types are a match. So be extra careful.

The previous review already made some pretty good points, so I'd just like to point out a few other things.

First a couple design considerations:

What you're trying to implement is collectively known as a Scene Graph, you probably already know that. This is a big subject in itself, and an important part of many 3D rendering applications. The three main tasks of a scene graph are:

  • Fast queries of objects in the scene. E.g.: Scene object A might need to ask the Scene Graph for the world positions of objects B and C at every update.

  • Space partitioning for culling. I.e.: Define which parts of the scene and which geometries are visibly and send only those to the renderer, avoiding wasted processing drawing invisible stuff.

  • Represent some hierarchy or ordering of objects. This is very useful for object transforms and animations / attaching stuff together so that the transforms are relative.

As you can see, the job of a scene graph is not trivial, but very important nonetheless. It is also not uncommon for a 3D rendering app to have separate graphs for different concerns. For instance, games usually have a separate graph just for world partitioning and culling, which is part of the "rendering system", while another graph just stores the hierarchies and relationships of game objects. This is a smart approach, since it better separates concerns. An interesting piece discussing a similar subject: GameArchitect.

Now the code, it looks good, but there's one tiny bit that is a real eye sore to me:

assert((Entity*)this != component);

I don't think you need that type cast at all, since Node is a child of Entity. Nevertheless, you should never, ever use a C-style cast to cast between related class types. A C-style pointer cast is just a bitwise reinterpretation of the data, so you can end up doing something like this (very contrived example, I know):

int * xyz = new int[...];

// Many lines and function calls latter...

Entity * ent = (Entity *)xyz;

And the compiler wouldn't mutter a word about it. Always use dynamic_cast or static_cast to type cast pointers to objects.

  • dynamic_cast is a "smart cast", it fails with a nullptr if there's no viable conversion between the types, so you should use this one when you're not sure if the types are compatible.

  • static_cast is more slack, it only performs some compile-time type inference, but no runtime error checking. Performance-wise, it is faster, but you can only use it in instances when you are 100% sure the types are a match. So be extra careful.

The previous review already made some pretty good points, so I'd just like to point out a few other things.

First a couple design considerations:

What you're trying to implement is collectively known as a Scene Graph, you probably already know that. This is a big subject in itself, and an important part of many 3D rendering applications. The three main tasks of a scene graph are:

  • Fast queries of objects in the scene. E.g.: Scene object A might need to ask the Scene Graph for the world positions of objects B and C at every update.

  • Space partitioning for culling. I.e.: Define which parts of the scene and which geometries are visible and send only those to the renderer, avoiding wasted processing drawing invisible stuff.

  • Represent some hierarchy or ordering of objects. This is very useful for object transforms and animations / attaching stuff together so that the transforms are relative.

As you can see, the job of a scene graph is not trivial, but very important nonetheless. It is also not uncommon for a 3D rendering app to have separate graphs for different concerns. For instance, games usually have a separate graph just for world partitioning and culling, which is part of the "rendering system", while another graph just stores the hierarchies and relationships of game objects. This is a smart approach, since it better separates concerns. An interesting piece discussing a similar subject: GameArchitect.

Now the code, it looks good, but there's one tiny bit that is a real eye sore to me:

assert((Entity*)this != component);

I don't think you need that type cast at all, since Node is a child of Entity. Nevertheless, you should never, ever use a C-style cast to cast between related class types. A C-style pointer cast is just a bitwise reinterpretation of the data, so you can end up doing something like this (very contrived example, I know):

int * xyz = new int[...];

// Many lines and function calls later...

Entity * ent = (Entity *)xyz;

And the compiler wouldn't mutter a word about it. Always use dynamic_cast or static_cast to type cast pointers to objects.

  • dynamic_cast is a "smart cast", it fails with a nullptr if there's no viable conversion between the types, so you should use this one when you're not sure if the types are compatible.

  • static_cast is more slack, it only performs some compile-time type inference, but no runtime error checking. Performance-wise, it is faster, but you can only use it in instances when you are 100% sure the types are a match. So be extra careful.

Source Link
glampert
glampert
  • 17.2k
  • 4
  • 30
  • 89

The previous review already made some pretty good points, so I'd just like to point out a few other things.

First a couple design considerations:

What you're trying to implement is collectively known as a Scene Graph, you probably already know that. This is a big subject in itself, and an important part of many 3D rendering applications. The three main tasks of a scene graph are:

  • Fast queries of objects in the scene. E.g.: Scene object A might need to ask the Scene Graph for the world positions of objects B and C at every update.

  • Space partitioning for culling. I.e.: Define which parts of the scene and which geometries are visibly and send only those to the renderer, avoiding wasted processing drawing invisible stuff.

  • Represent some hierarchy or ordering of objects. This is very useful for object transforms and animations / attaching stuff together so that the transforms are relative.

As you can see, the job of a scene graph is not trivial, but very important nonetheless. It is also not uncommon for a 3D rendering app to have separate graphs for different concerns. For instance, games usually have a separate graph just for world partitioning and culling, which is part of the "rendering system", while another graph just stores the hierarchies and relationships of game objects. This is a smart approach, since it better separates concerns. An interesting piece discussing a similar subject: GameArchitect.

Now the code, it looks good, but there's one tiny bit that is a real eye sore to me:

assert((Entity*)this != component);

I don't think you need that type cast at all, since Node is a child of Entity. Nevertheless, you should never, ever use a C-style cast to cast between related class types. A C-style pointer cast is just a bitwise reinterpretation of the data, so you can end up doing something like this (very contrived example, I know):

int * xyz = new int[...];

// Many lines and function calls latter...

Entity * ent = (Entity *)xyz;

And the compiler wouldn't mutter a word about it. Always use dynamic_cast or static_cast to type cast pointers to objects.

  • dynamic_cast is a "smart cast", it fails with a nullptr if there's no viable conversion between the types, so you should use this one when you're not sure if the types are compatible.

  • static_cast is more slack, it only performs some compile-time type inference, but no runtime error checking. Performance-wise, it is faster, but you can only use it in instances when you are 100% sure the types are a match. So be extra careful.