GenesisEngine: Yes, SRP Violations Hurt

(I originally posted this on my MSDN blog.)

In the process of my continuous learning about agile development, one of my biggest problems is that it’s easy to find materials that say, “Do this, don’t do that,” but offer only trivial examples at best.  I’m always wishing for some non-trivial examples of what the principles, or the violation of the principles, look like in the real world.  Part of the reason why I put GenesisEngine up on GitHub and am blogging about it here is to provide some slightly-less-than-trivial examples of good techniques, but just as importantly, examples of mistakes and how to fix them.

True confessions

So, I have a confession to make.

I work on GenesisEngine in my spare (ha!) time and progress has been kind of slow.  I spent a fair amount of time up front on infrastructure and was careful to build it all with TDD and SOLID principles.  I had a good time building that stuff but really, all of the interesting parts have to do with the generation and rendering of the terrain itself.  Everything else is just plumbing.

So after spending quite a number of weeks telling my friends that I was working on this nifty terrain engine project but having only a featureless white ball to show them, I really, really wanted to get something working that resembled actual planetary terrain.  The problem was moderately complex and I was growing impatient.  I started cutting corners.  My QuadNode class started out fairly small but it quickly accumulated a lot of responsibilities and started to sprawl all over the place.  I was violating the Single Responsibility Principle, and frankly, I made a mess.

Warning signs

One of the early warning signs that you have a fat class that does too many thing is that it’s not fun to write tests for it.  Rather than just having simple inputs and outputs to manage in your tests, you have to construct elaborate chains of actions just to get your object into the state that you want to test.  The tests aren’t elegant statements of intent; they’re full of confusing noise.

You’ll also see a lot of combinatorial explosion going on where you get a ridiculous number of different contexts that say, “when A is true and B is true and C is true”, then “when A is true and B is true and C is false”, and so on through all the combinations of states.  It’s tedious to write tests for each combination, especially when they’re messy tests anyway.

As I got deeper into the functionality of my quad tree terrain generation and rendering system, I started to clearly see those warning signs in my code.  But . . . I just wanted to get something working.  Like, now, darn it!  I was tired of waiting, and I resisted the obvious need to refactor the QuadNode class because it would take more time than I was willing to spend.  Rather than stopping to figure out how many responsibilities I had running around in QuadNode and then figuring out how to tease them apart into separate classes, I simply stopped writing tests for that class.  Once I did that then it was easy to not build the Perlin noise generation system test-first either.

Stampede!

In my non-technical life I’m into long-distance backpacking and in that world we have a term for when you’re about half a day out from town after four or five days and 100+ miles on the trail, and someone says the magic word.  “Pizza.”  Or maybe “hamburgers”.  The technical term for what happens then is “stampede”.  All common sense and self-preservation go right out the window and everyone hurtles down the trail at breakneck speed in an effort to get to town.  Sometimes people punish their bodies in ways they end up regretting later.

We stampede in software development, too.  We spend a lot of time being careful, doing thing right, making steady progress, but at some point close to the end we sometimes say, “Ah, screw it, let’s just hack it together and make it work!”  The result is usually something we call technical debt.  You build up a pile of messy stuff that you’ve got to go back and fix later.

I guess that’s not always a bad thing.  If you’re trying to hit an aggressive deadline and you need to just throw yourself forward right at the end, building up some technical debt is a valid way to do that.  Or if, like me, you just want to see something working and you’re not willing to wait, you can hack something together to scratch that itch.

The really evil thing about technical debt is not the short-term impact of creating it.  You can sometimes derive a lot of benefit from technical debt in the short term.  No, the evil thing about technical debt is when you don’t immediately go back and clean it up once your short-term goal is realized.

Anatomy of an SRP violation

Right now the QuadNode class is 472 text lines long.  Visual Studio code analysis reports that it has one of the worst maintainability indexes of any class in the project.  It has at least three big responsibilities jammed into it right now:

  1. As the name implies, it acts as a node in the quad tree.
  2. It also owns the job of generating heightfield data, vertex buffers, and index buffers.  This clearly has nothing to do with #1.
  3. It also has to decide when it’s appropriate to split itself into four node children or to merge and delete its children.  I first thought that was a trivial aspect of #1 but it turns out to be a huge deal in its own right.

Here’s one of the QuadNode spec contexts.  When a node is updated, it may decide to do certain things based on the state of the world.  In this case, when a non-leaf node (that is, a node that has children) is far enough away from the camera, the children nodes should be removed and disposed because we don’t need that level of detail any more.

[Subject(typeof(QuadNode))]
public class when_a_nonleaf_node_is_updated_and_the_camera_is_far : QuadNodeContext
{
    public static DoubleVector3 _nearCameraLocation;
    public static DoubleVector3 _farCameraLocation;
    Establish context = () =>
    {
        _nearCameraLocation = DoubleVector3.Up * 11;
        _farCameraLocation = DoubleVector3.Up * 15 * 10 * 2;
        _node.InitializeMesh(10, Vector3.Up, Vector3.Backward, Vector3.Right, _extents, 0);
        _node.Update(new TimeSpan(), _nearCameraLocation, DoubleVector3.Zero, _clippingPlanes);
    };
    Because of = () =>
        _node.Update(new TimeSpan(), _farCameraLocation, DoubleVector3.Zero, _clippingPlanes);
    It should_remove_subnodes = () =>
        _node.Subnodes.Count.ShouldEqual(0);
    It should_dispose_subnodes = () =>
    {
        foreach (var subnode in _node.Subnodes)
        {
            ((IDisposable)subnode).AssertWasCalled(x => x.Dispose());
        }
    };
}

This is not a horrible test set.  Believe me, I’ve seen (and written!) worse.  But let’s look at a couple of things that it’s trying to tell me:

  • The process of setting up a non-leaf node in the context is built on indirect side-effects.  Instead of just telling my class under test, “Hey, assume you’re a non-leaf node”, I have to initialize the node’s mesh, then call .Update() with camera coordinates that are near enough to cause the node to split itself and generate children, then call .Update() again with different camera coordinates that are far enough to cause the node to merge its children.  The spec isn’t able to say what it means explicitly; it’s very roundabout.  Someone unfamiliar with the code base would probably have to put in significant effort to understand how the spec works.
  • There’s no way to determine whether the QuadNode we’re testing decided to merge its children except by inspecting its children.  Again, this is relying on indirect side-effects.  There’s no way to get a clear statement from the class that says, “Yes, I’ve decided to merge!”, which is really what I’m interested in testing here.
  • This spec context is one of four that test a combinatorial set of conditions:
    • When a non-leaf node is far away from the camera
    • When a non-leaf node is close to the camera
    • When a leaf node is far away from the camera
    • when a leaf node is close to the camera
    • when a leaf node is at the maximum allowable tree depth and is close to the camera
  • There is another factor that isn’t even mentioned in these specs because I didn’t want to deal with a doubling of the condition set.  A node should only be split if it’s not over the horizon and out of sight, and it should be merged if it does get too far over the horizon even if the camera isn’t far enough to cause a merge on its own.  That would turn my five contexts into nine.  Yuck.

The implementation of .Update() in QuadNode is about as circuitous as these specs would lead you to believe.  There’s a lot of stuff going on in Update but it’s not clearly explained.  There are quite a few tests and branches and it’s not very maintainable.

So what’s the root problem here?  The root problem is that I violated the Single Responsibility Principle.  The decision of whether to split or merge a quad node is a good-sized responsibility all on its own.  There are different ways to make that decision and it’s probably something I’ll want to fiddle with a lot over time since it heavily impacts performance and memory footprint.  I probably need a SplitMergeStrategy class for the QuadNode to depend on, or maybe even separate SplitStrategy and MergeStrategy classes.

What would that buy me?  First, it would help break apart the combinatorial set.  The QuadNode wouldn’t have to care anything about the position of the camera or whether it’s below the horizon.  All it would have to know is that if it’s a leaf node, make a call to SplitStrategy, otherwise make a call to MergeStrategy.  If the return value is true, do the appropriate thing.

SplitStrategy and MergeStrategy, for their part, wouldn’t have to know whether they’re being called by a leaf or non-leaf node.  They trust the QuadNode to take care of that question.  They just need to think about the camera distance and the horizon and respond with yes or no.  Not only does that reduce the combinatorial set but it also makes the inputs and outputs very explicit.  Inputs are numbers, output is a boolean.  No mysterious multiple calls to QuadNode.Update to set up the context and no mysterious poking at child nodes to determine the results.

Cleaning up my mess

The technical debt I incurred certainly accomplished my short-term goal.  I’ve got a working proof of concept of a planetary terrain engine and I feel satisfied at reaching that milestone.  However, now I have a problem.  The implementation of my terrain generation is very naive and does all of its work on the main thread.  At low altitudes this causes so much stuttering as to render the program virtually unusable unless you first turn off updates, move somewhere, then turn updates back on and wait awhile.  The fix for that is obviously to a) enlist my other cores for terrain generation and b) do the generation asynchronously so that camera movement and frame rate aren’t impacted, even if I have to wait a bit for higher levels of detail to show up.

Well, yes, that’s a great plan except that my QuadNode class is a mess.  The code that I need to make more complex with threading and async logic is exactly the code that’s already overly-complex and obtuse and isn’t fully covered by tests.  Ah, ok, now we see the downside of technical debt.  You get a quick spike of progress and then a long, slow, painful slide into hell.

I’ve promised myself that before I do any more significant work on this project, I’m going to clean up my mess and break QuadNode into multiple classes with single responsibilities.  I’m curious to see how it turns out.  If you want to take a closer look at the code as it is at the time of this writing, the permalink to the current tree is here.

GenesisEngine: Behavior-oriented Language

(I originally posted this on my MSDN blog.)

As I wrote in my previous post, BDD is largely about preserving the flow of intent from your user stories to your unit tests (specifications, in BDD parlance) to your product code.  As developers, we’re in the habit of switching over from user intent (features that solve problems) to developer intent (mechanics of the code) when we write tests, but preserving as much user intent as possible all the way through is a lot better for maintainability and it helps drive better initial designs, too.  It’s the same as the ubiquitous language in DDD.  Don’t abandon the ubiquitous language; stick with it as long as possible.

In other words, don’t focus on how the code works, think about how the system behaves.  The code will follow naturally.

It turns out, though, that it’s surprisingly hard to do this well.  At least, I find that I often have to remind myself to leave implementation language out of my context and specification names and to focus on how a user would describe the behavior in domain language.

