Introduce the concept of a Context to slowly get rid of CoreRegistry.

[flo]

Reasoning: The core registry is just like static variables and it prevents
cool stuff like having a client and headless server running in the same VM.

[GooeyHub]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/72/

Build Log
last 5 lines

[...truncated 4381 lines...]
Build step 'Publish JUnit test result report' changed build result to UNSTABLE
[ANALYSIS-COLLECTOR] Computing warning deltas based on reference build #71
Notifying upstream projects of job completion
Setting status of 0a4b47f420da4365ca702b9ef798a3f2dd963d0b to FAILURE with url http://jenkins.terasology.org/job/TerasologyPRs/72/ and message: Merged build finished.

Uh oh, something went wrong with the build. Need to check on that

[GooeyHub]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/73/
Hooray Jenkins reported success with all tests good!

[GooeyHub]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/74/
Hooray Jenkins reported success with all tests good!

[msteiger]

It would be great if you could summarize the changes in a few sentences as the PR is quite large.

As I see it, a context is set in constructors of various classes and then used to replace the CoreRegistry? Is that the idea?

[Cervator]

Tests out OK locally, although yeah I only get the general idea of the PR and the overall goal :-)

Relates to http://forum.terasology.org/threads/making-singleplayer-a-special-case-of-multiplayer.1291

The only module compile error I noted is in TinyEnvironment in PolyWorld, where the CoreRegistry.putPermanently seems to be gone - what would the replacement be?

CoreRegistry.putPermanently(WorldGeneratorPluginLibrary.class, new WorldGeneratorPluginLibrary() {

[Cervator]

PolyWorld should be happy with Terasology/PolyWorld#3 merged after. Thanks @flo !

Pinging @immortius and @MarcinSc for further review :-)

[flo]

@msteiger the long term goal of this pull request series is to get rid of static variables so that it is for example possible to introduce a test mode in which a client connects to a headless server in a single JVM, possibly with configurable delay.

The main change of this pull request is the intrduction of a Context class.

To make the transition smooth the CoreRegistry class and it's most frequently used method "put" and "get" will remain. To make it possible to use either the non static Context object, or the static CoreRegistry methods the CoreRegistry will now internally work with a Context object.

The Context object provides non static put and get methods that equal in it's functionality those of CoreRegistry. The Context objects are hirachical. The TerasologyEngine class and only that class has access to a engine Context object which stores all objects that exist for the whole lifetime of the TerasologyEngine instance. All objects that got previously added to the CoreRegistry via putPermanently will go into that Context. The putPermanently method got removed from CoreRegistry. It got only used by a test of the PolyWorld module that is not yet considered stable.

For it's other states the TerasologyEngine offers now a method to create a child context. If a child context does not find an object then it will look it up at it's parent. Thus all objects that get for example registered during the loading of the game will only be in the child context that lifes as long as the game goes. The next game has a new context and will thus no longer contain the objects that got added to the context of the previous game.

Until the CoreRegistry usage has been removed completly, it will be necessary to set a context for it. During the initialization of the engine it is set to the engine context. Afterwards it is set to the context owned by the game states.

The main focus of the patch is the introduction of the Context object, it is however also removing a bit of CoreRegistry usage as a side effect.

[emanuele3d]

A couple of side questions:

As far as I understand the CoreRegistry only allowed to register one instance per class. Would it make sense for these Context objects to be more flexible, i.e. able to register any object against a simple string?

Also, in line with my recent suggestion of making Config Observable, it would be useful perhaps for these Context objects to be observable too? It would be useful to be informed if a context-held object has been replaced, removed or has even become available.

[MarcinSc]

I'm not sure if Context objects are supposed to be changing, its main purpose is going to be to set the @in dependencies in systems, and mutating those might require a lot of diligence in each system to be able to respond to those changes.

[flo]

@emanuele3d I thought about string keys too, but in the end I think it's better to register just one instance per class: It is simple and nice solution (e.g. type savety, no need to enforce name conventions) and if you really need to have multiple instances of a class then you can still register a Manager class that allows you to get instances by string key. e.g. like a AssetManager.

Yes and I agree with @MarcinSc. They are supposed to be static due to their use in @in and the big question what would happen with existing injected classes when you change it later on.

[emanuele3d]

Good point about registering Managers to obtain instances or other types of objects by string.

Regarding the registered objects being unchanging: good to know that's the aim. I'll double-check what I'm doing in renderland to make sure the objects I register do not change.

Thank you both for your replies.

[msteiger]

Thanks - sounds good to me.

[Cervator]

Bump - @immortius: got a moment to look at this sometime please?

[immortius]

Given there is a public getContext() method I would suggest making this private, unless subclasses are replacing the context?

[flo]

I had it first private too, but the field gets used a lot in the sub class HeadlessEnvironment and I think:
context.put(...)
reads better than:
getContext().put(...)

[immortius]

Ran my eyes over it. I see no fundamental issues with the approach, but found a couple of areas needing tweaks. Also suggest name change from Context to Registry (or similar), as I feel Registry describes what the class is better, and the contextual nature of it is how it is used - the class itself does not represent a context as such.

[flo]

I think Context is a good name, as:

1. you have one instance per context. e.g. server and client context
2. it contains the objects that makes up the context e.g. the entity manager that belongs to that context
3. the name context implies that there is another instance per context, which is exactly what I want to express. Just calling it Registry would you make assume that there is just one instance.

[immortius]

I don't really like naming classes by how they are used rather than what they do (List doesn't have anything to say about whether there is one or many of them) and disagree that Registry implies singleton, but not really bothered enough to argue further.

[GooeyHub]

Refer to this link for build results (access rights to CI server needed):
http://jenkins.terasology.org/job/TerasologyPRs/76/
Hooray Jenkins reported success with all tests good!

[Cervator]

Any further comments on this or should we go ahead and merge?

I can see both sides on Context vs. Registry. There is only one solution - Google Fight!

http://www.googlefight.com/index.php?word1=context&word2=registry

Context wins by about double and for some reason I'm getting results in Spanish French

Joking aside I really don't want to pick a winner on arbitrary grounds, but as long as work gets done I'm happy :-)