Integrate gestalt-module with Terasology, replacing previous module system

[immortius]

No description provided.

[msteiger]

1. I'm getting a weird compiler error when using gradle run (with the Java8_05 compiler - it works with Java7_60):

engine\src\main\java\org\terasology\reflection\metadata\ClassMetadata.java:99: error: incompatible types: Object cannot be converted to Field for:
(Field field : ReflectionUtils.getAllFields(clazz, includedFieldsPredicate)) {

I could fix it by explicitly specifying the type of the last parameter as:
Predicate<? super Field> includedFieldsPredicate

2. The NameGenerator module does not work anymore: AssetManager.loadAsset() fails for the last(?) prefab. Am I missing something here?

[immortius]

1. This is a preexisting issue with Terasology and JDK 8. It is not relevant for the PR, although it needs to be separately addressed.

2. More information? What error are you getting?

[msteiger]

1. Ah right. This was the only issue with Java 8 though.

2. To reproduce, you just need to create a new game with Core and NameGenerator. I uploaded the tail of the log here: http://pastebin.com/BphJbjtc

[Cervator]

Could it be a package swap between sun.reflect instead of java.lang.reflect or something like that somewhere? Since generally sun stuff is shunned

I ran it through my integration workspace with all "stable" modules - the following modules look to have compile errors:

Cities - @msteiger
Fluid - @MarcinSc
LightAndShadow - ? @synopia ?
Machines - @Josharias
MasterOfOreon - @mkienenb
Miniion - @mkienenb @overdhose
NameGenerator - @msteiger
PlantPack - @MarcinSc
TerraTech - @Josharias
WoodAndStone - @MarcinSc
Workstation - @MarcinSc

I figure most are simple fixes just tweaking packages and methods. Ideally we'd get the primary maintainer to fix, but should we try to be proactive, especially where the current maintainer is unavailable or low on time? LightAndShadow comes to mind, it needs some extra attention anyway.

Overall: Very impressive stuff :-)

We can push it to the Nanoware repos if we want to do some Jenkins testing / bundle with modules to see if any runtime/binary issues pop up. Can fork any missing modules for extra testing.

[msteiger]

Good point. Most compiler errors I encountered should be easy to fix. I can also look into some of the un-maintained ones if you want.

[immortius]

I recognize the error - previously I had sun.reflect hardcoded as an API package, but removed it in the change. Apparently that package needs to be API because Java will automatically call it when it first encounters a class.

I don't know what this means for non-Sun java distributions though.

[msteiger]

Will test NameGenerator and try to fix the errors in Cities tonight (or tomorrow) and see if it works.

OpenJDK has the same packages as the Oracle implementation, so this shouldn't cause any problems. It works with IcedTea on Linux Mint at least.

[MarcinSc]

I will wait to fix any issues in the mods I work on, once this is merged into develop.

[msteiger]

Both Cities and NameGenerator work.

[mkienenb]

Currently driving thu Iowa, Minnisota, South Dakota in last hour. On the
way to Montana. Back in New York mid July.

On Tuesday, July 1, 2014, Martin Steiger notifications@github.com wrote:

Both Cities and NameGenerator work.

Reply to this email directly or view it on GitHub
#1152 (comment)
.

[MarcinSc]

Any chance of changing the toString() method in Name to either return normalised, or introduce a method to the class that returns it?

[immortius]

I don't want to change toString(), as I want to keep any camel casing for display purposes, and store it that way. At the moment, I don't see any reason to return a "normalised" string - it is merely an implementation detail. If it is for comparison or hashing, makes more sense to use the name (and convert what you want to compare it to into a name). Effectively Name doesn't have a normalised form, it merely guarantees case-insenstive comparison and hashing.

[MarcinSc]

BTW ColorTextureAssetResolver will not work for that reason, and will not be able to distinguish between "Color:XXX" and "color:XXX". It also doesn't work right now due to:
if (!"engine".equals(uri.getModuleName())) {

[MarcinSc]

Without the access to normalised string, most of the AssetResolvers might have problems, and it's something I use quite a bit for generated assets.

[Cervator]

@mkienenb - nice! Enjoy your trip, we'll keep on the lights :-)

I added a slight tweak to let standalone modules build, otherwise looks good, just need a few more module fixes

[immortius]

@MarcinSc
Ok, having slept on this:

• if (!"engine".equals(uri.getModuleName())) { is simply a bug, "engine should be replaced with TerasologyConstants.ENGINE_MODULE.
• I had not considered the need to do string manipulation with Names. I would be happy to add a few string methods (toUppercase(), toLowercase(), startsWith(), endsWith()), and a split method that will break a name apart on some separator characters (maybe hardcoded to '.', maybe not). Would that satisfy all needs?

[MarcinSc]

I don't think the built-in split method is required, as everyone will use a different structure for their resources, so probably just the "toLowercase", or "getNormalised" will be enough.

[msteiger]

I noticed that HeadlessEnvironment doesn't have the activateAllModules method anymore which I used in JUnit tests. I re-implemented it like this:

Set<Module> modules = Sets.newHashSet(moduleManager.getRegistry());
moduleManager.loadEnvironment(modules, true);

However, I still get this for the NameGenerator tests:

org.terasology.asset.AssetManager - Failed to resolve PREFAB:elvenMaleNames

What's missing to make it work again?

[immortius]

The test moduleManager's registry doesn't include every module. For unit testing a specific module, you should add that module and dependencies.

[msteiger]

That's surprising. It gives me all modules plus "unittest".
Where do I get the module instance from, if not moduleManager.getRegistry() ?

I tried adding NameGenerator only and adding all available modules (and ModuleEnvironment seems to have them all), but still no luck.

The warning disappears if I qualify the asset with the module name (prefix with "NameGenerator:"), but loading still fails.

[Cervator]

Bump - any luck @msteiger or ideas @immortius ? Would like to get NameGenerator & friends fixed for stable build :-)

[immortius]

I think I need more details. In what way does loading fail?

Additionally you shouldn't just grab every module from the ModuleRegistry, because there could potentially be multiple versions of the same Module - use the Dependency Resolver to pull a compatible set out of the registry based on the module being tested. (see the user doc for details)

[Cervator]

In other news I think other than a quick Rails fix the NameGenerator tests looks to be the only broken thing left. Several of the modules I listed above may have failed from module compilation errors instead

Miinion had a simple fix and with that MOO is all set too

[msteiger]

One problem is that AssetManager is not synced with ModuleManager's environment. I manually set the new env. using:

CoreRegistry.get(AssetManager.class).setEnvironment(modEnv);

This helped, but loading still fails, because the PrefabSerializer does not know the components in the module:

o.t.p.serializers.PrefabSerializer - Prefab contains unknown component 'NameGenerator'

I assumed that the StorageManager needs updating as well, so I set the new module env. there, but it didn't change a thing.

So I tried setting the right module env. when creating the ModuleManager. This fails in EntitySystemBuilder.registerComponents(), because environment.getModuleProviding(CreatureName.class) returns null. The environment contains the NameGenerator module, though.

[immortius]

That last error is because you need to add the module being tested as a classpath module, as it is on the classpath when running unit tests.

[msteiger]

Ka-ching - found it! Apparently, the assets folder has to reside inside the src/main/resources folder now. Is that correct? I wish it hadn't took me so long to find this out.

• Maybe this info could be part of some kind of "Upgrade-module" guide?
• TODO: move the assets folder in other modules (e.g. Miniions) too

Similarly, I moved the module.txt into the assets folder (as in engine) - is that the correct location? However, this breaks the ModuleManager constructor which tries to load the engine's module.txt with getClass().getResourceAsStream. Since NameGenerator has the same file in the same (relative) folder, it returns the NameGenerator's module.txt file instead.

[Cervator]

Is that specific to unit testing? Since modules are working normally without any file movement (tested a few). Plus unit tests in modules are still pretty rare so easy for them to go funny

[immortius]

Oh, hmm.

For PathModules (so a directory), the assets path is supposed to be in the root path. Same for ArchiveModules (root of the zip).

For classpath modules, the assets path is expected to be in the root of the classes folder, since that is the classpath.

This all works well except for unit tests, since the module being tested is on the classpath, but the assets folder won't be there.

I'll have a think about this.

On module.txt, it should be at the root of the module (not under assets). And yes, again this is a problem when unit testing a module as you point out.

[Cervator]

I split this out into a new issue so we can stop talking in a closed one :-)