I recently prepared a presentation on MSpec for my group at work and I used the Camera specs in the GenesisEngine project as an example of specs written in the behavior-focused style.  There’s nothing like putting your work in front of other people to make you take a fresh look at it with a critical eye!  As I read over my Camera specs, I realized that I had let some implementation language sneak in when I wasn’t looking.  In other places I had been pretty vague about the behavior that was actually expected.

For instance, consider this context:

[Subject(typeof(Camera))]
public class when_view_parameters_are_set_by_look_at : CameraContext
{
    Because of = () =>
        _camera.SetViewParameters(new DoubleVector3(0, 1, 1), DoubleVector3.Zero);
    It should_set_the_camera_location_correctly = () =>
        _camera.Location.ShouldEqual(new DoubleVector3(0, 1, 1));
    It should_set_the_camera_yaw_correctly = () =>
        _camera.Yaw.ShouldEqual(0f);
    It should_set_the_camera_pitch_correctly = () =>
        _camera.Pitch.ShouldEqual((float)(-Math.PI / 4));
    It should_set_the_camera_roll_correctly = () =>
    _camera.Roll.ShouldEqual(0f);
    It should_set_the_camera_view_transformation_correctly = () =>
        _camera.OriginBasedViewTransformation.ShouldEqual(
            GenerateOriginBasedViewMatrix(_camera.Location, _camera.Yaw,
                                          _camera.Pitch, _camera.Roll));
}

It should set location/yaw/pitch/roll/transformation correctly?  What the heck does that mean?  That tells very little about what my intent actually was.  I was just being lazy and didn’t want to bother with trying to carefully describe the intent.

Actually, I bet what I was thinking was something like, “Hmm, my expectation here is that these properties should be set to specifc numbers.  I don’t want to state those numbers in the spec names, though, because that’s a test detail.  I’ll just say it should set the properties ‘correctly’ because that sounds more generalized.”

But what was my real intent for the behavior of the camera when the view parameters are set via a look-at point?  Well, the real intent is that the camera should go to the requested location and set its orientation to whatever values are needed to face toward the look-at point from that location, and finally generate a new view transformation based on the new camera state.  Ok, now that’s a description that’s phrased in terms of the problem domain, not the implementation domain.  Let’s see if we can improve those specs:

[Subject(typeof(Camera))]
public class when_view_parameters_are_set_by_look_at : CameraContext
{
    Because of = () =>
        _camera.SetViewParameters(new DoubleVector3(0, 1, 1), DoubleVector3.Zero);
    It should_go_to_the_requested_location = () =>
        _camera.Location.ShouldEqual(new DoubleVector3(0, 1, 1));
    It should_set_the_yaw_to_face_toward_the_look_at_point = () =>
        _camera.Yaw.ShouldEqual(0f);
    It should_set_the_pitch_to_face_toward_the_look_at_point = () =>
        _camera.Pitch.ShouldEqual(-MathHelper.Pi / 4);
    It should_set_the_roll_to_face_toward_the_look_at_point = () =>
        _camera.Roll.ShouldEqual(0f);
    It should_generate_a_view_transformation_for_the_current_state = () =>
        _camera.OriginBasedViewTransformation.ShouldEqual(
            GenerateOriginBasedViewMatrix(_camera.Location, _camera.Yaw,
                                          _camera.Pitch, _camera.Roll));
}

That’s better.

In the location spec, I got away from the implementation language of “setting the location (property)” and used the domain language of “going to a location (in the world)”.  Very similar, but different perspectives.  For the orientation components, I described the intent of facing in a particular direction.  And for the view transformation, I called out the fact that the transformation is dependent on the new state.

Now, a lot of you may be looking askance at me right now.  Isn’t this nitpicking pretty silly?  Well, sure, it’s not earthshattering or anything.  I didn’t fix a bug or implement a new feature with these changes.  But I think I made the code a little bit cleaner, and that’s the real reason why I started this project in the first place.  It’s not about terrain rendering, it’s about fine-tuning my techniques.  Lesson learned: avoid writing specs that say something should be done “correctly”.  Describe what the correct behavior actually is.

The diff of the improvements I made to the Camera specs can be found in two parts here and here.

BDD Unit Testing is Not That Complicated

(I originally posted this on my MSDN blog.)

One of the first sessions at the Alt.Net Seattle conference was one on Behavior-Driven Development, or BDD.  Actually, we had three suggested sessions all related to BDD that we combined into one session, which was probably a bit of a mistake in hindsight because we had a couple different groups of people looking for different things (BDD at the unit test level vs. BDD at the acceptance test level), which caused a bit of controversy.

I think that at the unit test level, BDD really isn’t that different than normal TDD that we all know and love.  All it really brings to the table is a strong emphasis on the arrange-act-assert structure and an even stronger emphasis on the behaviors you’re trying to build in your code rather than the mechanics of the code itself.  In other words, BDD asks that you think in terms of what the user wants to do and how you’re going to enable them to do it.  You give clear, explicit names to each scenario that you need to implement and you also give clear names to each expectation that you have for the scenario.  The overall point is simply to write tests that people can actually read.

Anyway, Charlie Poole (one of the developers of NUnit) made a comment to the effect of, “Well, I’ve been doing that sort of thing in my unit tests for years already.  Why do we even have to have a special name for this?”  I also noticed a lot of other people asking things like, “Well, what about SOLID principles?  Do they still apply?  How about mocking frameworks or IoC containers?  Can I still use those?”

