Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Engine is a singleton without the protections thereof. #43

Closed
Delwin9999 opened this issue Jan 14, 2013 · 6 comments · Fixed by #57
Closed

Engine is a singleton without the protections thereof. #43

Delwin9999 opened this issue Jan 14, 2013 · 6 comments · Fixed by #57
Milestone

Comments

@Delwin9999
Copy link

Engine has the private static _instance attribute and assignment to this of a singleton but the constructor is public. This means someone could instantiate it twice thinking they'd get the same instance (as you would with a real singleton) but in fact they'd overwrite the first one's _instance with the second one and the game state would be unrecoverable.

This should be a real singleton pattern. The only reason it isn't right now is that the unit tests fail when I tried to do the conversion on my branch. When I become more familiar with the unit test harness I will be able to untangle that and I can throw a pull request for the fix at you.

@bonesoul
Copy link
Owner

Yes, I had thrown some random _instances' all over the code, which should only exists for the Engine.cs itself and you are right, that singleton have to be fixed.

So I think actual singleton should be like;

    private static Engine _instance =new Engine(); // the instance.

    /// <summary>
    /// Returns the memory instance of Engine.
    /// </summary>
    public static Engine Instance
    {
        get { return _instance; }
    }

and the Engine should have private static constructor.

Then we have to find another method to let config to be attached that could be similar to something like;

    public void Run(Game game, EngineConfiguration config)
    { 
                this.Game = game;
                this.Configuration = config;

                config.Validate(); // validate the config.
    }

Update:
mm, you are right that will break how tests works - ie the ChunkStorageTests.
So maybe we can just fix the problem temp. until we sort out all the stuff - 9235b2e

@bonesoul
Copy link
Owner

bonesoul pushed a commit that referenced this issue Jan 15, 2013
…t. Engine is now free of both Nini reference and the ini file itself. The actual game show now do what ever it wants (ie . reading from the ini file) supply parameters to EngineConfig.

The Engine ctor() will now throw an exception when additional instances created (Discussed in issue #43).
Moved EngineConfiguration class to Core/Config and renamed as EngineConfig. Also added AudioConfig and GraphicsConfig classes.
Renamed Game.cs in Game project to SampleGame as it was conflicting with XNA.Game.
Removed GeneratedblockStorageTest.cs as it was reduntant.
Tests will now check if Engine is already initiated and dispose it first on test startup.
Also added Test teardown methods.
@bonesoul
Copy link
Owner

I've further worked on the issue and I think in an library (or in our case the engine) using singletons is all evil. So I'll be further re-factoring them out as much as I can. Maybe be we could let Engine.Instance stay until we find a better solution working for both the library itself and the tests.

bonesoul pushed a commit that referenced this issue Jan 15, 2013
@bonesoul
Copy link
Owner

Only Rasterizer.Instance left now other than Engine.Instance which later needs more work.

@bonesoul
Copy link
Owner

  • 563789e SettingsReaders are no more singletons.
  • e6b7e79 AssetManager fixed.
  • 87ffdbd ChunkStorage and InputManager fixed.
  • 449823b - Fix Rasterizer.Instance.

@bonesoul
Copy link
Owner

Further architectural discussion: #50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants