(I originally posted this on my MSDN blog.)
As I wrote last time, I made a bit of a mess in my GenesisEngine project by jamming too many responsibilities into one class. I’m working on straightening that out and ran across some interesting observations already. I’m not finished yet but I’ll share what I’ve discovered so far.
Performing the surgery
I decided to first separate the quad tree node responsibilities from the mesh generation responsibilities since there didn’t seem to be a lot of entangling between them and it appeared to be a straightforward exercise. It turned out that there actually was a fair bit of entanglement in the split/merge logic and the tests helped me identify that and sort it out. I’m a big believer in TDD and I’m still often surprised at how much clear feedback my unit tests give me about the quality of my design . . . if I take the time to listen!
Side note: as I’ve said before, I’m working on the GenesisEngine project and blogging about it in part because I wanted to provide some real-world examples that are a bit more complex and interesting than the typical toy problems you see in “intro to ” materials. The downside of real-world examples is that it’s a lot harder to paste in a bit of code that adequately illustrates what I’m talking about, since, well, it’s complex. I’ll do my best but if you’re really interested in understanding what’s going on you should probably inspect the diffs on GitHub or grab the code and study it.
So what happened? The first step in breaking up my SRP problem was to create a new QuadMesh class to handle the generation and management of the terrain mesh data. I moved the mesh code from QuadNode to QuadMesh and also created new QuadMeshRenderer and QuadMeshSpecs classes plus several other ancillary files. Once that was done I had to resolve several compiler errors because it turned out that QuadNode.Update() relied on the presence of the mesh data which was no longer there.
Here’s the original version of QuadNode.Update():
public void Update(TimeSpan elapsedTime, DoubleVector3 cameraLocation, DoubleVector3 planetLocation,
ClippingPlanes clippingPlanes)
{
var cameraRelationship = GetRelationshipToCamera(cameraLocation);
DetermineVisibility(cameraLocation, planetLocation, cameraRelationship.ClosestVertex);
if (_isVisible)
{
if (clippingPlanes.Near > cameraRelationship.ClosestDistance)
{
clippingPlanes.Near = cameraRelationship.ClosestDistance;
}
if (clippingPlanes.Far < cameraRelationship.FurthestDistance)
{
clippingPlanes.Far = cameraRelationship.FurthestDistance;
}
}
var distanceFromCamera = cameraRelationship.ClosestDistance;
if (_isVisible && distanceFromCamera < RealWidth() * 1 && !_hasSubnodes
&& Level = RealWidth() * 1.2 && _hasSubnodes)
{
Merge();
}
if (_hasSubnodes)
{
foreach (var subnode in _subnodes)
{
subnode.Update(elapsedTime, cameraLocation, planetLocation, clippingPlanes);
}
}
}
The GetRelationshipToCamera() method returned a private CameraRelationship DTO, which looked like this:
private class CameraRelationship
{
public DoubleVector3 ClosestVertex { get; set; }
public DoubleVector3 FurthestVertex { get; set; }
public double ClosestDistance { get; set; }
public double FurthestDistance { get; set; }
}
The compiler errors were in QuadNode.GetRelationshipToCamera(). The basic idea here is that QuadNode used to be looking at the mesh data and figuring out the distance from the camera to the closest vertex and the furthest vertex, and then was using that data to do several things:
- Figure out whether the node is visible
- Set the clipping planes appropriately if this node is closer or father than the clipping planes already are
- Decide whether to split or merge the node based on the ratio of camera distance to the real-space width of the node.
Complications set in
Ok, so obviously the GetRelationshipToCamera method needs to move to QuadMesh because it’s inspecting the mesh data, and the CameraRelationship class needs to be promoted to public so it can be shared between QuadNode and QuadMesh. QuadNode.Update() would call QuadMesh.GetRelationshipToCamera() and use the returned CameraRelationship DTO as it has been before. Simple. I made those changes (among others) and got everything to compile. There was only one change to QuadNode.Update(), which now looked like this:
public void Update(TimeSpan elapsedTime, DoubleVector3 cameraLocation, DoubleVector3 planetLocation, ClippingPlanes clippingPlanes)
{
var cameraRelationship = _mesh.GetRelationshipToCamera(cameraLocation);
// Other stuff is the same . . .
}
I then looked at my failing tests. Hmmm. All of my specs related to splitting and merging were failing because the stubbed-out QuadMesh object was returning null from GetRelationshipToCamera(). That’s not going to work. To solve that, I would need to create a CameraRelationship object in the spec context, populate it with specific numbers that would cause QuadNode to make the correct decision, and configure GetRelationshipToCamera() on the QuadMesh stub to return it. That means I’d have to think really hard about what the numbers ought to be in order to provoke the desired behavior in each spec context. Yuck.
The good news is that I’m lazy and that sounded like a lot of work. Way too much work, in fact. I thought about it for a couple of seconds and remembered the golden rule of TDD: “If you have to work hard to write your tests, you’re doing it wrong.”
Laziness FTW!
Ok, what am I doing wrong here? I have QuadNode going out to QuadMesh, retrieving a bunch of data, and making decisions based on that data. What kind of decisions? The same ones I listed above:
- Is the node visible to the camera?
- Do the clipping planes need to be adjusted to include this node?
- What is the ratio of camera distance to the real-space width of the node?
These decisions all have something to do with the mesh data:
- The visibility of the node is determined by the mesh because while a quad node has a 2D area, a mesh has a 3D volume. A large mountain may stick up over the horizon and be visible.
- The clipping plane adjustments are determined by the mesh for the same reason: the mesh is 3D.
- The camera distance part of the ratio is determined by the closest part of the node, which again is determined by the 3D mesh.
It’s at about this point that I got a mental image in my head of my unit test suite as a grizzled old sensei glowering at me, saying, “Have you learned nothing! Leave me and meditate upon the design principles! Perhaps you will find wisdom.”
I was trying to violate at least two principles with my stupid approach. First, the Law of Demeter, or “only talk to your immediate neighbors.” QuadNode was reaching through QuadMesh into the CameraRelationship object to get data.
Second, the Tell, Don’t Ask principle, or “don’t ask for information you need to do something, ask the object holding the data to do it for you.” Rather than telling QuadMesh to make decisions based on its private data and inform QuadNode of the results as I should have done, I was grabbing data from QuadMesh, moving it to QuadNode, and making the decision there.
Mind your own business
Ok, so how to fix it? The fixes were pretty simple once I had the principles firmly in my mind:
- Ask the QuadMesh whether it is visible to the camera. It has all the information needed to make that decision.
- Forward the clipping planes to the QuadMesh and have it modify them if necessary.
- Have QuadMesh calculate the ratio of of camera distance to the real-space width of the node and return that number. (This is technically still getting data from QuadMesh but it’s data that’s easily stubbed out and I’m ok with this until I find something wrong with it.)
Here’s the new QuadNode.Update() method which properly tells QuadMesh to do work and make decisions on its behalf:
public void Update(TimeSpan elapsedTime, DoubleVector3 cameraLocation, DoubleVector3 planetLocation,
ClippingPlanes clippingPlanes)
{
_mesh.Update(elapsedTime, cameraLocation, planetLocation, clippingPlanes);
if (_mesh.IsVisibleToCamera && _mesh.WidthToCameraDistanceRatio < 1 && !_hasSubnodes
&& Level 1.2 && _hasSubnodes)
{
Merge();
}
if (_hasSubnodes)
{
foreach (var subnode in _subnodes)
{
subnode.Update(elapsedTime, cameraLocation, planetLocation, clippingPlanes);
}
}
}
There’s another interesting lesson here as well, derived from Tell, Don’t Ask: it’s ok to create public members on the class that are highly specific to questions that other classes need to ask, as long as doing so helps you to hide private information. The QuadMesh.WidthToCameraDistanceRatio is a very specific sort of property. If I were designing this class as part of a generalized public framework this wouldn’t be something it would occur to me to implement. But I’m not designing a public framework; I’m designing a highly specific set of application classes that work with each other to solve a problem. In this case my goal should be to keep as much information hidden as possible (in this case, like the distance from the camera to the mesh) and only expose answers to questions or processed information that answers a specific question. This reduces coupling, increases cohesion, and makes the code more flexible and maintainable in the long run.
Side node: I’m maybe still not getting to the heart of Tell, Don’t Ask, since I’m still querying properties on QuadMesh rather than sending commands to QuadMesh, but it’s the best that I understand how to do right now.
It’s just magical how good unit tests will guide you to quality designs and warn you away from bad designs if you take the time to listen. You’d think I’d get used to it after awhile but the novelty hasn’t worn off for me yet. It’s ridiculously awesome. I giggle like a kid every time I see it.
If you want to examine the diff or download the source as it is after this fix, you can find it here.
SaintGimp 