This confusion is really unnecessary, and Charlie’s right: it’s unfortunate that we even have a name for it that makes it sound like it’s something different than TDD.  At least at the unit test level, BDD is not a brand new way of writing tests.  It’s just the same old red-green-refactor workflow that we’ve always used; just with a stronger emphasis on expressing customer-oriented intentions so that when other developers have to pick up your code and maintain it later, they’ll know why your tests exist, what user value they map to, and when they break, it’ll be obvious what needs to be fixed.  You still use all the same state-based and interaction-based testing techniques in your tests and the same SOLID principles in your product code.  Nothing changes.

Relax – it’s not that complicated.

 

GenesisEngine: Using WPF in XNA and other non-WPF applications

(I originally posted this on my MSDN blog.)

There are a couple of posts on the excellent Pandemonium game development blog (which sadly seems to have not been updated recently) that talk about the importance of making your game engine easily configurable and and diagnosable.  That’s important for any application, of course, but it’s particularly critical for graphics engine where things happen in real-time and a lot of what you see on the screen is not easily interpreted to root causes.  Diagnostic and configuration tools help you figure out what’s going on with your engine.

For GenesisEngine, I knew I wanted to have two debugging features:

  1. The ability to easily view the current configuration options and change them at runtime.
  2. The ability to view statistics and diagnostic information that would help me understand what the app is doing.

As I noted before, XNA doesn’t give you much help out of the box when it comes to building a UI with buttons, checkboxes, textboxes, and all those other things that we take for granted in standard Windows apps.  Development tools are important but I didn’t want to spend a lot of time building them.  Because I’m ok with my app being Windows-only right now, it made sense to try to use a Windows-based presentation system, like, say WPF.

The problem was that the XNA and WPF systems are very, very different and there wasn’t a whole lot of material that explained how to glue them together in one app.  Fortunately, the answer is pretty simple even if it was a little hard to find so I’ll share it here to help out anyone else who may be wondering the same thing.

To be clear, my approach here is to display WPF windows from an XNA application.  Embedding an XNA surface inside a WPF application is a whole different subject!  And actually this has nothing to do with XNA: the approach found below will work for any kind of application where you want to control the main app thread yourself and run WPF on a secondary thread.

In order for WPF to work correctly, it needs a few things:

  1. A separate STA thread
  2. A thread dispatcher object for that thread
  3. A message pump

Here’s my WindowManager that makes those things happen:

public class WindowManager : IWindowManager, IDisposable
{
    IContainer _container;
    IScreenCustodian _settingsCustodian;
    IScreenCustodian _statisticsCustodian;
    Dispatcher _windowDispatcher;
    public WindowManager(IContainer container)
    {
        _container = container;
        StartUIThread();
        _windowDispatcher.Invoke((Action)(() =>
        {
            // We pull these out of the container here instead of doing normal
            // constructor injection because we need them to be created on this thread.
            _settingsCustodian =
                _container.GetInstance<IScreenCustodian>();
            _statisticsCustodian =
                _container.GetInstance<IScreenCustodian>();
        }));
    }
    public void ShowAllWindows()
    {
        _windowDispatcher.Invoke((Action)(() =>
        {
            _settingsCustodian.ShowInactive();
            _statisticsCustodian.ShowInactive();
        }));
    }
    void StartUIThread()
    {
        var dispatcherCreatedEvent = new ManualResetEvent(false);
        var thread = new Thread(() =>
        {
            _windowDispatcher = Dispatcher.CurrentDispatcher;
            dispatcherCreatedEvent.Set();
            Dispatcher.Run();
        });
        thread.SetApartmentState(ApartmentState.STA);
        thread.IsBackground = true;
        thread.Start();
        dispatcherCreatedEvent.WaitOne();
    }
    public void Dispose()
    {
        if (_windowDispatcher != null)
        {
            _windowDispatcher.InvokeShutdown();
        }
    }
}

There are a few notable things here.  First, all of the WPF-related objects need to be created on the WPF thread.  I’m pulling them all out of my IoC container which means that they have to be pulled from the container on the WPF thread, not on the main app thread, which means that my WindowManager has to retrieve them from the container itself rather than having them injected.  Side node: I may be over-relying on the container again here but I have a very simple UI system at the moment so I haven’t run into major problems.

Second, when the WindowManager creates the UI thread it sets it to use the STA threading model which WPF requires.  It also makes it a background thread so that it won’t keep the application alive if the main thread quits.  That’s appropriate for GenesisEngine but maybe not for other apps.  The Event object is used to verify that the UI thread is indeed created and running before we continue.

Third, we call Dispatcher.Run to start the message pump on the UI thread.  If this isn’t done then WPF won’t work.

Fourth, all interaction between the main app thread and the WPF elements has to go through Dispatch.Invoke to marshal the calls onto the UI thread.  You can see that in the ShowAllWindows method.

Lastly, the WindowManager is disposable so that it can cleanly shut down the dispatcher’s message pump when appropriate.  Actually, I suspect I still have an issue with clean shutdown somewhere because occasionally the MSpec runner will complain about mysterious errors when cleaning up my unit tests but I haven’t yet invested a lot of time in chasing down the root cause.

This code seems to work pretty well to create and display WPF windows for my XNA app.  I’m not doing a whole lot with them yet; the statistics window updates itself once per second and shows a few interesting numbers but the settings window isn’t hooked up to anything yet.  I’ll make more use of them shortly but the infrastructure appears to be working.

GenesisEngine: Input

(I originally posted this on my MSDN blog.)

Once I had the event aggregator set up in GenesisEngine I could think about how to turn keyboard and mouse input into events that other parts of the app could respond to.

The XNA framework doesn’t offer as much help in this area as you might be used to in Windows Forms or WPF.  There isn’t any built-in windowing, or UI controls, or a commanding system so you pretty much have to build your own from scratch.  This isn’t a terribly difficult task, I suppose, but I like the way mine turned out.

XnaInputState

All XNA offers for input is a KeyboardState and MouseState struct every update cycle that contain the current state of the keyboard (the current up or down state of every key) and the current state of the mouse cursor (where it currently is and whether each button is currently up or down).

In order to figure out interesting things like did was a certain key just now pressed this update or has it been held down for awhile, or how far did the mouse move between the last update and this one, you’ve got to track both the last state and the current one and check the differences yourself.  The XnaInputState class handles this responsibility but it’s pretty trivial so I won’t list it here.

InputMapper

The InputMapper class is a bit more interesting.  It stores mappings between states and event messages that should be sent when those states occur, where states in this case mean a key was pressed, or a key is being held down, or the mouse cursor moved.  The mappings are set up in code right now but could be loaded from a config file in the future.  This is from the main Genesis program class:

void SetInputBindings()
{
    _inputMapper.AddKeyDownMessage(Keys.W);
    _inputMapper.AddKeyDownMessage(Keys.S);
    _inputMapper.AddKeyDownMessage(Keys.A);
    _inputMapper.AddKeyDownMessage(Keys.D);
    _inputMapper.AddKeyDownMessage(Keys.E);
    _inputMapper.AddKeyDownMessage(Keys.C);
    _inputMapper.AddKeyDownMessage(Keys.Z);
    _inputMapper.AddKeyPressMessage(Keys.F);
    _inputMapper.AddKeyPressMessage(Keys.U);
    _inputMapper.AddKeyPressMessage(Keys.P);
    _inputMapper.AddKeyPressMessage(Keys.OemPlus);
    _inputMapper.AddKeyPressMessage(Keys.OemMinus);
    _inputMapper.AddKeyPressMessage(Keys.G);
    _inputMapper.AddKeyPressMessage(Keys.Escape);
    // TODO: we don't specify which mouse button must be down
    // (hardcoded to right button ATM),
    // this can be extended when we need to.
    _inputMapper.AddMouseMoveMessage();
}

When a message and an input state are mapped together, InputMapper stores them in lists for later use.  Specifically, it stores a set of delegates that will be executed when the correct input conditions are detected and these delegates send the proper messages to the event aggregator to be forwarded to whomever is interested in them.

I’m creating and storing a delegate that sends an event message rather than simply storing the type of the message because that was the only way I could figure out how to call EventAggregator.SendMessage with a strongly-typed message object.  Essentially I have to capture a generic type parameter, save it away, and later pass it to another generic method without losing its type information.  Creating a delegate at save time accomplishes that.  I’m not thrilled with how obtuse it makes the code but it’s livable for now.  I wouldn’t mind finding a better solution, though.

public void AddKeyPressMessage(Keys key) where T : InputMessage, new()
{
    _keyPressEvents.Add(new KeyEvent { Key = key, Send = x =>
        _eventAggregator.SendMessage(new T { InputState = x}) });
}
public void AddKeyDownMessage(Keys key) where T : InputMessage, new()
{
    _keyDownEvents.Add(new KeyEvent { Key = key, Send = x =>
        _eventAggregator.SendMessage(new T { InputState = x }) });
}
public void AddMouseMoveMessage() where T : InputMessage, new()
{
    _mouseMoveEvents.Add(new MouseMoveEvent { Send = x =>
        _eventAggregator.SendMessage(new T { InputState = x }) });
}
private class KeyEvent
{
    public Keys Key;
    public Action Send;
}
private class MouseMoveEvent
{
    public Action Send;
}

During each update, InputMapper is told to handle input and is passed an IInputState reference.  Based on this input state, it finds any message-sending delegates who’s conditions match the current conditions and executes those delegates.  InputMapper doesn’t know anything about who’s interested in input events, it just fires them.

public void HandleInput(IInputState inputState)
{
    SendKeyPressMessages(inputState);
    SendKeyDownMessages(inputState);
    SendMouseMoveMessages(inputState);
}

private void SendKeyPressMessages(IInputState inputState)
{
    foreach (var keyEvent in _keyPressEvents.Where(keyEvent =>
        inputState.IsKeyPressed(keyEvent.Key)))
    {
        keyEvent.Send(inputState);
    }
}

I like how the responsibilities are clearly separated in this system:

  1. Tracking input state changes – XnaInputState
  2. Firing events based on the current input state – InputMapper
  3. Actually sending the events to listeners – EventAggregator
  4. Receiving and acting on events – Implementers of IListener

I think it would be interesting to see how to incorporate the new Reactive Extensions into the input system.  Rather than checking the current input state against a set of mappings every time, the InputMapper would set up some LINQ expressions against an input event sequence.  I haven’t tried using the Reactive Extensions yet but from what I’ve read so far it seems like it should simplify the concepts here.

GenesisEngine: The Event Aggregator

(I originally posted this on my MSDN blog.)

GenesisEngine is still a pretty small code base at this point but there are some design elements that I’m pretty happy with.  I’ll run through a series of posts describing these parts so that people can learn from them or maybe critique them and find ways to make them even better.

Event Aggregator

I lifted the design of the event aggregator directly from Jeremy Miller’s posts on the subject and from his implementation in StoryTeller.  There’s not much I can add to what Jeremy’s already said but I’ll summarize the concept.

Here’s the very small interface for my aggregator:

public interface IEventAggregator
{
    void SendMessage(T message);
    void AddListener(object listener);
}

An event, or message, can be any type you want to use.  You can put as much context data as you wish into your message object.  When you send a message, the message object is forwarded to all listeners who have stated a desire to receive that type of message.

When you add listeners to the event aggregator you don’t explicitly list what messages you want the listener to receive.  Instead, the listener’s class definition is marked up by implementing one or more flavors of IListener, like so:

public interface IListener
{
    void Handle(T message);
}

public class Settings : ISettings,
                        INotifyPropertyChanged,
                        IListener,
                        IListener,
                        IListener,
                        IListener,
                        IListener
{
    // Class stuff here
}

The event aggregator looks for those interfaces to figure out which messages the listener is interested in:

public void SendMessage(T message)
{
    IEnumerable recipients;
    lock (_lockObject)
    {
        recipients = FindEligibleListeners();
    }
    SendMessageToRecipients(message, recipients);
}

private IEnumerable FindEligibleListeners()
{
    var eligibleListeners = new List();
    foreach (var weakReference in _listeners)
    {
        // We need to create a strong reference before testing aliveness
        // so that the GC doesn't yank it out from under us.  Don't convert
        // this to a LINQ expression because it doesn't guarentee that behavior
        var strongReference = weakReference.Target as IListener;
        if (strongReference != null)
        {
            eligibleListeners.Add(strongReference);
        }
    }
    return eligibleListeners;
}

I use StructureMap as my IoC container and I’m using Jeremy’s neat trick of auto-registering listeners when they are created by the container, so most of the time I don’t even have to explicitly add listeners to the aggregator:

public class EventAggregatorTypeInterceptor : TypeInterceptor
{
    public object Process(object target, IContext context)
    {
        context.GetInstance().AddListener(target);
        return target;
    }

    public bool MatchesType(Type type)
    {
        return type.ImplementsInterfaceTemplate(typeof(IListener));
    }
}

I don’t have a RemoveListener() method on my event aggregator right now for two reasons:

  1. The event aggregator holds weak references to all listeners so it’s not necessary to explicitly remove them for garbage-collection purposes.
  2. So far I haven’t had a need to remove a listener before the end of its lifetime so there’s been no need to implement that functionality yet.

I’m very happy with this eventing design.  It’s simple, has low overhead, and just feels elegant to me.

The source for GenesisEngine is available here.

GenesisEngine: Don’t Get Domain Objects From The Container

(I originally posted this on my MSDN blog.)

IoC containers are awesome and I use them in all my non-trivial projects.  However, there’s an interesting caveat when it comes to using IoC containers that isn’t totally obvious: don’t get domain entities straight from the container.

Surprisingly, this rule doesn’t seem to have a lot of source material.  It’s well-known among the IoC experts (you might run into terse “don’t do that” comments on mailing lists and discussion boards) but I’ve had a really hard time finding a clear, concise explanation for exactly why getting domain objects from the container is an anti-pattern.  Maybe my web search kung fu is weak, I dunno.  This post is about the best I’ve been able to find and even it doesn’t do into the reasons behind the rule.  If anyone has a better source, let me know.

Unfortunately I’m not going to write that definitive explanation either but I can at least demonstrate what happened when I ignored it.  I already knew that domain objects don’t belong in the container before I started writing GenesisEngine but for some reason I wasn’t really thinking of the Planet class (and all of its component classes) as domain objects.  I’m not sure why; probably because I started with the assumption that there would be one and only one planet, but of course it’s really a domain entity because it has uniquely identifying information (its location and radius).

So what happened?

How it was

My MainPresenter was created from the container and received an injected IPlanet from the container, like so:

public MainPresenter(IPlanet planet, ICamera camera, IWindowManager windowManager,

    Statistics statistics, ISettings settings)

{

    _planet = planet;

    _planet.Initialize(DoubleVector3.Zero, PhysicalConstants.RadiusOfEarth);

    _camera = camera;

    _windowManager = windowManager;

    _statistics = statistics;

    _windowManager.ShowAllWindows();

    _settings = settings;

    _settings.ShouldUpdate = true;

}

The container created the Planet object but it couldn’t give it any uniquely identifying information because the container didn’t know anything about what kind of planet I wanted to create.  So in the MainPresenter constructor, I had to call IPlanet::Initialize() to pass in the unique attributes.  This two-stage construction is a definite anti-pattern because there’s no safety net to prevent the consumer of IPlanet from forgetting to initialize the injected dependency.

Here’s the construction and initialization of the planet class where you can see the two-stage construction.  Note that the concept of “inject then initialize” was cascading to the Planet dependencies like the PlanetRenderer as well.

public Planet(IPlanetRenderer renderer, ITerrainFactory terrainFactory,

    IHeightfieldGenerator generator, Statistics statistics)

{

    _renderer = renderer;

    _terrainFactory = terrainFactory;

    _generator = generator;

    _statistics = statistics;

    _clippingPlanes = new ClippingPlanes();

}

public void Initialize(DoubleVector3 location, double radius)

{

    _location = location;

    _radius = radius;

    CreateTerrain();

    _renderer.Initialize(_radius);

}

Aside from obtuse code that was likely to break, my main problem was that I could only have one planet in my program.  What if I wanted to put a moon in orbit around my planet, or model a solar system?  Clearly injecting planets from the container into the MainPresenter wasn’t the right approach.  I guess I could have had the MainPresenter grab additional planets straight from the container itself, but it’s a code smell to reference the container outside of bootstrapper/setup code.

Interestingly, this poor design was indirectly causing bizarre problems in other seemingly-unrelated areas of the code.  For instance, I had this comment in my initialize method for the main Genesis program class:

protected override void Initialize()

{

    this.IsMouseVisible = true;

    _mainPresenter = ObjectFactory.GetInstance<MainPresenter>();

    // TODO: we really don’t need a reference to the camera controller here but

    // no one else seems to need a reference to it either and it needs to be

    // created otherwise the event aggregator won’t subscribe it.  Is there a

    // better way to handle this?

    _cameraController = ObjectFactory.GetInstance<ICameraController>();

    _inputState = ObjectFactory.GetInstance<IInputState>();

    _inputMapper = ObjectFactory.GetInstance<IInputMapper>();

    SetInputBindings();

    base.Initialize();

}

I had a CameraController object that needed to exist but nothing in the whole app seemed to have a reference to it.  I had to hack in a reference in my main program class just to force the object to be created and to stay around.  Turns out that problem was caused by my pulling domain objects out of the container.  The IPlanet was being directly injected into the CameraController when it should have gone a different route.

How I fixed it

Fixing this kind of mess usually involves factories.

Rather than injecting the IPlanet from the container I injected an IPlanetFactory into the MainPresenter.  MainPresenter can now use the planet factory to create as many planets as it needs.  It turns out that MainPresenter now has kind of a two-step creation process – construct it then show it, but that makes logical sense for a presenter and actually helps straighten out the flow of showing other windows, etc.

public MainPresenter(IPlanetFactory planetFactory, ICamera camera, ICameraController cameraController, IWindowManager windowManager, Statistics statistics, ISettings settings)

{

    _planetFactory = planetFactory;

    _camera = camera;

    _cameraController = cameraController;

    _windowManager = windowManager;

    _statistics = statistics;

    _settings = settings;

    _settings.ShouldUpdate = true;

}

public void Show()

{

    _planet = _planetFactory.Create(DoubleVector3.Zero, PhysicalConstants.RadiusOfEarth);

    _cameraController.AttachToPlanet(_planet);

    _windowManager.ShowAllWindows();

}

Note that the CameraController is injected into the MainPresenter and is explicitly attached to a particular planet which fixed my phantom CameraController problem since now the container actually needs to create it and MainPresenter holds a reference to it.  The bizzaro reference I had in the Genesis program class went away and the handling of MainPresenter is more logical (get it from the container and then show it).

protected override void Initialize()

{

    this.IsMouseVisible = true;

    _mainPresenter = ObjectFactory.GetInstance<MainPresenter>();

    _mainPresenter.Show();

    _inputState = ObjectFactory.GetInstance<IInputState>();

    _inputMapper = ObjectFactory.GetInstance<IInputMapper>();

    SetInputBindings();

    base.Initialize();

}

Finally, the Planet class is simpler and there’s no cascading Initialize calls to the renderer any more:

public Planet(DoubleVector3 location, double radius, ITerrain terrain,

    IPlanetRenderer renderer, IHeightfieldGenerator generator, Statistics statistics)

{

    _location = location;

    _radius = radius;

    _terrain = terrain;

    _renderer = renderer;

    _generator = generator;

    _statistics = statistics;

    _clippingPlanes = new ClippingPlanes();

}

You can see the GitHub Compare View of all of the changes here.

The GenesisEngine Project

(I originally posted this on my MSDN blog.)

I’ve been working on a personal project off and on for awhile now.  It’s called GenesisEngine and it’s a program that generates and renders a procedurally-generated planet.  I can take in the view down at ground level with the terrain generated at ~1-meter precision, or I can zoom all the way out into high orbit and see the entire planet at once.  This requires relatively little memory and no storage because the whole landscape is generated on demand via fractal algorithms.

Now that I’ve got it to a decent alpha state where it renders mildly interesting terrain I decided to throw it up on GitHub for public amusement.  I don’t have any grand plans for this project; it’s just a little laboratory where I can experiment with new code design ideas or new tools, and in the process maybe build something that looks cool and teaches me more about graphics and other stuff that I’m interested in.  It’s already been a useful experience for me because I’ve taken several concepts that I read about, tried them out in GenesisEngine, and incorporated them into my code bases at work.

Along the way I plan to blog about the lessons I learn.  I’m certainly not blazing any new trails here; I’m just picking up good ideas from people who are much smarter than me and implementing them in a non-trivial project.  I hope my successes and failures, and the source code itself, might be of use to others who want to learn.

The code can be found at at http://github.com/SaintGimp/GenesisEngine.  Git is another thing I’ve been learning about thanks to GenesisEngine.  It’s well worth the time to explore either Git or Mercurial or both if you’re not already familiar with distributed version control systems.  It’s slightly bewildering at first to a Perforce/TFS guy but I’m beginning to see what people are so excited about.

If you want to download and build it you’ll first need to install Visual Studio 2008 (you can use the C# Express Edition if you like) and XNA Game Studio 3.1.  Everything else should be included in the repository.  You should probably check the README file for additional information.

I want to emphasize a couple of points:

  • This is absolutely not a Microsoft sample/demo/guidance project.  This is my own work that I’m doing on my own time.
  • There are areas of the code that I’m pretty happy with and areas that I know are abominable and need to be reworked.  This is not a tutorial or a best-practices showcase.  This is just a real-world developer working on real-world code.  I suspect the best lessons to be drawn from this project will be not how the code looks at any particular point in time but rather how it grows and evolves over time.

Ok, is that enough caveats, self-deprecations, and disclaimers?  Anyway, here are a few screenshots to illustrate what I’ve got so far.  There’s no texturing or atmosphere yet so think “ice moon”.  The red lines mark the boundaries of the individual terrain patches.

On the ground:

image_10.png

At airplane level:

image_2.png

Low orbit:

image_4.png

High orbit:

image_6.png

 

My BDD Naming Macro

(I originally posted this on my MSDN blog.)

Over the years several people have shared the Visual Studio macros they use to make the BDD boxcar naming style easier to work with.  I thought I’d add my own, not because it’s any better than the others but because it’s built for a slightly different workflow and someone might find it useful.

First, here’s the macro:

Imports System

Imports System.Windows.Forms

Imports EnvDTE

Imports EnvDTE80

Imports System.Diagnostics

Public Module BDDNaming

    Public Sub ReplaceSpacesInTestNameWithUnderscores()

        If DTE.ActiveDocument Is Nothing Then Return

        Dim selection As TextSelection = CType(DTE.ActiveDocument.Selection(), EnvDTE.TextSelection)

        If selection.IsEmpty Then

            ReplaceSpacesAtEditPoint(selection)

        Else

            ReplaceSpacesInSelection(selection)

        End If

    End Sub

    Private Sub ReplaceSpacesInSelection(ByVal selection As TextSelection)

        Dim text As String = selection.Text

        text = text.ToLower()

        text = text.Replace(” “, “_”)

        text = text.Replace(“”””, String.Empty)

        selection.Text = text

    End Sub

    Private Sub ReplaceSpacesAtEditPoint(ByVal selection As TextSelection)

        selection.CharLeft(True, 1)

        While selection.Text(0) <> “””” AndAlso (Not selection.ActivePoint.AtStartOfLine)

            selection.CharLeft(True, 1)

        End While

        If selection.Text(0) = “””” Then

            ReplaceSpacesInSelection(selection)

            DeleteTrailingQuote(selection)

        Else

            selection.CharRight(False, 1)

        End If

    End Sub

    Private Sub DeleteTrailingQuote(ByVal selection As TextSelection)

        selection.CharRight(True, 1)

        If selection.Text(0) = “””” Then

            selection.Delete()

        Else

            selection.CharLeft(False, 1)

        End If

    End Sub

End Module

I usually bind this macro to Alt– (Alt-[dash]) because it’s easy to remember and it’s not bound by default to anything important.

To use it, I start by typing an open quote mark where I want to type a BDD context or spec name.  I use Resharper so it automatically inserts two quote marks for me, but the macro works equally well without Resharper.  The quotes prevent Intellisense from freaking out as I start to type the context or spec name:

    public class “”

 

Then I type the context or spec name as a normal sentence using the space bar like so:

    public class “when a message with no eligible listeners is sent”

 

Then I hit Alt– to convert the sentence to a proper boxcar-style identifier:

    public class when_a_message_with_no_eligible_listeners_is_sent

 

I can also highlight any arbitrary piece of text and hit Alt– to convert the spaces to underscores.

Other people like to put a lot of boilerplate code into the macro to make it easier to set up contexts and specs quickly, but I prefer this macro that does just one thing and does it well.  Hopefully someone else will find it useful too!

 

Hiring Managers for Agile Teams

(I originally posted this on my MSDN blog.)

Someone asked a question along the lines of, “Let’s say you were hiring senior managers for a group made up of multiple agile teams.  What qualities would you look for in an interview?”

The very first thing I’d look for would be a strong understanding of and belief in agile principles and philosophy.  There are a lot of people out there who say, “Yeah, sure, agile’s great!” but they panic and revert to non-agile strategies when the going gets tough.  Does the candidate deeply understand not just how agile works mechanically, but why it works and the factors that can help or hinder success in that model?  Do they know how to fix it when it breaks?

The absolute worst thing you can have when in a difficult situation is a senior manager who doesn’t truly trust agile and knee-jerks back to traditional methods when the risks get high (which of course is precisely when traditional methods don’t work).  A thrashing manager can absolutely destroy even a strong agile team.

I guess I could drill down into specific beliefs and name things like support for self-directed teams, a passion for delivering real value rather than just sticking to a pre-determined plan, and a willingness to blur traditional role lines (dev, test, PM) in the interest of a cohesive team.

Of course, many of the usual things like the ability to hire and grow quality people, real-world experience with shipping software, a strategic understanding of the business environment, etc, are still critical.  That stuff doesn’t go away.